New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backup encryption is not using a key derivation function #817

Open
lunar-debian opened this Issue Dec 30, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@lunar-debian
Copy link

lunar-debian commented Dec 30, 2016

Hi!

The encryption mechanism used for backups is currently using the given password as the AES key as ASCII bytes, later padded with 0 to reach the required 256 bits.

This seriously reduces the encryption security as it makes the actual keyspace quite tiny. I haven't verified this, but I suspect passwords will silently be truncated to 31 characters in case they are longer.

Proper key stretching should be implemented. I see it's already implemented in crypto/lollipop/cryptfs.c and crypto/ext4crypt/KeyStorage.cpp, both with scrypt and PKDF2.

I guess one question is how to keep compatibility with older backups. In all cases, I guess trying twice, with derived key and then with the raw password would work.

@lunar-debian

This comment has been minimized.

Copy link

lunar-debian commented Jan 2, 2017

I've pointed this issue to a friend with proper experience in implemeting crypto algorithm whose reaction was the following:

ooooh, openaes does CBC with its own nonstandard made-up padding and header. How fun!
Oh, and this isn't even CBC!
If you model CBC as IV || CBC_ENCRYPT(IV, MSG) ...
they're doing HEADER || CBC_ENCRYPT(HEADER, IV || MSG)
that's going to give you one block of known plaintext at the start of every message.
trivial to find two messages encrypted with the same key that way.
oooooh, IV made from the ISAAC RNG, with a fallback to rand()!
and the padding on the last block is just a bunch of '0' bytes
wait, no , there's some kind of weird padding
ah, of course. They pad with a prefix of [00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f].
There's probably a padding oracle attack in there.
*with a prefix of [01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f].
Also the format is is malleable, but that's the least of your worries
summary: Don't use openaes. If you must use it, don't use it like this.

@rugk

This comment has been minimized.

Copy link

rugk commented Jan 2, 2017

Well,… never build your own crypto!

mdmower added a commit to mdmower/twrp that referenced this issue Feb 27, 2017

Disable encrypted backups
Encrypted backups are poorly implemented (both by TWRP for padding a
password with zeros to achieve an AES key and openaes for using
non-standard CBC) and not to be trusted. Do not give users a false
sense of security by offering this option. Remove the ability to use
openaes to encrypt backups, but still allow already openaes-encrypted
backups to be restored.

Ref: TeamWin/Team-Win-Recovery-Project#817

Change-Id: Ibc1802372d8caa8c475039e66fb08ce163df27e9

mdmower added a commit to mdmower/twrp that referenced this issue Feb 27, 2017

Disable encrypted backups
Encrypted backups are poorly implemented (both by TWRP for padding a
password with zeros to achieve an AES key and openaes for using
non-standard CBC) and not to be trusted. Do not give users a false
sense of security by offering this option. Remove the ability to use
openaes to encrypt backups, but still allow already openaes-encrypted
backups to be restored.

Ref: TeamWin/Team-Win-Recovery-Project#817

Change-Id: Ibc1802372d8caa8c475039e66fb08ce163df27e9

mdmower added a commit to mdmower/twrp that referenced this issue Mar 9, 2017

Disable encrypted backups
Encrypted backups are poorly implemented (both by TWRP for padding a
password with zeros to achieve an AES key and openaes for using
non-standard CBC) and not to be trusted. Do not give users a false
sense of security by offering this option. Remove the ability to use
openaes to encrypt backups, but still allow already openaes-encrypted
backups to be restored.

Ref: TeamWin/Team-Win-Recovery-Project#817

Change-Id: Ibc1802372d8caa8c475039e66fb08ce163df27e9

mdmower added a commit to mdmower/twrp that referenced this issue Mar 10, 2017

Disable encrypted backups
Encrypted backups are poorly implemented (both by TWRP for padding a
password with zeros to achieve an AES key and openaes for using
non-standard CBC) and not to be trusted. Do not give users a false
sense of security by offering this option. Remove the ability to use
openaes to encrypt backups, but still allow already openaes-encrypted
backups to be restored.

Ref: TeamWin/Team-Win-Recovery-Project#817

Change-Id: Ibc1802372d8caa8c475039e66fb08ce163df27e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment