[Bug] Potential GCM misuse

[yawkat]

I've found two potential bugs in the crypto code of this app.

• https://github.com/machiav3lli/oandbackupx/blob/main/app/src/main/java/com/machiav3lli/backup/actions/RestoreSpecialAction.kt#L58 : This code uses .use on a stream that is backed by a GCM decryption stream. Because of the nature of GCM, the tag verification only takes place at the end of the stream, i.e. when .use finishes. However, the code already replaces files as they come in on the stream. This could lead to a potential attack where the malleability of counter mode is exploited to change some files, and they would be replaced before the tag verification fails.
• https://github.com/machiav3lli/oandbackupx/blob/main/app/src/main/java/com/machiav3lli/backup/handler/Crypto.kt#L159 : Here, the IV is initialized as all zeros, which compromises the security of GCM.

Unfortunately I'm having some trouble testing whether these potential bugs are applicable to the actual app, because the encryption code does not work on OpenJDK (IvParameterSpec does not work for GCM, I assume this works on Android).

[machiav3lli]

And any concrete suggestions to cover up those bugs?

[yawkat]

The first thing should be straight-forward, just extract the full thing (maybe to temp files) before replacing sensitive files. Even better would be just extracting the whole thing as bytes and not touching the tar at all to make it less error-prone, but I don't know if that's feasible. This could be fixed by using a streaming AEAD (like tink does for example) but this would probably break format

I don't know how to fix the second issue without breaking the format.

[ioogithub]

Just to weight in here, rolling your own crypto is a definitely not a best practice. The original oandbackup app had rock solid, proven pgp cencryption support via the actively supported openkeychain app. It would be nice to see this code reintroduced as an encryption option for oandbackupx.

[machiav3lli]

@ioogithub Yeah, I agree it's not the best practice. But gpg's concept/principle has been criticised enough so that I don't really need to spend a word on that.

Its (re-)integration is still not on my short TODO-list. Code contributions to #190 are nevertheless always welcome.

[blaueente]

GPG had the exact same problem, when used in a streaming way as a command line encryption/decryption tool. Although I would not subscribe to the general critique on it. The only real solution is to not trigger any actions until the whole application backup has been verified. Is this "streaming AEAD" a chunked one, where chunks are verified separately? Then the following attack remains: the attacker can just have the update stop (e.g. by corrupting it) at a specific point. Depending on the data structure, replacing half of the files may be enough to make the app's state inconsistent in a way that the attack goal is reached.

[yawkat]

@blaueente Yes, and I believe this has lead to security bugs with GPG in the past.

You can fairly easily defend against this in a backup tool, simply by writing to a temporary file first, and confirming that the full data was received (e.g. with a stop mark), before moving to the restore location. I don't know if tink does this.

[blaueente]

Writing temporary files on flash is something one wants to avoid, for speed and wear reasons. So, if you have direct access to the original file, just rewinding and reading it twice. Might need some access permission trickery to make sure that the original file does not change between the reads.

[yawkat]

Temporary files are not an issue because renaming is basically free. The technique is typically called an atomic file write