Closed Bug 1345665 Opened 4 years ago Closed 4 years ago

Support new "aes128gcm" crypto scheme

Categories

(Core :: DOM: Push Notifications, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: lina, Assigned: lina)

Details

Attachments

(3 files)

The headers, padding, and HKDF info strings are different (https://github.com/kitcambridge/ecec#aes128gcm-1), but the rest should be mostly the same. We can also take this opportunity to fix bug 1230038 and remove the ancient "aesgcm128" scheme.

https://github.com/mozilla-services/autopush/issues/774 tracks the server work to accept "aes128gcm".
Started working on this on the train. Will have a patch up soon.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Comment on attachment 8846089 [details]
Bug 1345665 - Refactor push decryption into separate decoder classes.

https://reviewboard.mozilla.org/r/119164/#review121326

This looks pretty good.  I think that you could probably iterate on this once more though.  I have a small restructuring suggestion and I think that you could improve test coverage of the new stuff a fair bit.

::: dom/push/PushCrypto.jsm:115
(Diff revision 1)
>  
> +// Extracts the sender public key, salt, and record size from the payload for the
> +// "aes128gcm" scheme.
> +function getCryptoParamsFromPayload(payload) {
> +  let rs = (payload[16] << 24) | (payload[17] << 16) | (payload[18] << 8) | payload[19];
> +  let keyIdLen = payload[20];

Check that this is equal to 65.

::: dom/push/PushCrypto.jsm:127
(Diff revision 1)
> +}
> +
> +// Extracts the sender public key, salt, and record size from the `Crypto-Key`,
> +// `Encryption-Key`, and `Encryption` headers for the "aesgcm" and "aesgcm128"
> +// schemes.
>  function getCryptoParams(headers) {

getCryptoParamsFromHeaders

::: dom/push/PushCrypto.jsm:296
(Diff revision 1)
> +  async decode() {
> +    if (this.ciphertext.byteLength === 0) {
> +      // Zero length messages will be passed as null.
> +      return null;
> +    }
> +    if (this.isTruncated) {

You can move the isTruncated check into the "Old" class.  This check isn't valid for aes128gcm.

::: dom/push/PushCrypto.jsm:362
(Diff revision 1)
> +   *
> +   * @param {Uint8Array} decoded The decrypted, padded chunk.
> +   * @returns {Uint8Array} The chunk with padding removed.
> +   */
> +  unpadChunk(decoded) {
> +    if (decoded.length < this.padSize) {

Becaause padSize is only used in the two older forms, I suggest that you do this instead:

```
Base -> Old -> aesgcm
            -> aesgcm128
     -> aes128gcm
```

Leave unpadChunk and chunkSize are abstract in the base.  Old can define this method and have an abstract padSize function.

::: dom/push/PushCrypto.jsm:395
(Diff revision 1)
> +   * Indicates whether the pad size exceeds the record size, or if the
> +   * ciphertext size is a multiple of the record size. For "aesgcm" and
> +   * "aesgcm128", this means the ciphertext is invalid.
> +   */
> +  get isTruncated() {
> +    return this.rs <= this.padSize ||

If you do as I suggest, then you should either add a minimum size getter, or you can just rely on checking that this.chunkSize > 0 (to ensure that you don't have a zero-length split) and having unpadChunk do a more detailed check.

::: dom/push/PushCrypto.jsm:435
(Diff revision 1)
> +  unpadChunk(decoded, last) {
> +    if (!decoded.length) {
> +      throw new CryptoError('Zero plaintext', BAD_PADDING);
> +    }
> +    let length = decoded.length;
> +    while (length--) {

ever heard about the "down-to" operator?

```
while (length --> 0) {
}
```

::: dom/push/PushCrypto.jsm:601
(Diff revision 1)
> +                                     cryptoParams.ciphertext);
> +    } else if (encoding == AESGCM128_ENCODING || encoding == AESGCM_ENCODING) {
> +      // "aesgcm" and "aesgcm128" include the salt, record size, and sender
> +      // public key in the `Crypto-Key` and `Encryption` HTTP headers.
> +      let cryptoParams = getCryptoParams(headers);
> +      let senderKey = base64URLDecode(cryptoParams.dh);

Move this base64URLDecode call to the `getCryptoParams[FromHeaders]()` function.

::: dom/push/test/xpcshell/test_crypto.js:244
(Diff revision 1)
>        test.desc
>      );
>    }
>  });
> +
> +add_task(async function test_aes128gcm() {

I think that you could probably run a few more tests than what you have here.

You can probably steal the test vectors from https://github.com/martinthomson/encrypted-content-encoding/blob/master/python/http_ece/tests/test_ece.py#L214 (for example).

::: dom/push/test/xpcshell/test_crypto.js:262
(Diff revision 1)
> +    },
> +    publicKey: 'BCVxsr7N_eNgVRqvHtD0zTZsEc6-VV-JvLexhqUzORcxaOzi6-AYWXvTBHm4bjyPjs7Vd8pZGH6SRpkNtoIAiw4',
> +  }, {
> +    desc: 'rs = 24',
> +    result: "I am the very model of a modern Major-General; I've information vegetable, animal, and mineral",
> +    data: 'goagSH7PP0ZGwUsgShmdkwAAABhBBDJVyIuUJbOSVMeWHP8VNPnxY-dZSw86doqOkEzZZZY1ALBWVXTVf0rUDH3oi68I9Hrp-01zA-mr8XKWl5kcH8cX0KiV2PtCwdkEyaQ73YF5fsDxgoWDiaTA3wPqMvuLDqGsZWHnE9Psnfoy7UMEqKlh2a1nE7ZOXiXcOBHLNj260jYzSJnEPV2eXixSXfyWpaSJHAwfj4wVdAAocmViIg6ywk8wFB1hgJpnX2UVEU_qIOcaP6AOIOr1UUQPfosQqC2MEHe5u9gHXF5pi-E267LAlkoYefq01KV_xK_vjbxpw8GAYfSjQEm0L8FG-CN37c8pnQ2Yf61MkihaXac9OctfNeWq_22cN6hn4qsOq0F7QoWIiZqWhB1vS9cJ3KUlyPQvKI9cvevDxw0fJHWeTFzhuwT9BjdILjjb2Vkqc0-qTDOawqD4c8WXsvdGDQCec5Y1x3UhdQXdjR_mhXypxFM37OZTvKJBr1vPCpRXl-bI6iOd7KScgtMM1x5luKhGzZyz25HyuFyj1ec82A',

I can confirm that this decrypts successfully with my library.
Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

https://reviewboard.mozilla.org/r/119168/#review121328
Attachment #8846090 - Flags: review?(martin.thomson)
Kit, do we have a bug open for telemetry on usage of these three schemes?
Comment on attachment 8846089 [details]
Bug 1345665 - Refactor push decryption into separate decoder classes.

https://reviewboard.mozilla.org/r/119166/#review124184

::: commit-message-58753:7
(Diff revision 1)
> +
> +This patch moves the decryption methods into a base `Decoder` class,
> +with subclasses for the "aesgcm" and "aesgcm128" schemes. The base
> +class provides a `decrypt` method that computes the ECDH shared secret,
> +derives the key and nonce, decrypts each record, and removes padding.
> +Subclasses implement `deriveKeyAndNonce` and `padSize`.

Wow, we get to review commit messages, that's awesome.
Attachment #8846089 - Flags: review?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #6)
> Kit, do we have a bug open for telemetry on usage of these three schemes?

We don't. I think it would be easier to collect this on the server than in telemetry. Filed https://github.com/mozilla-services/autopush/issues/875.
Comment on attachment 8846089 [details]
Bug 1345665 - Refactor push decryption into separate decoder classes.

https://reviewboard.mozilla.org/r/119166/#review141322

Much better than before, thanks.
Attachment #8846089 - Flags: review?(martin.thomson) → review+
Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

https://reviewboard.mozilla.org/r/119168/#review141326

::: dom/push/PushCrypto.jsm:476
(Diff revision 2)
> +    if (!decoded.length) {
> +      throw new CryptoError('Zero plaintext', BAD_PADDING);
> +    }

This is redundant with the throw at the end.  I don't think we need to worry that decoded.length is NaN or something other than 0 that might be falsy.

::: dom/push/PushCrypto.jsm:621
(Diff revision 2)
> +                                     cryptoParams.senderKey, cryptoParams.salt,
> +                                     cryptoParams.rs, authenticationSecret,

Should you pass cryptoParams as an argument rather than unbundling it here?

::: dom/push/test/xpcshell/test_crypto.js:281
(Diff revision 2)
> +    let publicKey = ChromeUtils.base64URLDecode(test.publicKey, {
> +      padding: 'reject',
> +    });

Maybe assign a variable and use it:

const rejectPadding = { padding: 'reject' };
Attachment #8846090 - Flags: review?(martin.thomson) → review+
Comment on attachment 8866431 [details]
Bug 1345665 - Improve push crypto tests.

https://reviewboard.mozilla.org/r/138052/#review141332

Nothing here that you can't trivially fix.


Thanks for putting the extra tests together.  I didn't independently validate the tests, but I trust that you got these right.  If you think that would be a good idea to check that the tests do indeed contain the errors you intended, you might have to give me some more time (I have travel soon, so it might be a while).

::: dom/push/test/xpcshell/test_crypto.js:240
(Diff revision 1)
> +      x: '_dBBKKhcBYltf4H-EYvcuIe588H_QYOtxMgk0ShgcwA',
> +      y: '6Yay37WmEOWvQ-QIoAcwWE-T49_d_ERzfV8I-y1viRY',
> +      ext: true,
> +    },
> +    publicKey: 'BP3QQSioXAWJbX-B_hGL3LiHufPB_0GDrcTIJNEoYHMA6Yay37WmEOWvQ-QIoAcwWE-T49_d_ERzfV8I-y1viRY',
> +    authSecret: 'NVo4zW2b7xWZDi0zCNvWAA',

It's hard to see that this is truly shorter just by eyeballing it.  Why not make it really short.  Or maybe:

```
'astringoftherightlength'.slice(0, 21 /* needed: 22 */)
```

Is the example valid otherwise?  It's hard to tell when the values are all different.  You could reuse a previous example.

::: dom/push/test/xpcshell/test_crypto.js:256
(Diff revision 1)
> +    let authSecret = ChromeUtils.base64URLDecode(test.authSecret, {
> +      padding: "reject",
> +    });

See comment on previous patch.  Alternatively, you could wrap ChromeUtils.base64URLDecode(), you use it enough to justify that.

::: dom/push/test/xpcshell/test_crypto.js:318
(Diff revision 1)
> +    privateKey: {
> +      crv: 'P-256',
> +      d: '4h23G_KkXC9TvBSK2v0Q7ImpS2YAuRd8hQyN0rFAwBg',
> +      ext: true,
> +      key_ops: ['deriveBits'],
> +      kty: 'EC',
> +      x: 'sd85ZCbEG6dEkGMCmDyGBIt454Qy-Yo-1xhbaT2Jlk4',
> +      y: 'vr3cKpQ-Sp1kpZ9HipNjUCwSA55yy0uM8N9byE8dmLs',
> +    },

If you use the same key more than once, can you extract it somewhere.

::: dom/push/test/xpcshell/test_crypto.js:468
(Diff revision 1)
> -      crypto_key: 'dh=BCHn-I-J3dfPRLJBlNZ3xFoAqaBLZ6qdhpaz9W7Q00JW1oD-hTxyEECn6KYJNK8AxKUyIDwn6Icx_PYWJiEYjQ0',
> -      encryption: 'salt=c6JQl9eJ0VvwrUVCQDxY7Q; rs=25',
> +      crypto_key: 'dh=BBicj01QI0ryiFzAaty9VpW_crgq9XbU1bOCtEZI9UNE6tuOgp4lyN_UN0N905ECnLWK5v_sCPUIxnQgOuCseSo',
> +      encryption: 'salt=SbkGHONbQBBsBcj9dLyIUw; rs=6',
> +      encoding: 'aesgcm',
> +    },
> +  }, {
> +    desc: 'rs = 6, auth tag for last record',

Not sure that I understand the comment.  Can you try to explain better?

::: dom/push/test/xpcshell/test_crypto.js:524
(Diff revision 1)
> +    let privateKey = test.privateKey;
>      let publicKey = ChromeUtils.base64URLDecode(test.publicKey, {
>        padding: 'reject',
>      });
> -    let authSecret = ChromeUtils.base64URLDecode(test.authSecret, {
> -      padding: 'reject',
> -    });
>      let payload = ChromeUtils.base64URLDecode(test.data, {
>        padding: 'reject',
>      });
> -    let result = await PushCrypto.decrypt(test.privateKey, publicKey,
> -                                          authSecret,
> +    let result = await PushCrypto.decrypt(privateKey, publicKey, null,
> +                                          test.headers, payload);

It looks like you are duplicating this part of the code quite a bit now.  Time to extract.
Attachment #8866431 - Flags: review?(martin.thomson) → review+
Comment on attachment 8866431 [details]
Bug 1345665 - Improve push crypto tests.

https://reviewboard.mozilla.org/r/138052/#review141332

Thanks for the reviews, Martin. I generated the vectors using your implementation, changing some of the functions to emit intentionally bad data. I also used these to test ecec, which gives slightly more informative errors. More checks are always worthwhile, but I think they're testing what they intend.

> It's hard to see that this is truly shorter just by eyeballing it.  Why not make it really short.  Or maybe:
> 
> ```
> 'astringoftherightlength'.slice(0, 21 /* needed: 22 */)
> ```
> 
> Is the example valid otherwise?  It's hard to tell when the values are all different.  You could reuse a previous example.

No. This used the first 12 bytes of the secret to encrypt, and the full 16 to decrypt, so the CEK shouldn't match at all. I clarified this description, and the others.

> Not sure that I understand the comment.  Can you try to explain better?

Done. The first record is complete (22 bytes total, including the tag); the second doesn't have any data, just the 16-byte tag.
Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

https://reviewboard.mozilla.org/r/119168/#review141326

> Should you pass cryptoParams as an argument rather than unbundling it here?

Yes. :-)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f43eac053667
Refactor push decryption into separate decoder classes. r=mt
https://hg.mozilla.org/integration/autoland/rev/7c9b42962705
Implement the "aes128gcm" push decryption scheme. r=mt
https://hg.mozilla.org/integration/autoland/rev/0861e5c3f271
Improve push crypto tests. r=mt
https://hg.mozilla.org/mozilla-central/rev/f43eac053667
https://hg.mozilla.org/mozilla-central/rev/7c9b42962705
https://hg.mozilla.org/mozilla-central/rev/0861e5c3f271
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.