Skip to content
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

fix!: remove padding from decryptedBytes if left in by the cipher #647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrejborstnik
Copy link

@andrejborstnik andrejborstnik commented Aug 7, 2024

On (some?) Pixel devices (tested Pixel 6 and Pixel 7 on Android 14, AP2A.240705.004 build) for certain string lengths (e.g. 256) the encryption/decryption using CipherStorageKeystoreAesCbc doesn't erase the PKCS7 padding from the returned string.

To work around this issue I added a method that will detect & delete the padding if left in.

This technically a breaking change for any stored strings that end with a valid PKCS7 padding of characters up to with an ascii int value of IV.IV_LENGTH. Not sure how likely this use-case would be.

Opened this workaround as a discussion point, happy to discuss alternatives.

Also reported to Android https://issuetracker.google.com/issues/357896946, so they can look at the issue on their side

byte[] _decryptedBytes = cipher.doFinal(bytes, IV.IV_LENGTH, bytes.length - IV.IV_LENGTH);

// removing padding is required to work around a decryption padding issue with
// some Pixel devices e.g. for strings of length 256
Copy link
Author

@andrejborstnik andrejborstnik Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my testing up to 2000 bytes the problematic strings lengths are

256 -> 271
528 -> 543
800 -> 815
1280 -> 1295
1552 -> 1567
1824 -> 1839

@andrejborstnik
Copy link
Author

would this be the reason we are getting

I have only seen the issue in the OP on Pixel devices and E_CRYPTO_FAILED happens on other devices as well. So I would be sceptical this is the only reason. But I would not exclude this being the reason for at least some of the decryption issues, e.g. this padding one seems related on the surface

@DorianMazur
Copy link
Collaborator

@andrejborstnik @jahead Consider using AES GCM #678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants