1257821 - Content-Encoding should be 'aesgcm' for the new standard

Status: RESOLVED FIXED

(Core :: DOM: Push Notifications, defect)

Description

[Marco Castelluccio [:marco]]

http://hg.mozilla.org/mozilla-central/file/341344bdec8f/dom/push/PushCrypto.jsm#l249

This will require some complexity in the application servers, as they will need to use the old standard with 'Encryption-Key' for Firefox 45, 'Content-Encoding: aesgcm128' for Firefox 46-47 and 'Content-Encoding: aesgcm' for Firefox 48.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: use_new_enc_info

Something like this would work, but maybe we should support both 'Content-Encoding: aesgcm128' and 'Content-Encoding: aesgcm' for a while so application servers wouldn't need to do two different things for different versions of Firefox?

Comment 2

[Lina Butler [:lina]]

Almost! We can definitely support both, but we need to increase the padding for `aesgcm` to 2 bytes, per https://github.com/httpwg/http-extensions/pull/136.

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

Review commit: https://reviewboard.mozilla.org/r/41055/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41055/

Comment 4

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/41055/#review37667

::: dom/push/test/xpcshell/test_notification_data.js:206
(Diff revision 1)
>        },
>        receive: {
>          scope: 'https://example.com/page/2',
>          data: 'Some message'
>        }
>      },

I'm having trouble generating a test vector using https://github.com/martinthomson/encrypted-content-encoding. Even for aesgcm128 with `padSize: 1` and no auth secret, `_decodeChunk` rejects for the first chunk.

But is the general approach sensible?

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> https://reviewboard.mozilla.org/r/41055/#review37667
> 
> ::: dom/push/test/xpcshell/test_notification_data.js:206
> (Diff revision 1)
> >        },
> >        receive: {
> >          scope: 'https://example.com/page/2',
> >          data: 'Some message'
> >        }
> >      },
> 
> I'm having trouble generating a test vector using
> https://github.com/martinthomson/encrypted-content-encoding. Even for
> aesgcm128 with `padSize: 1` and no auth secret, `_decodeChunk` rejects for
> the first chunk.

You could use some code from https://github.com/marco-c/web-push/blob/master/test/testEncrypt.js#L12 for the old standard (padSize 1 and no auth secret) and from https://github.com/marco-c/web-push/blob/support_gcm_payload/test/testEncrypt.js#L12 for the new standard (padSize 2 and auth secret).

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41055/diff/1-2/

Comment 7

[Lina Butler [:lina]]

(In reply to Marco Castelluccio [:marco] from comment #5)
> You could use some code from
> https://github.com/marco-c/web-push/blob/master/test/testEncrypt.js#L12 for
> the old standard (padSize 1 and no auth secret) and from
> https://github.com/marco-c/web-push/blob/support_gcm_payload/test/
> testEncrypt.js#L12 for the new standard (padSize 2 and auth secret).

It would help if I passed in the correct param names. :-) Thanks, Marco!

I haven't changed this to remove the intermediate "aesgcm128 + auth secret" encoding yet. I'll do that in a follow-up commit and see if we can uplift both to Aurora and Beta.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41055/diff/2-3/

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

https://reviewboard.mozilla.org/r/41055/#review37809

Please require an authentication secret with the new encoding.

::: dom/push/PushCrypto.jsm:88
(Diff revision 3)
>    var rs = (enc.rs)? parseInt(enc.rs, 10) : 4096;
>  
>    if (!dh || !salt || isNaN(rs) || (rs <= 1)) {
>      return null;
>    }
> -  return {dh, salt, rs, auth: requiresAuthenticationSecret};
> +  return {dh, salt, rs, auth: requiresAuthenticationSecret, encoding};

Can you make sure that `aesgcm` always has an authentication secret here?  We tolerated no secret before because we had to, but we don't need to now.

::: dom/push/PushCrypto.jsm:287
(Diff revision 3)
> +    var encryptInfo = encoding == AESGCM_ENCODING ? AESGCM_ENCRYPT_INFO :
> +                                                    AESGCM_128_ENCRYPT_INFO;

Based on the above comment, you could move this under the `if (authenticationSecret) {` check.

Comment 10

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/41055/#review37667

