Closed Bug 1257821 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: marco, Assigned: Lina)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch use_new_enc_info (obsolete) — Splinter Review
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?
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.
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?
(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 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/
(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 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 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.
Attachment #8732245 - Flags: review?(martin.thomson) → review+
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 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/
Attachment #8732245 - Attachment description: MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r?mt → MozReview Request: Bug 1257821 - Support the new `aesgcm` content encoding scheme. r=mt
https://reviewboard.mozilla.org/r/41055/#review37667

> Still having trouble?  Or is the latest OK?

The latest is OK. Thanks!
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.
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 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.
Attachment #8733668 - Flags: review?(martin.thomson) → review+
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.
Attachment #8733668 - Flags: review?(mcastelluccio)
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
Attachment #8733668 - Flags: review+
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
See Also: → 1257405
Keywords: leave-open
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 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/
Attachment #8733668 - Attachment description: MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r?mt,marco → MozReview Request: Bug 1257821 - Remove the authenticated `aesgcm128` content coding scheme. r=mt,marco
Attachment #8732245 - Attachment is obsolete: true
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.
Attachment #8733668 - Flags: approval-mozilla-beta?
Attachment #8733668 - Flags: approval-mozilla-aurora?
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+
Attachment #8733668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8733668 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8732245 - Attachment is obsolete: false
Comment on attachment 8732121 [details] [diff] [review]
use_new_enc_info

Superseded by attachment 8732245 [details].
Attachment #8732121 - Attachment is obsolete: true
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.
Attachment #8732245 - Flags: approval-mozilla-beta?
Attachment #8732245 - Flags: approval-mozilla-aurora?
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+
Attachment #8732245 - Flags: approval-mozilla-beta?
Attachment #8732245 - Flags: approval-mozilla-beta+
Attachment #8732245 - Flags: approval-mozilla-aurora?
Attachment #8732245 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
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?
Keywords: leave-open
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/
Rebased on m-i.
m-i is open again, so I went ahead and landed this. Thanks, everyone!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/906bb507cee3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.