Support client-side push encryption

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: markh, Assigned: markh)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
Currently Firefox desktop exposes support for decrypting incoming push messages. However, the pushbox ("improved send tab") project design calls for the device sending a tab to use webpush encryption on information about the tab before handing it off to a mozilla service, which will arrange for that payload to be delivered to the target device, which will then decrypt it.

IOW, PushCrypto.jsm should grow an |encrypt| method to match the existing |decrypt| method.
Assignee

Updated

a year ago
Blocks: 1442133
Assignee

Comment 1

a year ago
I'm about to put a WIP up that seems oh-so-close to working. It basically steals most of the implementation in webpush.js and does work OK with the encryption done by that module - but that's only aesgcm128.  aes128gcm doesn't quite work - I believe I'm confused about how the nonce and encryption key should be put together (and that's because I'm still confused about what keys should be used where)

Apart from making the encryption actually work, I'm looking for feedback on the shape of the API and implementation. I'm also unclear if we want to support aesgcm128 at all - I'm fairly sure we never want to actually encrypt using it, but it might make sense to keep it around for testing purposes while we still support decrypting that scheme. OTOH, the shape of the API would be cleaner without supporting it (eg, we could drop the need to do magic with base64-encoding headers etc)

I'll add additional annotations in mozreview.
Assignee

Comment 3

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review230156

