Fix aesgcm not accounting for padding

mozilla / rust-ece

Encrypted Content-Encoding for HTTP (RFC 8188) Rust implementation

Mozilla Public License 2.0

25 stars 11 forks source link

Fix aesgcm not accounting for padding #45

Closed Jordandev678 closed 4 years ago

Jordandev678 commented 4 years ago

If I'm correct Firefox Fenix uses application-services which uses this library for push notification decryption of aesgcm. I'm running into it not accounting for padding correctly. If I understand the spec correctly the first two bytes are the number of following padding bytes then the message. The current code only skips the first two bytes but doesn't then use that information to skip ahead and remove the padding.

This updates the unpad function in aesgcm to use those bytes to skip the padding and adds an accompanying unit test.

First piece of rust code so review carefully :)

jrconlin commented 4 years ago

As noted in my comment on https://github.com/mozilla-mobile/fenix/issues/14582, if this is an issue that only impacts aesgcm, I'm tempted to suggest that it not be fixed in order to further encourage folk to use the correct aes128gcm content encoding.

Jordandev678 commented 4 years ago

I totally get the point, but to add to the comment I just left in the fenix issue. Other providers aside for a moment on a personal note I just tried to set our system to aes128gsm and it's not supported. For various reasons I'm currently locked into the current library version. Updating is a work in progress that will be finalized before the new year but in the mean time I'm unfortunately stuck. If I can't get this fixed the outcome won't be us updating sooner it'll be us dropping Firefox from support and just rolling with chrome. I'd really prefer to not have to do that given the choice if for no other reason than FF is my own default browser 🙂

Jordandev678 commented 4 years ago

Close, just had to grab the iterator first. Update pushed.