1141133 - Implement encrypt/decrypt of context information

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

We are going to encrypt and decrypt the context-in-conversation data client-side.

This bug is filed to track implementation of the encryption/decryption that can be used by the Loop code for the context-in-conversation work.

See https://wiki.mozilla.org/Loop/Architecture/Context#Information_Privacy for more information.

Comment 1

[Mark Banner (:standard8)]

I think this is meant to be Loop product, certainly not social. I think we want this on the content side of Loop (especially for the standalone).

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP - Implement encrypt/decrypt of context information ready for Loop's context in conversation work.

This is a wip patch based on Adam's PoC that he sent me. Adam, could you take a look at the crypto.js file and see if you see any issues with it?

Some questions & notes:

- What does "IV" stand for? The wiki page references it, but doesn't say what it is.
- Should we be saving the exportedKey.kty value - this seems constant at "oct", I'm just wondering if it might change in the future.
- I've set the Tag Length as 128 per the wiki page, not 64 as it was in your PoC

Comment 3

[Mark Banner (:standard8)]

(Note: if anyone applies the patch, it depends on the one in bug 1142950).

Comment 4

[Mark Banner (:standard8)]

Adam: the other thing I've just realised, it seems like we need save the generated IV alongside the context information to be able to decode it.

This would mean we need to store two lots of IV - one for the context, and one for the wrappedKey.

Am I right, or am I missing something with respect to IV?

Comment 5

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #2)
> Created attachment 8577309 [details] [diff] [review]
> WIP - Implement encrypt/decrypt of context information ready for Loop's
> context in conversation work.
> 
> This is a wip patch based on Adam's PoC that he sent me. Adam, could you
> take a look at the crypto.js file and see if you see any issues with it?
> 
> Some questions & notes:
> 
> - What does "IV" stand for? The wiki page references it, but doesn't say
> what it is.

It's the initialization vector for the encryption (cf. http://en.wikipedia.org/wiki/Initialization_vector). In simple terms, it's a random string of bits mixed in by the encryption algorithm to ensure that there is no commonality between messages that could be used to retrieve the key. What this means is that it is utterly critical that the same IV never be used more than once for a key. In practice, we achieve this by using a crypto-random array of bytes as our IV, and using a new one each time we encrypt.

> - Should we be saving the exportedKey.kty value - this seems constant at
> "oct", I'm just wondering if it might change in the future.

This will always be "oct" for symmetric keys, so storing it won't be necessary.

> - I've set the Tag Length as 128 per the wiki page, not 64 as it was in your
> PoC

Thanks; this is correct.

