Repository: syncthing/syncthing (language: go)

Issue:
Mutual encrypted sharing doesn't work (both sides with password)
Please forgive me if I'm wrong here, but I remember this being mentioned on the forum as untested, but as something that "should still work". However, in my testing, it seems to always fail with a "generic error".

1. On Device 1, add a new folder filled with some files to Syncthing.
2. Share the folder with Device 2, setting a password on it.
3. On Device 2, accept the folder, but change the type to "Send & Receive", while also inputting the same password.
4. The two devices connect and try to sync the files, but the process fails with a "generic error".

The final result looks like this:

![image](https://user-images.githubusercontent.com/5626656/163138964-bf492b63-7915-4cac-a692-33b78b8a2a14.png)
![image](https://user-images.githubusercontent.com/5626656/163138972-a98cf008-3bcd-4117-b591-d32fa4b2c0c6.png)
![image](https://user-images.githubusercontent.com/5626656/163138979-06e875cf-c417-4f82-957e-8a2555a8f64b.png)

Below are logs with `db,model` enabled:

- [syncthing1.log](https://github.com/syncthing/syncthing/files/8479798/syncthing1.log)
- [syncthing2.log](https://github.com/syncthing/syncthing/files/8479800/syncthing2.log)

All testing was done under Windows 10, on two different PCs. The problem isn't limited to the latest code either, as I've tested Syncthing versions up to v1.18.6, and the same errors pop up on all of them.
lib/protocol: Correctly request encrypted block (fixes #8277)
An untrusted device includes an "encrypted block hash" in the request for properly locating and verifying the requested data. We didn't add the same thing to the request that an encrypted but trusted peer sends, so running encrypted-to-encrypted didn't work. This resolves it. I don't want to figure out a test for this, sorry.

lib/protocol: Correctly request encrypted block (fixes #8277)
An untrusted device includes an "encrypted block hash" in the request for properly locating and verifying the requested data. We didn't add the same thing to the request that an encrypted but trusted peer sends, so running encrypted-to-encrypted didn't work. This resolves it. I don't want to figure out a test for this, sorry.



Discussion / hints:
I think it's meant to disallow any other folder type as long as the password is empty. Maybe that logic is missing or broken in the GUI code. What would be the purpose for choosing other types without giving the password? 
Can you elaborate? I'm not sure I quite understand. Just to be clear, here we're talking about a situation where both sides have been given the password.

To be more specific,

SR/RO (password) --> RE (no password) - syncs fine
RE (no password) --> SR/RO (password) - syncs fine
SR/RO (password) --> SR/RO (password) - generic error
I thought you were talking about a situation where the password is not given on one side. Aren't your first two examples the same, just backwards? 
> I thought you were talking about a situation where the password is not given on one side.

Oh no, when there is no password, the devices don't even connect displaying an error message. The problem happens only when there is the same password on both sides, yet neither of the two folder types is set to Receive Encrypted.

> Aren't your first two examples the same, just backwards?

Yeah, it's the same thing eventually, but I just wanted to point out the direction of sharing the folder in case there's any difference in relation to it.
> same password on both sides, yet neither of the two folder types is set to Receive Encrypted.

This is something that used to work, sort off, but it was never a designed feature as there is no point to it (?), so it may have been broken by subsequently added checks.
> but it was never a designed feature as there is no point to it (?)

I was actually thinking about this as a possible short-term solution, e.g. when we've a local Receive Encrypted folder that was shared with us previously. One day, we decide that we don't want to encrypt the files anymore and would rather prefer to have it set as SR or RO. However, the original device is located in a place that we've got no immediate access to. Then, at least temporarily, we could just input the original password and sync the folder unencrypted.
Yeah I could see that being a use case. It's gotta be a small corner of the already small niche though.
That's obviously true 😉. If too difficult or complex to implement, then at least having a useful error message would be nice regardless.
I don't think there's a good reason for it to *not* work, so it's probably just a question of figuring out why it doesn't. (And perhaps figuring out a way to add a test.)
Hi! I have occured this problem. I've got 2 devices with setting "unsecured" on both. Can't sync anything.
> Hi! I have occured this problem. I've got 2 devices with setting "unsecured" on both. Can't sync anything.

Not sure what this means. If you need help with your setup resp. a problem you encounter, please use the forum - https://forum.syncthing.net/ - for support, first by searching for any similar issues and then by posting a new one when required. The forum has a lot more eyes and friendly people on it than the issue tracker.
Encrypting the hash will be useless overhead for the most/intended usecases, as a receive-only folder will just ignore this hash. I'd rather handle the case of no blockshash such that the deficiency is on the unintended usecase. That should already work when just forwarding the nil blockshash in the `encryptedModel`, the model itself looks like it can already deal with no blockhash.  
Does this make sense?
It does, and I'm not really sure why we do what we do here to be honest. There's also a thing about passing the original block offset as additional data in the encrypted hash which ... I'm not sure why we do. :D
So disregarding the thing about the offset that I haven't investigated why we do it, there's some (necessary, I think) asymmetry in how the requests are handled. 

When a *trusted* device requests data from an *untrusted* device, they just pass the relevant offset/size and maybe fictitious hash, and the untrusted device reads the data and can do nothing to verify it. The trusted side verifies the data by way of decrypting it, but not (as far as I can see) that it matches the expected plaintext hash...

When an *untrusted* device requests data from a *trusted* device it gets the encrypted hash as part of the request, decrypts that hash and uses it to verify the data that is being read from disk. (Same as when we serve a normal request from a trusted peer.) It then obviously does no further verification of the received data.

I think it has some value to retain the second part, as the hash check when reading from disk is how we catch certain database-out-of-sync-with-disk conditions.
Agreed we should retain that, in untrusted to trusted conns. I think we should just drop it in the case of trusted to trusted but both en-/decrypting sent data, instead of alwasy sending an encrypted hash from a trusted to a untrusted device - which in most cases will be useless. So have a condition in `encryptedModel.Request` that it detects a missing hash and just handles it without a hash, instead of currently erroring as it can't decrypt it, and keep decrypting if there is something to decrypt.

Didn't remember the offset bit either, found this explanation:

```
		// The offset goes into the encrypted block hash as additional data,
		// essentially mixing in with the nonce. This means a block hash
		// remains stable for the same data at the same offset, but doesn't
		// reveal the existence of identical data blocks at other offsets.
```
Thanks for digging up the bit about the offset. I disagree about the uselessness however. We do a hash check of the data read from disk for requests from normal devices, and we do it for requests from untrusted devices, why not do it in this case? I think it's equally use{ful,less} as in the regular device case (except for the fact that the "both devices runs encryption" use case is dubious to begin with, but somewhat plausible corner cases have been produced for it...)
Am I missing something? Afaik it's useful for the case of trusted to trusted with encryption, but it doesn't do anything for trusted to untrusted: The trusted side encrypts the hash on request, the untrusted side ignores the hash (as it's not tied to the data content). So we either send the encrypted hash for the validation in the trusted to trusted with encryption case, doing useless encryption computations in the trusted to untrusted case. Or we don't get validation and no computation cost respectively. Given trusted to trusted with encryption is not recommended to do, I'd rather go with no computation cost.
Not sure if you're missing something, or I am, or we just disagree. :) To be sure we have the same world view I made a diagram of mine. This shows what happens with a *response* (assume a *request* just went in the other direction) -- whether the response data is verified when read from disk, verified before being written to disk, and where it's en/de-crypted.

<img width="784" alt="Screenshot 2023-03-13 at 07 48 38" src="https://user-images.githubusercontent.com/125426/224627950-9b1598cf-8366-4d77-90e4-b1dd6ca03d0f.png">

We're talking about the to-be-or-not-to-be of the yellow highlighted verification in the bottom left, right? I think it nicely mirrors the one in the normal case at the top left, and provides "drift detection" of the data on disk which we think is valuable in the normal case. 

(In all cases the verification-on-read on the left side is against the hash we got in the request, so we need to encrypt that hash into the request in order to be able to do the verification...)
 Yep, ok we are in sync on the understanding :)  

I'd just rather get rid of the overhead of encrypting that hash when it gets discarded in all expected use-cases (sent to untrusted), and in reality likely in most cases, and at the same time lose verification in the yellow part in the unexpected use-case. At the same time it's not like it matters very much either way IMO.
Encrypting the hash will be useless overhead for the most/intended usecases, as a receive-only folder will just ignore this hash. I'd rather handle the case of no blockshash such that the deficiency is on the unintended usecase. That should already work when just forwarding the nil blockshash in the `encryptedModel`, the model itself looks like it can already deal with no blockhash.  
Does this make sense?
It does, and I'm not really sure why we do what we do here to be honest. There's also a thing about passing the original block offset as additional data in the encrypted hash which ... I'm not sure why we do. :D
So disregarding the thing about the offset that I haven't investigated why we do it, there's some (necessary, I think) asymmetry in how the requests are handled. 

When a *trusted* device requests data from an *untrusted* device, they just pass the relevant offset/size and maybe fictitious hash, and the untrusted device reads the data and can do nothing to verify it. The trusted side verifies the data by way of decrypting it, but not (as far as I can see) that it matches the expected plaintext hash...

When an *untrusted* device requests data from a *trusted* device it gets the encrypted hash as part of the request, decrypts that hash and uses it to verify the data that is being read from disk. (Same as when we serve a normal request from a trusted peer.) It then obviously does no further verification of the received data.

I think it has some value to retain the second part, as the hash check when reading from disk is how we catch certain database-out-of-sync-with-disk conditions.
Agreed we should retain that, in untrusted to trusted conns. I think we should just drop it in the case of trusted to trusted but both en-/decrypting sent data, instead of alwasy sending an encrypted hash from a trusted to a untrusted device - which in most cases will be useless. So have a condition in `encryptedModel.Request` that it detects a missing hash and just handles it without a hash, instead of currently erroring as it can't decrypt it, and keep decrypting if there is something to decrypt.

Didn't remember the offset bit either, found this explanation:

```
		// The offset goes into the encrypted block hash as additional data,
		// essentially mixing in with the nonce. This means a block hash
		// remains stable for the same data at the same offset, but doesn't
		// reveal the existence of identical data blocks at other offsets.
```
Thanks for digging up the bit about the offset. I disagree about the uselessness however. We do a hash check of the data read from disk for requests from normal devices, and we do it for requests from untrusted devices, why not do it in this case? I think it's equally use{ful,less} as in the regular device case (except for the fact that the "both devices runs encryption" use case is dubious to begin with, but somewhat plausible corner cases have been produced for it...)
Am I missing something? Afaik it's useful for the case of trusted to trusted with encryption, but it doesn't do anything for trusted to untrusted: The trusted side encrypts the hash on request, the untrusted side ignores the hash (as it's not tied to the data content). So we either send the encrypted hash for the validation in the trusted to trusted with encryption case, doing useless encryption computations in the trusted to untrusted case. Or we don't get validation and no computation cost respectively. Given trusted to trusted with encryption is not recommended to do, I'd rather go with no computation cost.
Not sure if you're missing something, or I am, or we just disagree. :) To be sure we have the same world view I made a diagram of mine. This shows what happens with a *response* (assume a *request* just went in the other direction) -- whether the response data is verified when read from disk, verified before being written to disk, and where it's en/de-crypted.

<img width="784" alt="Screenshot 2023-03-13 at 07 48 38" src="https://user-images.githubusercontent.com/125426/224627950-9b1598cf-8366-4d77-90e4-b1dd6ca03d0f.png">

We're talking about the to-be-or-not-to-be of the yellow highlighted verification in the bottom left, right? I think it nicely mirrors the one in the normal case at the top left, and provides "drift detection" of the data on disk which we think is valuable in the normal case. 

(In all cases the verification-on-read on the left side is against the hash we got in the request, so we need to encrypt that hash into the request in order to be able to do the verification...)
 Yep, ok we are in sync on the understanding :)  

I'd just rather get rid of the overhead of encrypting that hash when it gets discarded in all expected use-cases (sent to untrusted), and in reality likely in most cases, and at the same time lose verification in the yellow part in the unexpected use-case. At the same time it's not like it matters very much either way IMO.

Fix patch:
diff --git a/lib/protocol/encryption.go b/lib/protocol/encryption.go
index ea2a4ad01ca..322dfedcfe4 100644
--- a/lib/protocol/encryption.go
+++ b/lib/protocol/encryption.go
@@ -96,18 +96,26 @@ func (e encryptedModel) Request(deviceID DeviceID, folder, name string, blockNo,
 		return nil, errors.New("short request")
 	}
 
-	// Decrypt the block hash.
-
+	// Attempt to decrypt the block hash; it may be nil depending on what
+	// type of device the request comes from. Trusted devices with
+	// encryption enabled know the hash but don't bother to encrypt & send
+	// it to us. Untrusted devices have the hash from the encrypted index
+	// data and do send it. The model knows to only verify the hash if it
+	// actually gets one.
+
+	var realHash []byte
 	fileKey := e.keyGen.FileKey(realName, folderKey)
-	var additional [8]byte
-	binary.BigEndian.PutUint64(additional[:], uint64(realOffset))
-	realHash, err := decryptDeterministic(hash, fileKey, additional[:])
-	if err != nil {
-		// "Legacy", no offset additional data?
-		realHash, err = decryptDeterministic(hash, fileKey, nil)
-	}
-	if err != nil {
-		return nil, fmt.Errorf("decrypting block hash: %w", err)
+	if len(hash) > 0 {
+		var additional [8]byte
+		binary.BigEndian.PutUint64(additional[:], uint64(realOffset))
+		realHash, err = decryptDeterministic(hash, fileKey, additional[:])
+		if err != nil {
+			// "Legacy", no offset additional data?
+			realHash, err = decryptDeterministic(hash, fileKey, nil)
+		}
+		if err != nil {
+			return nil, fmt.Errorf("decrypting block hash: %w", err)
+		}
 	}
 
 	// Perform that request and grab the data.


Test patch: