1345665 - Support new "aes128gcm" crypto scheme

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, enhancement)

Description

[Lina Butler [:lina]]

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".

Comment 1

[Lina Butler [:lina]]

Started working on this on the train. Will have a patch up soon.

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1345665 - Refactor push decryption into separate decoder classes.

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`.

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

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

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

Comment 4

[Martin Thomson [:mt:]]

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 5

[Martin Thomson [:mt:]]

Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

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

Comment 6

[Martin Thomson [:mt:]]

Kit, do we have a bug open for telemetry on usage of these three schemes?

Comment 7

[Martin Thomson [:mt:]]

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.

Comment 8

[Lina Butler [:lina]]

(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 9

[Lina Butler [:lina]]

Attachment: Bug 1345665 - Improve push crypto tests.

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

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8846089 [details]
Bug 1345665 - Refactor push decryption into separate decoder classes.

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

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

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

Comment 12

[Martin Thomson [:mt:]]

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.

Comment 13

[Martin Thomson [:mt:]]

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' };

Comment 14

[Martin Thomson [:mt:]]

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.

Comment 15

[Lina Butler [:lina]]

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 16

[Lina Butler [:lina]]

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. :-)

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8846089 [details]
Bug 1345665 - Refactor push decryption into separate decoder classes.

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

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8846090 [details]
Bug 1345665 - Implement the "aes128gcm" push decryption scheme.

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

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8866431 [details]
Bug 1345665 - Improve push crypto tests.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f43eac053667
https://hg.mozilla.org/mozilla-central/rev/7c9b42962705
https://hg.mozilla.org/mozilla-central/rev/0861e5c3f271