Remote completion on untrusted devices is incorrect

[imsodin]

Reported in https://forum.syncthing.net/t/remote-device-status-on-encrypted-end/16577

The version vector on an encrypted file info has it's counter set to now whenever it's sent:

	// The vector is set to something that is higher than any other version sent
	// previously, assuming people's clocks are correct. We do this because
	// there is no way for the insecure device on the other end to do proper
	// conflict resolution, so they will simply accept and keep whatever is the
	// latest version they see. The secure devices will decrypt the real
	// FileInfo, see the real Version, and act appropriately regardless of what
	// this fake version happens to be.

	version := Vector{
		Counters: []Counter{
			{
				ID:    1,
				Value: uint64(time.Now().UnixNano()),
			},
		},
	}

That means if two devices are connected to an untrusted device, the second device sending an identical file-info, will send it with a newer version. Thus to the untrusted device the first device looks out of sync. The first device, having de-crypted the version, doesn't care.

Synchronisation still works fine, even if multiple untrusted devices are involved. It just causes a few unnecessary index sends. For the remote status I see three options:

1. Just don't display the status of remotes on untrusted nodes.
2. Don't encrypt version vectors.
3. Devise some scheme to get consistent encrypted/fake versions on all devices and that still is correctly ordered from the point of view of the untrusted device (e.g. just hashing won't do).

An option for 3. would be to keep using id 1, together with the maximum of all counter values in the plain vector (haven't thought much about possible corner cases yet :) ). However I am currently not sure it's worth encrypting the version at all: An untrusted node can already obtain the information about who/when files are modified, by observing from whom and when it receives index updates. That's not always possible (e.g. when receiving indexes indirectly, i.e. not from the device that changed the file) and less precise than having all the counters. Still I am not sure knowing who edited a file and when is sensitive information within our threat model. And the less difference there is between an encrypted file info and a plain one, the less issues like this will crop up with the implementation approach taken (untrusted devices work almost the same as any other device).

[calmh]

I don't think leaking version vectors is acceptable as it essentially lists the device IDs of everyone in the cluster. A single entry with the sum of the other version values should work I think (but I didn't think deeply about it...). :)

[klimkiewicz]

@imsodin - I think the problem still exists, although it's harder to trigger. I have three devices:

TrustedA <-> Untrusted <-> TrustedB

TrustedA talks to Untrusted only, and TrustedB talks to Untrusted only (TrustedA & TrustedB don't talk to each other).

First TrustedA & TrustedB are both in sync. Let's say they only track FileA. The version vector for the file (on both machines) looks sth like: {{TrustedA, 1638435678}, {TrustedB, 1638435170}}. Untrusted sees the vector as {1, 3276870848} (sum of counters of TrustedA & TrustedB). They now both go offline and we modify the file on both machines.

TrustedA now has vector of {{TrustedA, 1638435766}, {TrustedB, 1638435170}}. Untrusted vector is {1, 3276870936}.
TrustedB now has vector of {{TrustedA, 1638435678}, {TrustedB, 1638435740}}. Untrusted vector is {1, 3276871418}.

(Sorry for these big timestamps, but they're actual values from debugging session). Note how latest modification is on TrustedA but the biggest untrusted vector is the one from TrustedB.

Now TrustedA sends IndexUpdate to Untrusted, and the latest version on Untrusted becomes {1, 3276870936}. Untrusted doesn't fetch the file from TrustedA yet, but instead TrustedB connects and sends its IndexUpdate to Untrusted and the latest version on Untrusted becomes {1, 3276871418}. Untrusted fetches the file from TrustedB and sends IndexUpdate to TrustedA. But TrustedA is not convinced by this IndexUpdate, as it thinks it has the latest version (according to "unencrypted" latest modification date of the file) and so it expects Untrusted to fetch its version of the file. And it shows "Syncing..." message until another change on the file occurs. I hope the above is somewhat clear ;).

It seems to me the sum of version counters is not the correct approach here. The maximum value from the version vector would probably work, but it leaks the latest modification date of the file, so that may be problematic too (although this can be inferred from the network communication rather easily as well).

What do you think? Thanks!