::: dom/push/PushCrypto.jsm:679
(Diff revision 1)
> +      let headerKey = params.dh ? params.key : new Uint8Array([]);
> +      payloadHeader = encryptHelpers.writeHeader(params.salt, params.rs, headerKey);
> +    } else {
> +      // *sob* - not clear where base64 boundaries should be. Probably out of this module. See also testsAreHard :(
> +      payloadHeader = new Uint8Array(0);
> +      headers.encryption = "keyid=p256dh;salt=" +

The handling of headers smells a little to me, but we can probably drop that entire concept if we drop easgcm128

::: dom/push/PushCrypto.jsm:686
(Diff revision 1)
> +      headers.encryption_key = "keyid=p256dh;dh=" +
> +                               ChromeUtils.base64URLEncode(localPubKey, {pad: false});
> +    }
> +
> +    let ciphertext = concatArray([payloadHeader, ...ciphertextChunks]);
> +    if (testsAreHard) {

I believe this is something to do with the mochitests trying to use this module via SpecialPowers - I'm sure it's solvable, but this yak is low on the list of all yaks here.

::: dom/push/PushCrypto.jsm:711
(Diff revision 1)
> +  },
> +
> +  async deriveKeyAndNonce(params, localKey) {
> +    let secret, nonceInfo, keyInfo;
> +    if (params.version == "aes128gcm") { // latest.
> +      // XXX - this is almost certainly wrong.

I believe all my issues are here. FWIW, I've confirmed we get the same "shared secret" in both encryption and decryption.

::: dom/push/test/xpcshell/test_crypto_encrypt.js:214
(Diff revision 1)
> +  // XXX - this fails crypto.subtle.decrypt for the chunk.
> +  // markh believes encryptHelpers.deriveKeyAndNonce is wrong - encrypt and decrypt get different nonce values :(
> +  // (note that computeSharedSecret() *does* compute the same on input and output)
> +  // is at fault - but also can't work out how to trick ece into a comparable command-line
> +  // with static values (specifically, how to feed a private key into ece).
> +  let result = await PushCrypto.decrypt(recvPrivateKey, recvPublicKey, params.authSecret /*??*/, got.headers, got.ciphertext);

All tests above here work. This decrypt fails with "bad encryption"
Assignee: nobody → markh
Status: NEW → ASSIGNED

Comment 4

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review230326

Thanks for taking this on, Mark! I left some long comments. Sorry this is so convoluted and complicated.

::: dom/push/PushCrypto.jsm:133
(Diff revision 1)
>    let rs = (payload[16] << 24) | (payload[17] << 16) | (payload[18] << 8) | payload[19];
>    let keyIdLen = payload[20];
> -  if (keyIdLen != 65) {
> +  // XXX - ece allows no key here, so temporarily allow it here too, just as
> +  // a sanity check for our encrypt tests.
> +  // This should almost certainly be removed.
> +  if (keyIdLen != 65 && keyIdLen != 0) {

Hmm, this doesn't seem right. The `keyId` in aes128gcm is the sender's public key, which we need for decryption. (It matches the `dh` param in aesgcm128 and aesgcm; despite the similar name, it's not the same as the `keyid` from those encodings).

::: dom/push/PushCrypto.jsm:659
(Diff revision 1)
> +
> +  // Not sure what signature and return value we should return here for a
> +  // theoretically generic API
> +  async encrypt(buffer, params, testsAreHard=false) {
> +    params = encryptHelpers.fixupEncryptParams(params);
> +    // The ephemeral ECDH key for just this message (right?)

It is. There are several keys involved here:

* The ephemeral key pair (public and private), same as the sender key. The sender public key goes in the header block (or the `dh` param, in the older `aesgcm` and ancient `aesgcm128`). We can probably have a public `encrypt` function that generates a key pair via `crypto.subtle.generateKey(ECDH_KEY, true, ['deriveBits'])`, and a random Web Push salt...and an internal `encryptWithKeyAndSalt` that we can use for tests with known keys and salts.
* The receiver public key, that we get from the subscription, or via `getKey("p256dh")`. We derive an ECDH shared secret using the receiver public key and sender private key.
* We feed the shared secret into `HKDF-Expand(HKDF-Extract(salt, IKM), info, L)`, where the auth secret (`.getKey("auth")`) is the `salt`, and `IKM` is the ECDH shared secret. The `info` param is `"WebPush: info\0" + receiverPublicKey + senderPublicKey`. That gives us our Web Push IKM.
* Finally, we take the Web Push IKM and use it to produce two keys: the content encryption key (`gcmBits`) and the nonce. `gcmBits = HKDF-Expand(HKDF-Extract(webPushSalt, webPushIKM), "Content-Encoding: aes128gcm\0", 28)`. `nonce = HKDF-Expand(HKDF-Extract(webPushSalt, webPushIKM), "Content-Encoding: nonce\0", 24)`.

That's for aes128gcm, the newest. aesgcm derives `info` params *just* differently enough to be annoying, and aesgcm128 doesn't use an auth secret at all. As we chatted about on IRC, we can ignore old schemes for encryption, and support them only for decryption. aes128gcm is the standard now, so I don't expect (*fingers crossed, citation needed*) we'll be adding new algorithms any time soon. :-)

So the flow is going to be something like (untested copypasta, needs cleanup):

```
let rawReceiverPublicKey = new Uint8Array([...]); // The receiver's `p256dh` key, Base64-decoded.
let receiverAuthSecret = new Uint8Array([...]); // The `auth` secret, also Base64-decoded.

let senderKeyPair = await crypto.subtle.generateKey({ name: "ECDH", namedCurve: "P-256" }, true, ["deriveBits"]); // Or import with JWK, for tests.
let salt = crypto.getRandomValues(new Uint8Array(16)); // Or fixed, for tests.

let rawSenderPublicKey = await crypto.subtle.exportKey("raw", senderKeyPair.publicKey);
let receiverPublicKey = await crypto.subtle.importKey("raw", rawReceiverPublicKey, { name: "ECDH", namedCurve: "P-256" }, false, ["deriveBits"]);
let sharedSecret = await crypto.subtle.deriveBits({ name: "ECDH", public: receiverPublicKey }, senderKeyPair.privateKey, 256);
let authKdf = new hkdf(receiverAuthSecret, sharedSecret);
let authInfo = concatArray([new TextEncoder("utf-8").encode("WebPush: info\0"), rawReceiverPublicKey, rawSenderPublicKey]);
let prk = await authKdf.extract(authInfo, 32);
let prkKdf = new hkdf(salt, prk);
let [gcmBits, nonce] = await Promise.all([
  prkKdf.extract(new TextEncoder("utf-8").encode("Content-Encoding: aes128gcm\0"), 16),
  prkKdf.extract(new TextEncoder("utf-8").encode("Content-Encoding: nonce\0"), 12),
]);
let contentEncryptionKey = await crypto.subtle.importKey("raw", gcmBits, "AES-GCM", false, ["encrypt"]);
```

::: dom/push/PushCrypto.jsm:661
(Diff revision 1)
> +  // theoretically generic API
> +  async encrypt(buffer, params, testsAreHard=false) {
> +    params = encryptHelpers.fixupEncryptParams(params);
> +    // The ephemeral ECDH key for just this message (right?)
> +    // XXX - this must be wrong - the localPubKey isn't communicated
> +    // in aes128gcm. Is this the "sender key" for deriveKeyAndNonce?

It's communicated as the `keyId` in the header.

::: dom/push/PushCrypto.jsm:672
(Diff revision 1)
> +
> +    let headers = {encoding: params.version};
> +
> +    let payloadHeader;
> +    if (params.version == "aes128gcm") {
> +      // markh doesn't quite understand this - if there's no .dh, we don't

I'm guessing this is from the Node library? I think it does some curious things to juggle the different schemes, and TBH, I don't understand all the different params here, but we can be less fancy. (I think `keyid` and `key` are for aes128gcm without ECDH, which we don't need here, and `privateKey` is what we generate or import in tests for encryption).

::: dom/push/PushCrypto.jsm:679
(Diff revision 1)
> +      let headerKey = params.dh ? params.key : new Uint8Array([]);
> +      payloadHeader = encryptHelpers.writeHeader(params.salt, params.rs, headerKey);
> +    } else {
> +      // *sob* - not clear where base64 boundaries should be. Probably out of this module. See also testsAreHard :(
> +      payloadHeader = new Uint8Array(0);
> +      headers.encryption = "keyid=p256dh;salt=" +

Let's nix this entirely. aes128gcm doesn't use HTTP headers.

::: dom/push/PushCrypto.jsm:687
(Diff revision 1)
> +                               ChromeUtils.base64URLEncode(localPubKey, {pad: false});
> +    }
> +
> +    let ciphertext = concatArray([payloadHeader, ...ciphertextChunks]);
> +    if (testsAreHard) {
> +      return JSON.stringify({

Ouch. It's probably easier to start with adding tests to https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/push/test/xpcshell/test_crypto.js, and we can shave the mochitest yak later. https://github.com/web-push-libs/ecec/blob/7526bfed2035395038d6bcbf249affde65cfc76e/test/encrypt/aes128gcm.c (shameless plug) has some test vectors we can copy-paste and use here.

Sadly, I don't think Web Crypto can import `raw` ECDH private keys yet, for Reasons (though importing `raw` public keys works), but we can use the JWK trick, as in https://github.com/beverloo/webcrypto-push-encryption/blob/a2c13ee9a2a1c7fa55bf36363e28f81d0ab5fda7/src/keypair.js#L52-L62.

::: dom/push/PushCrypto.jsm:699
(Diff revision 1)
>  };
> +
> +let encryptHelpers = {
> +  async computeSharedSecret(headers) {
> +    let [appServerKey, subscriptionPrivateKey] = await Promise.all([
> +      crypto.subtle.importKey('raw', headers.dh, ECDH_KEY,

So this is the receiver public key (`p256dh`), which the other client gives us, and which we do need to import...

::: dom/push/PushCrypto.jsm:701
(Diff revision 1)
> +let encryptHelpers = {
> +  async computeSharedSecret(headers) {
> +    let [appServerKey, subscriptionPrivateKey] = await Promise.all([
> +      crypto.subtle.importKey('raw', headers.dh, ECDH_KEY,
> +                              false, ['deriveBits']),
> +      crypto.subtle.importKey('jwk', headers.privateKey, ECDH_KEY,

And this is our ephemeral private key. I don't think we need to import it, especially if we're generating our own. We will need to import in tests; this is the JWK trick I mentioned.

::: dom/push/PushCrypto.jsm:740
(Diff revision 1)
> +    let key = await crypto.subtle.importKey('raw', gcmBits, 'AES-GCM', false, ['encrypt']);
> +    let nonce = await kdf.extract(nonceInfo, 12);
> +    return [key, nonce];
> +  },
> +
> +  async aesgcm_encrypt(version, key, nonce, params, data) {

s/aesgcm/aes128gcm

::: dom/push/PushCrypto.jsm:761
(Diff revision 1)
> +        // latest
> +        let isLast = index == inChunks.length - 1;
> +        let padding = new Uint8Array([isLast ? 2 : 1]);
> +        input = [slice, padding];
> +      } else {
> +        let padding = new Uint8Array([0]);

Let's take out old padding.

::: dom/push/PushCrypto.jsm:809
(Diff revision 1)
> +    if (!["aes128gcm", "aesgcm128"].includes(header.version)) {
> +      throw new Error("Bad version");
> +    }
> +    header.rs = params.rs || 4096;
> +
> +    header.salt = params.salt; // should we support creating a salt?

We should. :-) Passing a fixed salt is only really useful for tests.
Assignee

Comment 5

a year ago
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #4)
> Comment on attachment 8955030 [details]
> Bug 1442128 - support push encryption in desktop - a very early wip that
> doesn't quite work
> 
> https://reviewboard.mozilla.org/r/224214/#review230326
> 
> Thanks for taking this on, Mark! I left some long comments. Sorry this is so
> convoluted and complicated.

Thanks heaps for this - it was a massive help.

> As we chatted about on IRC, we can ignore old schemes for
> encryption, and support them only for decryption. aes128gcm is the standard
> now, so I don't expect (*fingers crossed, citation needed*) we'll be adding
> new algorithms any time soon. :-)

Yep, thanks - that's what I've done.

> So the flow is going to be something like (untested copypasta, needs
> cleanup):

Heh - not bad for untested copypasta - you'll recognise alot of this in my next patch :) I didn't attempt to split it into helpers as, IMO, having it inline is a fair slab of code, but it's quite clear.

> Ouch. It's probably easier to start with adding tests to
> https://searchfox.org/mozilla-central/rev/
> 33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/push/test/xpcshell/test_crypto.
> js, and we can shave the mochitest yak later.

OK, I've reverted the mochitest changes, but did add a comment to webpush.

> https://github.com/web-push-libs/ecec/blob/
> 7526bfed2035395038d6bcbf249affde65cfc76e/test/encrypt/aes128gcm.c (shameless
> plug) has some test vectors we can copy-paste and use here.
> 
> Sadly, I don't think Web Crypto can import `raw` ECDH private keys yet, for
> Reasons (though importing `raw` public keys works), but we can use the JWK
> trick, as in
> https://github.com/beverloo/webcrypto-push-encryption/blob/
> a2c13ee9a2a1c7fa55bf36363e28f81d0ab5fda7/src/keypair.js#L52-L62.

Ack - I just realized I asked you about this earlier today - sorry for making you repeat yourself!

Re test fixtures, I added the one test from the standard doc, but didn't think it was necessary to support variable record sizes etc - hopefully that 1 fixture is OK.

Re testability, I support an optional "options" param that allows both the salt and the keys to be passed in, which isn't quite what you suggested, but it seems OK to me - but let me know if you'd like changes to the signature here, and also let me know what else you think needs to be done before requesting review.

Again, thanks heaps for the help.
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8955030 - Flags: feedback?(kit)
Assignee

Comment 7

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review233696

::: dom/push/PushCrypto.jsm:642
(Diff revision 2)
>    },
> +
> +  /**
> +   * Encrypts a payload suitable for using in a push message.
> +   *
> +   * @throws {CryptoError} if decryption fails.

s/decryption/encryption/

::: dom/push/test/xpcshell/test_crypto_encrypt.js:47
(Diff revision 2)
> +                        pK4Mqgkf1CXztLVBSt2Ks3oZwbuwXPXLWyouBWLVWGNWQexSgSxsj_Qulcy4a-fN`),
> +    plaintext: new TextEncoder("utf-8").encode("When I grow up, I want to be a watermelon"),
> +    authSecret: from64("BTBZMqHH6r4Tts7J_aSIgg"),
> +    receiver: {
> +      private: from64("q1dXpw3UpT5VOmu_cf_v6ih07Aems3njxI-JWgLcM94"),
> +      public: from64(`BCVxsr7N_eNgVRqvHtD0zTZsEc6-VV-JvLexhqUzORcx

these multi-line strings aren't indented consistently.

Comment 8

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review233718

LGTM, all I have are nits! I'll flag Martin for an extra pair of eyes, but I think this is ready to land once everything is fixed up. Thanks again, Mark!

::: dom/push/PushCrypto.jsm:640
(Diff revision 2)
>  
>      return decoder.decode();
>    },
> +
> +  /**
> +   * Encrypts a payload suitable for using in a push message.

Let's note that we only support a record size of 4096 and no padding.

::: dom/push/PushCrypto.jsm:652
(Diff revision 2)
> +   *  message recipient as a buffer.
> +   * @param {options} Object Encryption options, used for tests.
> +   * @returns {ciphertext, encoding} The encrypted payload and encoding.
> +   */
> +  // XXX - note that |plaintext| is the first param, which kinda makes sense
> +  // to markh, although I note that the ciphertext is the last param to

Yours makes more sense to me, too, I think we should use yours and change `decrypt` to match in a follow-up.

::: dom/push/PushCrypto.jsm:659
(Diff revision 2)
> +  async encrypt(plaintext, receiverPublicKey, receiverAuthSecret, options={}) {
> +    const encoding = options.encoding || AES128GCM_ENCODING;
> +    // We only support one encoding type.
> +    if (encoding != AES128GCM_ENCODING) {
> +      // XXX - not strictly a header, but this still seems the most appropriate
> +      // error to throw.

This is fine, or you could even use a vanilla `TypeError` if you want. `CryptoError` exists mostly for reporting more informative decryption errors to the console (https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/dom/push/PushService.jsm#828-834) for web devs, which we don't have to worry about here.

::: dom/push/PushCrypto.jsm:666
(Diff revision 2)
> +                            BAD_ENCODING_HEADER);
> +    }
> +    // We typically use an ephemeral key for this message, but for testing
> +    // purposes we allow it to be specified.
> +    const senderKeyPair = options.senderKeyPair ||
> +                          await crypto.subtle.generateKey(ECDH_KEY, true, ["deriveBits"]);

Async functions make all this so much nicer. ^_^

::: dom/push/PushCrypto.jsm:678
(Diff revision 2)
> +                                                                  ECDH_KEY, false, ["deriveBits"]);
> +
> +    const sharedSecret = await crypto.subtle.deriveBits({ name: "ECDH", public: receiverPublicCryptoKey },
> +                                                        senderKeyPair.privateKey, 256);
> +    const authKdf = new hkdf(receiverAuthSecret, sharedSecret);
> +    const authInfo = concatArray([new TextEncoder("utf-8").encode("WebPush: info\0"),

You could use `AES128GCM_AUTH_INFO` if you want here, or, if you think this is clearer, feel free to ignore.

::: dom/push/PushCrypto.jsm:684
(Diff revision 2)
> +                                 receiverPublicKey,
> +                                 rawSenderPublicKey]);
> +    const prk = await authKdf.extract(authInfo, 32);
> +    const prkKdf = new hkdf(salt, prk);
> +    const [gcmBits, nonce] = await Promise.all([
> +      prkKdf.extract(new TextEncoder("utf-8").encode("Content-Encoding: aes128gcm\0"), 16),

Ditto, `AES128GCM_KEY_INFO`.

::: dom/push/PushCrypto.jsm:685
(Diff revision 2)
> +                                 rawSenderPublicKey]);
> +    const prk = await authKdf.extract(authInfo, 32);
> +    const prkKdf = new hkdf(salt, prk);
> +    const [gcmBits, nonce] = await Promise.all([
> +      prkKdf.extract(new TextEncoder("utf-8").encode("Content-Encoding: aes128gcm\0"), 16),
> +      prkKdf.extract(new TextEncoder("utf-8").encode("Content-Encoding: nonce\0"), 12),

We get fancy when we decrypt and pass this as `concatArray([NONCE_INFO, new Uint8Array([0])])`.

How about adding a constant like `AES128GCM_NONCE_INFO` to match `AES128GCM_KEY_INFO`, with the trailing `\0`, and using it here and in https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/dom/push/PushCrypto.jsm#469?

::: dom/push/PushCrypto.jsm:691
(Diff revision 2)
> +    ]);
> +
> +    const contentEncryptionKey = await crypto.subtle.importKey("raw", gcmBits,
> +                                                               "AES-GCM", false,
> +                                                               ["encrypt"]);
> +    const helpers = aes128gcmEncryptHelpers; // one day we might support others

It seems a bit odd to me to have the key derivation inline in this function, but `aes128gcmEncryptHelpers` separately. How do you feel about moving the key derivation into a `deriveKeyAndNonce` helper, or going the other way and inlining everything?

::: dom/push/PushCrypto.jsm:704
(Diff revision 2)
>  };
> +
> +// Helpers for aes128gcm encryption - the only kind we support.
> +let aes128gcmEncryptHelpers = {
> +  // Perform the actual encryption.
> +  async encrypt(data, key, nonce, rs) {

Nit: Let's check that rs >= 18, just in case.

::: dom/push/PushCrypto.jsm:713
(Diff revision 2)
> +      chunks = [await crypto.subtle.encrypt({
> +        name: 'AES-GCM',
> +        iv: generateNonce(nonce, 0)
> +      }, key, new Uint8Array([2]))];
> +    } else {
> +    // Use specified recordsize, though we burn 1 for padding and 16 byte

Nit: Bump indentation one level here.

::: dom/push/PushCrypto.jsm:730
(Diff revision 2)
> +    }
> +    return chunks;
> +  },
> +
> +  // aes128gcm's header.
> +  createHeader(salt, rs, key) {

Ditto, let's assert that `key.length == 65`, even though we don't expect this to change. Let's also change the param name to `rawSenderPublicKey`.
Attachment #8955030 - Flags: review+
Attachment #8955030 - Flags: review?(martin.thomson)
Attachment #8955030 - Flags: feedback?(kit)
Comment hidden (mozreview-request)
Assignee

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review233718

> This is fine, or you could even use a vanilla `TypeError` if you want. `CryptoError` exists mostly for reporting more informative decryption errors to the console (https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/dom/push/PushService.jsm#828-834) for web devs, which we don't have to worry about here.

I ended up sticking with CryptoError for these errors, as TypeError seems wrong - JS needs a ValueError :)

Comment 11

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review233796

This looks pretty solid and well structured, though I have a few suggestions that I think will improve it a little.

It could probably use one or two more examples in tests.  One test vector isn't that good.  RFC 8188 has a few that you should be able to replicate.  That will mean adding `rs` to the options, which I don't see a problem with based on your code.  It's probably just a case of doing let rs = options.rs || 4096 and adding a check on the lower and upper bounds.

Testing some of the error cases would be good (esp. if you add the ability to change record sizes).

::: dom/push/PushCrypto.jsm:653
(Diff revision 3)
> +   * @param {receiverAuthSecret} Uint8Array The auth secret of the of the
> +   *  message recipient as a buffer.
> +   * @param {options} Object Encryption options, used for tests.
> +   * @returns {ciphertext, encoding} The encrypted payload and encoding.
> +   */
> +  async encrypt(plaintext, receiverPublicKey, receiverAuthSecret, options={}) {

Structurally, I think that this would be better  factored into a new class with an encrypt() method and all the helper functions attached to that class.

::: dom/push/PushCrypto.jsm:725
(Diff revision 3)
> +      }));
> +    }
> +    return chunks;
> +  },
> +
> +  async deriveKeyAndNonce(sharedSecret, salt, receiverAuthSecret,

This duplicates aes128gcmDecoder.deriveKeyAndNonce.

::: dom/push/PushCrypto.jsm:739
(Diff revision 3)
> +      prkKdf.extract(AES128GCM_KEY_INFO, 16),
> +      prkKdf.extract(AES128GCM_NONCE_INFO, 12),
> +    ]);
> +  },
> +
> +  async computeSharedSecret(receiverPublicKey, senderPrivateKey) {

This duplicates a good bit of Decoder.computeSharedSecret()

::: dom/push/test/xpcshell/test_crypto_encrypt.js:110
(Diff revision 3)
> +                    1024*100]) {
> +    info(`testing encryption of ${size} byte payload`);
> +    let message = new TextEncoder("utf-8").encode("x".repeat(size));
> +    let authSecret = crypto.getRandomValues(new Uint8Array(16));
> +    let {ciphertext, encoding} = await PushCrypto.encrypt(message, recvPublicKey, authSecret);
> +    // and decrypt it.

we should test that the encoding is correct
Attachment #8955030 - Flags: review?(martin.thomson) → review+
Assignee

Comment 12

a year ago
Thanks Martin,

(In reply to Martin Thomson [:mt:] from comment #11)

> It could probably use one or two more examples in tests.  One test vector
> isn't that good.  RFC 8188 has a few that you should be able to replicate. 
> That will mean adding `rs` to the options, which I don't see a problem with
> based on your code.  It's probably just a case of doing let rs = options.rs
> || 4096 and adding a check on the lower and upper bounds.

I can't see any suitable examples in rfc8188 - there are a couple of examples there, but IIUC, they both end up with a header that stores either 0 or 2 bytes for the key - but PushCrypto.jsm doesn't handle decrypting them (it insists on a 65byte key) - it doesn't seem worthwhile to me to support encrypting stuff we can't decrypt.

Also, unlike decryption (where you *do* want to support as much as possible as you don't have control over the encrypter), I don't think we need that much flexibility here - I can't really see why client code in Firefox would choose to use a different rs.

I also looked at both test_crypto.js and ecac, but none of the tests there seem to supply all the keys needed to encrypt - they only have enough for the decryption.

If you can point me at some reasonable examples I can use as fixtures and you feel strongly about this, I'm happy to add them.

> Structurally, I think that this would be better  factored into a new class
> with an encrypt() method and all the helper functions attached to that class.

Done.

> This duplicates aes128gcmDecoder.deriveKeyAndNonce.

Yeah, that's unfortunate, but I can't see a reasonable way to unify them in a way that doesn't add even more ugliness to the module, and it's only around 6 lines - so I just added a comment to that effect.

> > +  async computeSharedSecret(receiverPublicKey, senderPrivateKey) {
> 
> This duplicates a good bit of Decoder.computeSharedSecret()

This is only a couple of lines and the processing of the private key is different - so as above, I just commented this fact.

> we should test that the encoding is correct

Done.

I'll put up a new patch - if you don't mind, can you please have another quick look and let me know what you think I should do about other tests (and whether we might be able to do them in a followup)

Thanks!
Flags: needinfo?(martin.thomson)
Comment hidden (mozreview-request)
I think that you have the tests you concretely need, I was looking to get a little more diverse tests for implementation, but that isn't critical.

As you point out, we are very specific about RFC 8291 support, not generic RFC 8188 support, so the examples in 8188 aren't that much use.

However, I think that you might consider providing some varied values for the inputs to the new aes128gcmEncoder class.  If you support multiple records (which push doesn't), then varying rs and seeing that it can be decoded successfully would be good.
Flags: needinfo?(martin.thomson)

Comment 15

a year ago
mozreview-review
Comment on attachment 8955030 [details]
Bug 1442128 - support aes128gcm encryption in PushCrypto.jsm.

https://reviewboard.mozilla.org/r/224214/#review234586

Yeah, the relatively light set of tests is just something we will have to deal with.  Thanks for doing the refactor, it's much cleaner as a result.
Comment hidden (mozreview-request)

Comment 17

a year ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/8715148b05f8
support aes128gcm encryption in PushCrypto.jsm. r=kitcambridge,mt
https://hg.mozilla.org/mozilla-central/rev/8715148b05f8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.