743070 - Implement storage format 6 crypto libraries for desktop, replacing WeaveCrypto

Status: NEW

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

For the big Sync rewrite and storage format 6 roll-out, we're planning on rewriting WeaveCrypto.

Goals:

* Targeted C++ APIs to reduce C++ <-> JS traversal
* Merge ciphertext, HMAC signature, and IV into 1 field and hide semantics from JS
* Allow Sync to run in FIPS mode (all key matter resides in C++/NSS and JS merely holds references to C++ objects)

When we're done, WeaveCrypto won't exist \o/

Comment 1

[Gregory Szorc [:gps]]

Attachment: Proposed IDL and JS interfaces, v1

Using notes from https://etherpad.mozilla.org/sync-storage-version-6 and the work-in-progress specification for storage format 6 (http://docs.services.mozilla.com/sync/storageformat6.html), I've put together an initial draft of the IDL and JS interfaces for the rewrite.

This is my first time writing IDL, so I probably made plenty of rookie mistakes.

Comment 2

[Gregory Szorc [:gps]]

Tracking all bugs that can be marked WONTFIX once WeaveCrypto is nuked from orbit.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 612741 [details] [diff] [review]
Proposed IDL and JS interfaces, v1

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

Seems pretty reasonable, and pretty much like what I expected. These are the interfaces that you want me to implement, right?

::: services/crypto/component/nsIKeyBundleContext.idl
@@ +9,5 @@
> + *
> + * Each key in the pair is 256 bit. The key bundle can also have arbitrary
> + * metadata associated with it. This metadata is defined as a string type, but
> + * it is treated as an opaque buffer and can be whatever the application wants
> + * it to be.

I suggest "One key is an AES-256 key, the other key is a HMAC-SHA256 key. The key bundle can also have arbitrary metadata associated with it..."

What is the motivation for the arbitrary metadata?

@@ +22,5 @@
> +   *
> +   * @param cleartext Raw data to encrypt and sign.
> +   * @param message   Raw byte representation of ciphertext, signature, and IV.
> +   */
> +  void encrypt(in ACString cleartext, out ACString message);

I suggest you use a different name than "encrypt" since the data is encrypted *and* signed.

I think you might also be able to write this:

     ACString encode(in ACString cleartext);

I don't know what the standard style is; up to you, I guess.

Why do we need encrypt()/decrypt() variants in addition to encryptAndBase64Encode()/decryptBase64Encoded variants? I would prefer to defer the non-base64-encoded variants if they are not (immediately) necessary so we can fully optimize the base64 variants without duplicating code. (Maybe it isn't an issue; I don't know yet).

@@ +104,5 @@
> +
> +  /**
> +   * Arbitrary metadata associated with key bundle.
> +   */
> +  attribute ACString metadata;

Are you intending this to be mutable?

@@ +112,5 @@
> +   *
> +   * This is not FIPS compliant and should only be used for testing and
> +   * debugging purposes.
> +   */
> +  attribute ACString encryptionKey;

readonly attribute ...

I really recommend that we don't do this, and instead we use logging inside the implementation of the encrypt/decrypt functions to dump the encryption keys, if we really need them.

::: services/crypto/component/nsIKeyBundleContextFactory.idl
@@ +6,5 @@
> +
> +interface nsIKeyBundleContext;
> +
> +[scriptable, uuid(8deb1073-7f68-11e1-b54d-c82a144a924f)]
> +interface nsIKeyBundleContextFactory : nsISupports

IMO it is best to have the BrowserID crypto component return a nsIKeyBundleContext that it derived from the Sync key using PBKDF2. This will avoid the FIPS mode issues that you would have with this function's createFromKeys method. If an addon or something implements a non-BrowserID way to create/derive nsIKeyBundleContexts, then the implementation of that component can figure out how to do the various key-fu. In that case, you don't need this interface at all in the browser itself. However, it might be useful as a *test-only* component, so we can write tests for the crypto stuff without depending on the BrowserID stuff.

::: services/crypto/modules/keybundle.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/. */

I do not understand the purpose of this file, since the implementation of nsIKeyBundleContext will be 100% in C++, as far as I understand the plan.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Brian Smith (:bsmith) from comment #3)
> @@ +22,5 @@
> > +   *
> > +   * @param cleartext Raw data to encrypt and sign.
> > +   * @param message   Raw byte representation of ciphertext, signature, and IV.
> > +   */
> > +  void encrypt(in ACString cleartext, out ACString message);
> 
> I think you might also be able to write this:
> 
>      ACString encode(in ACString cleartext);
> 
> I don't know what the standard style is; up to you, I guess.

If there's only one out parameter, definitely the form suggested by bsmith.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Brian Smith (:bsmith) from comment #3)
> I suggest "One key is an AES-256 key, the other key is a HMAC-SHA256 key.

Sounds good to me.

> The key bundle can also have arbitrary metadata associated with it..."
> 
> What is the motivation for the arbitrary metadata?

I believe it was a suggestion by one of {warner,dchan,benadida}. Basically, this would provide an "out" to store arbitrary metadata tied to the key matter sometime in the future without having to change the storage format. Ideas including recording the device that produced the key and the time it was produced. We have nothing specific in mind. But, it seemed like a nifty idea, so I'm running with it.

> @@ +22,5 @@
> > +   *
> > +   * @param cleartext Raw data to encrypt and sign.
> > +   * @param message   Raw byte representation of ciphertext, signature, and IV.
> > +   */
> > +  void encrypt(in ACString cleartext, out ACString message);
> 
> I suggest you use a different name than "encrypt" since the data is
> encrypted *and* signed.
> 
> I think you might also be able to write this:
> 
>      ACString encode(in ACString cleartext);
> 
> I don't know what the standard style is; up to you, I guess.

Good idea on the name. gavin also suggested returning values. I wasn't sure what the convention was since this is my first time writing IDL!

> Why do we need encrypt()/decrypt() variants in addition to
> encryptAndBase64Encode()/decryptBase64Encoded variants? I would prefer to
> defer the non-base64-encoded variants if they are not (immediately)
> necessary so we can fully optimize the base64 variants without duplicating
> code. (Maybe it isn't an issue; I don't know yet).

We don't technically need them. I thought it would round out the API nicely. I would think that the base64 variants could just call into the raw variants and code duplication would be minimal. Perhaps I'm being naive.

> @@ +104,5 @@
> > +
> > +  /**
> > +   * Arbitrary metadata associated with key bundle.
> > +   */
> > +  attribute ACString metadata;
> 
> Are you intending this to be mutable?

Yes. Alternatively, the constructors could take a metadata parameter and this would be readonly.

> 
> @@ +112,5 @@
> > +   *
> > +   * This is not FIPS compliant and should only be used for testing and
> > +   * debugging purposes.
> > +   */
> > +  attribute ACString encryptionKey;
> 
> I really recommend that we don't do this, and instead we use logging inside
> the implementation of the encrypt/decrypt functions to dump the encryption
> keys, if we really need them.

I threw it in to round out the API. I imagine it might be nice-to-have when debugging/troubleshooting crypto. It also might serve a real purpose if we ever had alternative key management facilities. Although, for those we might have dedicated APIs that returned specific representations of the entire key bundle. Hmm...
> 
> ::: services/crypto/component/nsIKeyBundleContextFactory.idl
> @@ +6,5 @@
> > +
> > +interface nsIKeyBundleContext;
> > +
> > +[scriptable, uuid(8deb1073-7f68-11e1-b54d-c82a144a924f)]
> > +interface nsIKeyBundleContextFactory : nsISupports
> 
> IMO it is best to have the BrowserID crypto component return a
> nsIKeyBundleContext that it derived from the Sync key using PBKDF2. This
> will avoid the FIPS mode issues that you would have with this function's
> createFromKeys method.

I agree. CC'ing ddahl so it appears on radar.

> If an addon or something implements a non-BrowserID
> way to create/derive nsIKeyBundleContexts, then the implementation of that
> component can figure out how to do the various key-fu.

I disagree. Generally, I'm in favor of offloading as much burden from extensions as possible. When you consider this is important crypto foo, I'm even more inclined to provide additional interfaces.

Also, Sync needs to produce pairs of randomly-generated key pairs. That necessitates the createFromRandom() API, right?

> However, it might be
> useful as a *test-only* component, so we can write tests for the crypto
> stuff without depending on the BrowserID stuff.

Most likely, yes.
 
> ::: services/crypto/modules/keybundle.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/. */
> 
> I do not understand the purpose of this file, since the implementation of
> nsIKeyBundleContext will be 100% in C++, as far as I understand the plan.

It's not technically required. I wrote it because I like abstracting the XPCOM voodoo away from the caller so people can consume pure, clean, and friendly JS. This abstraction also enables easier testing since all XPCOM usage goes through one proxy (hopefully). Unless there is a good reason why it should be nuked, I'd prefer to keep it (although I will remove it from subsequent reviews since it is irrelevant).

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Gregory Szorc [:gps] from comment #5)
> > If an addon or something implements a non-BrowserID
> > way to create/derive nsIKeyBundleContexts, then the implementation of that
> > component can figure out how to do the various key-fu.
> 
> I disagree. Generally, I'm in favor of offloading as much burden from
> extensions as possible. When you consider this is important crypto foo, I'm
> even more inclined to provide additional interfaces.

Well, for example, let's say that they have some new method for exchanging the sync key, other than the browser-ID-based mechanism. Presumably, they will want that to work in NSS's FIPS mode and/or otherwise use the key management system from whatever other crypto API they use. 

> Also, Sync needs to produce pairs of randomly-generated key pairs. That
> necessitates the createFromRandom() API, right?

Yes, the other method that takes the two keys is the one that is more problematic.

> It's not technically required. I wrote it because I like abstracting the
> XPCOM voodoo away from the caller so people can consume pure, clean, and
> friendly JS. This abstraction also enables easier testing since all XPCOM
> usage goes through one proxy (hopefully). Unless there is a good reason why
> it should be nuked, I'd prefer to keep it (although I will remove it from
> subsequent reviews since it is irrelevant).

OK.

Comment 7

[Gregory Szorc [:gps]]

(In reply to Brian Smith (:bsmith) from comment #6)
> Well, for example, let's say that they have some new method for exchanging
> the sync key, other than the browser-ID-based mechanism. Presumably, they
> will want that to work in NSS's FIPS mode and/or otherwise use the key
> management system from whatever other crypto API they use. 

Ideally it would work in FIPS mode, yes. However, I don't think we should be strict about FIPS mode. We're only talking about FIPS mode now because we realized it was effectively a low-hanging fruit.

I also believe in flexibility of APIs. If you build it, they will come. But, I'm not the one implementing things. If it is too much work to build, I'll remove it in consideration of your time.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Proposed IDL interfaces, v2

* Generally incorporated feedback
* Return things instead of using out arguments
* Got rid of raw variants
* Renamed to encode/decode
* Switch to AString (double byte strings) for JS interop optimization

I feel dirty having things like raw key matter be double wide from a C API. But, we are tailoring this for JS. Yes, I believe this means that half the bytes will be NULL and wasted. But, it saves us an allocation crossing XPCOM (if I understand things correctly). If I'm wrong, I'll happily revert to ACString to restore sanity to the C side of things.

Comment 10

[Richard Newman [:rnewman]]

Clarifying this to be the desktop version of Bug 798218.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 613880 [details] [diff] [review]
Proposed IDL interfaces, v2

Cancelling old, likely unneeded feedback.