> I'm having trouble generating a test vector using https://github.com/martinthomson/encrypted-content-encoding. Even for aesgcm128 with `padSize: 1` and no auth secret, `_decodeChunk` rejects for the first chunk.
> 
> But is the general approach sensible?

Still having trouble?  Or is the latest OK?

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41055/diff/3-4/

Comment 12

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Review commit: https://reviewboard.mozilla.org/r/41861/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41861/

Comment 13

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/41055/#review37667

> Still having trouble?  Or is the latest OK?

The latest is OK. Thanks!

Comment 14

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/41055/#review37809

> Can you make sure that `aesgcm` always has an authentication secret here?  We tolerated no secret before because we had to, but we don't need to now.

Added a check, and refactored a bit to make part 2 cleaner. Please take another look.

Comment 15

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/41055/#review38367

This is better, yes.

::: dom/push/PushCrypto.jsm:19
(Diff revisions 3 - 4)
>  var AESGCM_128_ENCODING = 'aesgcm128';
>  var AESGCM_128_ENCRYPT_INFO = UTF8.encode('Content-Encoding: aesgcm128');

Maybe `AESGCM128_EN*`

::: dom/push/PushCrypto.jsm:70
(Diff revisions 3 - 4)
> -  if (!keymap) {
> +  if (!keymap && encoding == AESGCM_128_ENCODING) {
> +    // Only allow the older `aesgcm128` scheme to omit the secret.

The presence of Crypto-Key (as opposed to Encryption-Key) was previously being used to indicate that the authentication secret was present.  Now that you have a better signal (the encoding), use that.

You should be able to switch on the encoding and set the variables straight up:

keymap = null;
if (encoding == 'aesgcm') {
  requiresAuthSecret = true;
  keymap = get('Crypto-Key');
} else if (encoding == 'aegcm128') {
  requiresAuthSecret = false;
  keymap = get('Encryption-Key');
}
if (!keymap) {
  return null;
}

::: dom/push/PushCrypto.jsm:282
(Diff revisions 3 - 4)
> +      encryptInfo = padSize == 2 ? AESGCM_ENCRYPT_INFO :
> +                                   AESGCM_128_ENCRYPT_INFO;

Based on your next change, this will always be "aesgcm".

::: dom/push/PushCrypto.jsm:329
(Diff revisions 3 - 4)
> -        throw new Error('Padding size must be 1 byte');
> -      }
> +    }
> -      padSize = 1;
> -      pad = decoded[0];
> +    if (decoded.length < padSize) {
> +      throw new Error('Decoded array is too short!');
>      }
> +    var pad = padSize == 2 ? (decoded[0] << 8) | decoded[1] : decoded[0];

pad = decoded[0];
if (padSize === 2) {
  pad = (pad << 8) | decoded[1];
}

Makes operator precedence clearer.  It's not often that you have to know that | goes before ?:

::: dom/push/PushService.jsm:814
(Diff revisions 3 - 4)
>            record.p256dhPrivateKey,
>            record.p256dhPublicKey,
>            cryptoParams.dh,
>            cryptoParams.salt,
>            cryptoParams.rs,
> -          cryptoParams.auth ? record.authenticationSecret : null
> +          cryptoParams.auth ? record.authenticationSecret : null,

You can remove the ?: operator here if you use padSize as the marker.

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

https://reviewboard.mozilla.org/r/41861/#review38369

Do we actually have many tests of bad encodings?  Missing Crypto-Key header fields, wrong header fields, bad values, that sort of thing?  It looks like our coverage of some of this is light.

This r+ is conditional on getting approval to uplift.

