Discussion: Hardening of remote sync commands against injection

[phoerious]

Have you searched for an existing issue?

• Yes, I tried searching and reviewed the pinned issues

Brief Summary

Concerns have been raised about potential command injection in the new experimental remote sync feature.

The executed command comes from the encrypted part of the database and is therefore considered trusted input. However, if a database is used in a team, a rogue team member could potentially change the sync command to something malicious. This would not result in a loss of confidentiality of the database (the team member has access already), but it could be problematic nonetheless.

Potential mitigations:

1. Split commands beforehand and store as a list of arguments, which is executed without a shell. This is not really a mitigation of the problem, but it would make the command more robust in general.
2. Generate a random machine ID and store it in the user's local settings (unecrypted). Inside the database, store hash(command + template + machine-id). Commands are compared against this hash before being executed, and the user is asked to trust a command before running it if no matching hash is found.
3. Same as 2., but store a secure salt inside the database and store hash(command + template + salt) in the user's local settings.

Mitigations 2. and 3. are alternatives. 2. would be best, but could create a lot of rubbish in the database over time. We could tie the hashes to a plain-text hostname to reduce the amount of accumulated rubbish.

[droidmonkey]

We technically have a salt in the form of the database or root group uuid

[phoerious]

Especially for option 3, I would prefer a separately generated Salt. The root group UUID is probably fine, but it is not guaranteed to be generated with a safe PRNG.

[droidmonkey]

But we aren't making cryptographic guarantees with a simple hash check for first run checks

[phoerious]

Since the command + template are considered secret information, we should protect it accordingly. Thus for option 3, we would need a strong hash function + a secure salt.

[phoerious]

CC @t-h-e as main contributor.

[t-h-e]

Good catch. One should definitely not run random commands on their machine without being informed beforehand.
Option 3 looks good to avoid storing unnecessary data within the database.
I don't have a strong opinion regarding the hash though.

@phoerious What are your concerns regarding the salt in the form of the database or root group uuid? That someone could get their hands on the hashes and be able to decrypt the command + template in case of a "weak" salt?

[phoerious]

Not decrypt, but run brute force or run known-plaintext attacks, yes.

[varjolintu]

Especially for option 3, I would prefer a separately generated Salt. The root group UUID is probably fine, but it is not guaranteed to be generated with a safe PRNG.

It's possible that user has two databases with an identical root group UUID.