(In reply to Mark Banner (:standard8) from comment #4)
> Adam: the other thing I've just realised, it seems like we need save the
> generated IV alongside the context information to be able to decode it.
> 
> This would mean we need to store two lots of IV - one for the context, and
> one for the wrappedKey.
> 
> Am I right, or am I missing something with respect to IV?

This is correct. Both the context information and the wrapped key should be stored in the format "Base64(IV || ciphertext || tag)". Note that the webcrypto primitives automatically append the tag to the ciphertext for you, so you don't need to explicitly add it. JS crypto libraries don't all do this (and you'll notice that the one I used for the PoC in particular does not).

Comment 6

[Adam Roach [:abr]]

Comment on attachment 8577309 [details] [diff] [review]
WIP - Implement encrypt/decrypt of context information ready for Loop's context in conversation work.

Review of attachment 8577309 [details] [diff] [review]:
-----------------------------------------------------------------

Heading in the right direction.

Since future versions of the data stored on the server may use something other than AES-GCM, the publicly exposed methods need to take an algorithm name. In this initial version, you would simply compare the name against "AES-GCM" and complain if it doesn't match. We need to make sure that the calling code can distinguish between "I don't know how to decrypt this" and "I know how to decrypt this but this key doesn't work."

::: browser/components/loop/content/shared/js/crypto.js
@@ +29,5 @@
> +
> +  /**
> +   * Generates a random key using the Web Crypto libraries.
> +   */
> +  function generateKey() {

Check for wk !== null here and log a message if it is.

@@ +42,5 @@
> +        // Now extract the key in the JSON Web Key format
> +        return wk.exportKey(KEY_FORMAT, cryptoKey);
> +      }).then(function(exportedKey) {
> +        // Lastly resolve the promise with the new key.
> +        // XXX Is it safe not to return exported.kty as well?

Yep, for symmetric keys, this will always be "oct"

@@ +51,5 @@
> +    });
> +  }
> +
> +  /**
> +   * Encrypts an object using the specified key.

I'm not sure we want this to operate on objects -- the room keys will be wrapped and unwrapped using this function, and they're just going to be raw byte buffers rather than objects.

I would suggest reducing the core functionality to a base function (encryptBytes). If you want to maintain this API surface, you could add  a convenience function that handles object marshalling (encryptObject) and then calls encryptBytes.

The same split would naturally apply to decryption.

@@ +59,5 @@
> +   * @param {Object} context  The object to be encrypted.
> +   *
> +   * @return {Object} An object with two base64 encoded string values:
> +   *   - context  The encrypted context
> +   *   - iv       XXX ???

As we discussed, just prepend the IV to the context before base64 encoding. (This is a difference between the PoC script and the behavior documented on the wiki).

@@ +63,5 @@
> +   *   - iv       XXX ???
> +   */
> +  function encrypt(key, context) {
> +    var iv = new Uint8Array(12);
> +

Check for wk !== null here and log a message if it is.

@@ +67,5 @@
> +
> +    return new Promise(function(resolve, reject) {
> +      // First import the key to a format we can use.
> +      wk.importKey(KEY_FORMAT,
> +        {k: key, kty: "oct"},

Use KEY_TYPE here rather than "oct".

@@ +98,5 @@
> +
> +        // We use base-64 encoded strings.
> +        resolve({
> +          context: btoa(encryptedContext),
> +          iv: btoa(_uintArrayToString(iv))

btoa doesn't work here: it's designed to take unicode as input, not arbitrary binary strings; and it will choke on certain sequences, per spec: "The btoa() method must throw an InvalidCharacterError exception if the method's first argument contains any character whose code point is greater than U+00FF". You'll want to use a more general-purpose base64 encoding implementation. b64encode and b64decode from http://polycrypt.net/common/util.js work correctly.

@@ +113,5 @@
> +   * @param {String} key              The key to use for encryption. This should have
> +   *                                  been generated by generateKey.
> +   * @param {Object} encryptedContext An object with two base64 encoded string values:
> +   *   - context  The encrypted context
> +   *   - iv       XXX ???

A above, this will arrive as a single base64 string with IV preprended to the context.

@@ +117,5 @@
> +   *   - iv       XXX ???
> +   * @param {Object}  The decrypted object.
> +   */
> +
> +  function decrypt(key, encryptedContext) {

Check for wk !== null here; if it is null, either log a message, or dispatch to a separate, non-webcrypto-based decryption implementation.

@@ +121,5 @@
> +  function decrypt(key, encryptedContext) {
> +    return new Promise(function(resolve, reject) {
> +      // First import the key to a format we can use.
> +      wk.importKey(KEY_FORMAT,
> +        {k: key, kty: "oct"},

Use KEY_TYPE here rather than "oct"

@@ +131,5 @@
> +      ).then(function(cryptoKey) {
> +        // Now we've got the key, start the decryption.
> +
> +        // Convert the data back into byte arrays.
> +        var cipherText = _stringToUint8Array(atob(encryptedContext.context));

atob suffers the same shortcomings as btoa, and cannot be used here.

::: browser/components/loop/test/shared/crypto_test.js
@@ +47,5 @@
> +        {test: true})).to.eventually.be.a("object");
> +    });
> +  });
> +
> +  describe("#decypt", function() {

Please also add a test to check for decryption failure -- in particular, make sure that you can programmatically distinguish between "this key didn't work" and "something else went wrong."

Comment 7

[Mark Banner (:standard8)]

Attachment: Implement encrypt/decrypt of context information ready for Loop's context in conversation work.

I think this is now fully implemented. I've addressed Adam's comments and used the base64 examples from devmo. There's some tests included for good measure as well.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8580272 [details] [diff] [review]
Implement encrypt/decrypt of context information ready for Loop's context in conversation work.

Review of attachment 8580272 [details] [diff] [review]:
-----------------------------------------------------------------

I ended up writing quite a bit down, so my gut says 'f+'. I think that all these boxes should be easy to tick, so I do expect the next round to be final ;-)

Nice work, this is an important piece of boilerplate.

::: browser/components/loop/content/shared/js/crypto.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

missing "use strict"; ?

@@ +30,5 @@
> +
> +  /**
> +   * Generates a random key using the Web Crypto libraries.
> +   *
> +   * @return {String} A promise which is rejected on failure, or resolved

s/String/Promise/

@@ +31,5 @@
> +  /**
> +   * Generates a random key using the Web Crypto libraries.
> +   *
> +   * @return {String} A promise which is rejected on failure, or resolved
> +   *                   with a string that is in the JSON web key format.

nit: indentation.

'JSON web key', yet another variation. (see my comment below)

@@ +35,5 @@
> +   *                   with a string that is in the JSON web key format.
> +   */
> +  function generateKey() {
> +    return new Promise(function(resolve, reject) {
> +      if (!isSupported()) {

Please move this up. If Web Crypto is not supported, there's also a chance that the `Promise` global is not supported.

@@ +36,5 @@
> +   */
> +  function generateKey() {
> +    return new Promise(function(resolve, reject) {
> +      if (!isSupported()) {
> +        reject(new Error("Web Crypto is not supported"));

You forgot to return here. Remember, `reject` needn't throw!

@@ +39,5 @@
> +      if (!isSupported()) {
> +        reject(new Error("Web Crypto is not supported"));
> +      }
> +
> +      // First get a crypto key

nit: missing dot.

@@ +41,5 @@
> +      }
> +
> +      // First get a crypto key
> +      window.crypto.subtle.generateKey({name: ALGORITHM, length: KEY_LENGTH },
> +        // true = the key can be extracted from the CryptoKey object

nit: "`true` means that the key..." and missing dot.

@@ +43,5 @@
> +      // First get a crypto key
> +      window.crypto.subtle.generateKey({name: ALGORITHM, length: KEY_LENGTH },
> +        // true = the key can be extracted from the CryptoKey object
> +        true,
> +        // usages for the key

nit: "Usages for the key."

@@ +46,5 @@
> +        true,
> +        // usages for the key
> +        ["encrypt", "decrypt"]
> +      ).then(function(cryptoKey) {
> +        // Now extract the key in the JSON Web Key format

nit: missing dot.

Here it's 'JSON Web Key' and above you mention 'JSONWebKey' and 'JSON web key'. Perhaps it's good to provide a description above what a JSON Web Key is exactly.

@@ +62,5 @@
> +   * Encrypts an object using the specified key.
> +   *
> +   * @param {String} key      The key to use for encryption. This should have
> +   *                          been generated by generateKey.
> +   * @param {String} context  The string to be encrypted.

Perhaps it's good to keep this lib 'Context' (feature) agnostic and just call this `data` or something.

@@ +71,5 @@
> +  function encryptBytes(key, context) {
> +    var iv = new Uint8Array(INITIALIZATION_VECTOR_LENGTH);
> +
> +    return new Promise(function(resolve, reject) {
> +      if (!isSupported()) {

Same move here, with the added benefit that we won't need to 'allocate' a size 12 Uint8Array.

@@ +87,5 @@
> +      ).then(function(cryptoKey) {
> +        // Now we've got the cryptoKey, we can do the actual encryption.
> +
> +        // First get the data into the format we need.
> +        var clearBuffer = sharedUtils.strToUTF8Arr(context);

UTF8Arr? Isn't this a Uint8Array?
Why did you call this variable `clearBuffer`?

@@ +100,5 @@
> +            tagLength: ENCRYPT_TAG_LENGTH
> +          }, cryptoKey,
> +          clearBuffer);
> +      }).then(function(cipherText) {
> +        // Join the initialization vector and context for returning

nit: missing dot.

@@ +118,5 @@
> +   * Decrypts an object using the specified key.
> +   *
> +   * @param {String} key              The key to use for encryption. This should have
> +   *                                  been generated by generateKey.
> +   * @param {String} encryptedContext The encrypted context

s/Context/Data/

@@ +124,5 @@
> +   *                   with a string that is the decrypted context.
> +   */
> +  function decryptBytes(key, encryptedContext) {
> +    return new Promise(function(resolve, reject) {
> +      if (!isSupported()) {

Same move, please.

@@ +141,5 @@
> +        // Now we've got the key, start the decryption.
> +
> +        var splitContext = _splitIVandContext(encryptedContext);
> +
> +        // and decrypt.

nit: I think we don't really need this comment.

@@ +149,5 @@
> +          tagLength: ENCRYPT_TAG_LENGTH
> +        }, cryptoKey, splitContext.cipherText);
> +      }).then(function(plainText) {
> +        // Now we just turn it back into a string and then an object.
> +        resolve(sharedUtils.UTF8ArrToStr(new Uint8Array(plainText)));

Still puzzled by `UTF8Arr`. Why did you choose not to go with my suggestion of just calling it `btoa(Uint8Array)` and `atob(String)`?

@@ +163,5 @@
> +   * @param {Uint8Array} ivArray The array of initialization vector values.
> +   * @param {DataView} cipherTextDataView The cipherText in data view format.
> +   * @return {Uint8Array} A joined array of the IV and cipherText.
> +   */
> +  function _joinIVandContext(ivArray, cipherTextDataView) {

Please rename this function `_joinIVAndCipherText`. Now that I think about it, why not just make this two function that are more generic, like:

`mergeBuffers(buf1, buf2)`, `splitBuffers(buf)`? I'm not against s/Buffer/Array either.

@@ +184,5 @@
> +    return joinedContext;
> +  }
> +
> +  /**
> +   * Splits IV and cipher text arrays into a joined array.

What does 'into a joined array' mean? I don't really see that here...

@@ +186,5 @@
> +
> +  /**
> +   * Splits IV and cipher text arrays into a joined array.
> +   *
> +   * @param {Uint8Array} A joined array of the IV and cipherText.

Please update this to reflect reality.

::: browser/components/loop/content/shared/js/utils.js
@@ +231,5 @@
> +      }
> +    }
> +
> +    return result.substr(0, result.length - 2 + mod3) +
> +      (mod3 === 2 ? '' : mod3 === 1 ? '=' : '==');

s/'/"/

@@ +271,5 @@
> +   * Utility function to convert a string into a UTF8 array.
> +   *
> +   * Taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Base64_encoding_and_decoding
> +   *
> +   * @param {String} domString The string to convert.

domString?

@@ +296,5 @@
> +    // Transcription.
> +    for (var chrIndex = 0; index < arrayLength; chrIndex++) {
> +      chr = domString.charCodeAt(chrIndex);
> +      if (chr < 128) {
> +        /* one byte */

Please turn these into `// One byte.`

@@ +320,5 @@
> +        result[index++] = 128 + (chr >>> 18 & 63);
> +        result[index++] = 128 + (chr >>> 12 & 63);
> +        result[index++] = 128 + (chr >>> 6 & 63);
> +        result[index++] = 128 + (chr & 63);
> +      } else /* if (chr <= 0x7fffffff) */ {

`} else { // if (chr <= 0x7fffffff)

@@ +351,5 @@
> +    for (var index = 0; index < length; index++) {
> +      part = arrayBytes[index];
> +      result += String.fromCharCode(
> +        part > 251 && part < 254 && index + 5 < length ? // six bytes
> +          /* (part - 252 << 30) may be not so safe in ECMAScript! So...: */

Please turn these comments into inline ones.

@@ +403,5 @@
> +    locationData: locationData,
> +    base64Decode: base64Decode,
> +    base64Encode: base64Encode,
> +    strToUTF8Arr: strToUTF8Arr,
> +    UTF8ArrToStr: UTF8ArrToStr

I'd still like to see `btoa` and `atob` here...

::: browser/components/loop/test/shared/crypto_test.js
@@ +31,5 @@
> +      delete window.crypto;
> +
> +      expect(loop.crypto.isSupported()).eql(false);
> +
> +      window.crypto = oldCrypto;

Since you're doing this in `afterEach` too, I don't think you need this dance here.

@@ +72,5 @@
> +    it("should decypt an object via a specific key", function() {
> +      var key = "Wt2-bZKeHO2wnaq00ZM6Nw";
> +      var encryptedContext = "XvN9FDEm/GtE/5Bx5ezpn7JVDeZrtwOJy2CBjTGgJ4L33HhHOqEW+5k=";
> +
> +      return expect(loop.crypto.decryptBytes(key, encryptedContext)).to.eventually.eql(JSON.stringify({test: true}));

Just wanted to say: nice! :)

@@ +87,5 @@
> +  describe("Full cycle", function() {
> +    it("should be able to encrypt and decypt in a full cycle", function(done) {
> +      var context = JSON.stringify({
> +        contextObject: true,
> +        i18nString: "对话"

ITYM 'UTF8String`

Comment 9

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> @@ +163,5 @@
> > +   * @param {Uint8Array} ivArray The array of initialization vector values.
> > +   * @param {DataView} cipherTextDataView The cipherText in data view format.
> > +   * @return {Uint8Array} A joined array of the IV and cipherText.
> > +   */
> > +  function _joinIVandContext(ivArray, cipherTextDataView) {
> 
> Please rename this function `_joinIVAndCipherText`. Now that I think about
> it, why not just make this two function that are more generic, like:
> 
> `mergeBuffers(buf1, buf2)`, `splitBuffers(buf)`? I'm not against
> s/Buffer/Array either.

I wanted to split them out from the encrypt/decrypt functions and be specific about what they're doing.

We could make them more generic, but then I think we'd want at least one more intermediate function for the merge case as there's an amount of set-up that needs doing to get the data in the right format.

Comment 10

[Mark Banner (:standard8)]

Attachment: Implement encrypt/decrypt of context information ready for Loop's context in conversation work. v2

Updated patch for review comments.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8580653 [details] [diff] [review]
Implement encrypt/decrypt of context information ready for Loop's context in conversation work. v2

Review of attachment 8580653 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, all clear. Ship it!

::: browser/components/loop/content/shared/js/crypto.js
@@ +36,5 @@
> +   *
> +   * @param {Object}
> +   */
> +  function setRootObject(obj) {
> +    console.log("loop.shared.mixins: rootObject set to " + obj);

ITYM 'loop.crypto'

@@ +91,5 @@
> +   *                   with a string that is the encrypted context.
> +   */
> +  function encryptBytes(key, data) {
> +    if (!isSupported()) {
> +      throw(new Error("Web Crypto is not supported"));

`throw new Error(...)`

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/c0262092fed2

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/c0262092fed2