::: dom/push/PushCrypto.jsm:69
(Diff revision 1)
>      return null;
>    }
>  
> -  var requiresAuthenticationSecret = true;
>    var keymap = getEncryptionKeyParams(headers.crypto_key);
>    if (!keymap && encoding == AESGCM_128_ENCODING) {

See other review comment, s/requiresAuthSecret/padSize/

::: dom/push/PushCrypto.jsm:252
(Diff revision 1)
>    _deriveKeyAndNonce(padSize, ikm, salt, receiverKey, senderKey,
>                       authenticationSecret) {
>      var kdfPromise;
>      var context;
>      var encryptInfo;
> -    // The authenticationSecret, when present, is mixed with the ikm using HKDF.
> +    // We use the pad size to infer the encoding scheme.

The size of the padding determines which key derivation we use.

Also, cite the spec we use for pad size 1, because that is going to disappear: https://tools.ietf.org/html/draft-thomson-http-encryption-02

::: dom/push/PushService.jsm:814
(Diff revision 1)
>            record.p256dhPrivateKey,
>            record.p256dhPublicKey,
>            cryptoParams.dh,
>            cryptoParams.salt,
>            cryptoParams.rs,
> -          cryptoParams.auth ? record.authenticationSecret : null,
> +          record.authenticationSecret,

Ahh, I see that you did that here.

Comment 17

[Marco Castelluccio [:marco]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

https://reviewboard.mozilla.org/r/41861/#review38489

Looks good to me, it would be great if we can uplift to Firefox 46 and 47.

It would be useful to add other tests, for example containing also the VAPID key in Crypto-Key.

Comment 18

[Marco Castelluccio [:marco]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

https://reviewboard.mozilla.org/r/41861/#review38491

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9d1b43cf76

Comment 20

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/41861/#review38369

As always, thank you for the reviews! I added some tests for header parsing and bad crypto in part 1.

> See other review comment, s/requiresAuthSecret/padSize/

I kept `requiresAuthSecret` in part 1, because I think we still need it to distinguish between authenticated and unauthenticated aesgcm128. But I restructured it as you suggested to keep the delta smaller.

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41861/diff/1-2/

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Approval Request Comment
[Feature/regressing bug #]: Bug 1257405, Bug 1257821.
[User impact if declined]: Chrome 50 is shipping a newer implementation of Push message crypto [http://blog.chromium.org/2016/03/chrome-50-beta-push-notification.html]. Developers will need to use workarounds to send encrypted messages to Firefox 46 and 47, causing web compat pain.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d4363629bc
[Risks and why]: Low to medium risk. Tests cover valid and invalid encryption, but this is a coordinated change: we'll need to either uplift all three patches (the two attached to this bug, and the one for bug 1257405), or none of them.
[String/UUID change made/needed]: None.

Firefox currently supports three encryption schemes: aesgcm128 without a secret, aesgcm128 with a 12-byte secret (an intermediate state), and aesgcm with a 16-byte secret. I'm requesting uplift approval for attachment 8733668 [details] before landing on m-i because it removes the intermediate state.

We can only do this if uplift is granted for all three patches (https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9d1b43cf76, attachment 8733668 [details], and attachment 8732017 [details] [diff] [review]).

If not, we shouldn't land attachment 8733668 [details] on m-i.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e9d1b43cf76

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Fix needed for Push notification encrypted message webcompat, Aurora47+

Comment 25

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b97e56d0ff2

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Webcompat fix, please uplift for beta along with the patches from bug 1257405

Comment 27

[Lina Butler [:lina]]

Comment on attachment 8732121 [details] [diff] [review]
use_new_enc_info

Superseded by attachment 8732245 [details].

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

I requested Aurora and Beta approval for this patch verbally in comment 22 (it landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9d1b43cf76), but didn't set the flags because I didn't realize Bugzilla attachments could be un-obsoleted.

(MozReview marked it as obsolete because I pushed the second patch via `hg push -c`).

Formally requesting approval now, using the same rationale as comment 22.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8732245 [details]
MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt

This patch also needed to be uplifted by it was not nominated earlier, Aurora47+, Beta46+

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Tomcat, Wes: We also need to land this patch to fix the bug, Kit missed attaching it in the first go. I have not reverted the 46, 47 status flags, not sure if I should have.

Comment 31

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97badc14b304
https://hg.mozilla.org/releases/mozilla-beta/rev/a35a5a4d072a

Comment 32

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11a460c667e4

Comment 33

[Lina Butler [:lina]]

Thanks, Ritu! Wes, now that we've uplifted to Aurora and Beta, we can land attachment 8733668 [details] on m-i. Is that OK with you?

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8733668 [details]
MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41861/diff/2-3/

Comment 35

[Lina Butler [:lina]]

Rebased on m-i.

Comment 36

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5193dfb284d

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/906bb507cee3

Comment 38

[Lina Butler [:lina]]

m-i is open again, so I went ahead and landed this. Thanks, everyone!

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/906bb507cee3

Comment 40

[Marco Castelluccio [:marco]]

Should we add this and bug 1257405 to https://developer.mozilla.org/en-US/Firefox/Releases/46?