433949 - Use WeaveCrypto component (NSS) instead of OpenSSL

Status: RESOLVED FIXED

(Cloud Services :: General, defect, P1)

Description

[Dan Mills [:thunder]]

This will require some sort of upgrade path (either a server wipe, or let the users decrypt using openssl and re-encrypt using nss).

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (work in progress)

Checkpointing some work. This adds code to WeaveCrypto for generating RSA keys, random AES keys, and wrapping thereof. Needs a bit of cleanup, but it's a functional component.

The next steps are to finish changes to the encrypt/decrypt functions, and make Weave use this code instead of OpenSSL.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

This should largely finish off the WeaveCrypto component work. There are a handful of XXX's still to look at, but I think it's ready for a review pass.

Comment 3

[Dan Mills [:thunder]]

Comment on attachment 323949 [details] [diff] [review]
Patch v.2


>+   * One of the above constants. Used as the mechanism for encrypting bulk
>+   * data and wrapping keys.
>+   *
>+   * Default is AES_128_CBC.

Why not AES_256_CBC? (speed?)

>   /**
>-   * Encrypt data using a passphrase
>-   * See algorithm attribute, it determines which mechanisms will be used
>+   * Encrypt data using a symmetric key.
>+   * The algorithm attribute specifies how the encryption is performed.
...
>+   * @param   symmetricKey
>+   *          A base64-encoded symmetric key (eg, one from generateRandomKey).

encrypt() and decrypt() now take base-64 encoded symmetric keys instead of passphrases.  Perhaps it is a documentation problem, but it's not clear to me from the IDL what methods I would have to call if my starting point is a) some data to encrypt, and b) a passphrase to encrypt with.  Do I just base-64 encode the passphrase?  If so, why don't just call that argument a passphrase? :)  Or at least add a note to explain that a bit better.

>+   * @param   iv
>+   *          A base64-encoded initialization vector

Is this argument required now?

>+  /**
>+   * Generate a RSA public/private keypair.
>+   *
>+   * @param aBits
>+   *        Strength of key pair. Eg, 1024, 2048.
>+   * @param aPassphrase
>+   *        User's passphrase. Used with PKCS#5 to generate a symmetric key
>+   *        for wrapping the private key.
>+   * @param aSalt
>+   *        Salt for the user's passphrase.
>+   * @param aIV
>+   *        Random IV, used when wrapping the private key.

Can salt and iv be optional?  They will be encoded in the resulting module, no?

>+   * @param aEncodedPublicKey
>+   *        The public key, base-64 encoded.
>+   * @param aWrappedPrivateKey
>+   *        The public key, encrypted with the user's passphrase, and base-64 encoded.
>+   */
>+  void generateKeypair(in unsigned long aBits,
>+                       in ACString aPassphrase, in ACString aSalt, in ACString aIV,
>+                       out ACString aEncodedPublicKey, out ACString aWrappedPrivateKey);
>+


>+  /**
>+   * Encrypts a symmetric key with a user's public key.
>+   *
>+   * @param aSymmetricKey
>+   *        The base64 encoded string holding a symmetric key.
>+   * @param aEncodedPublicKey
>+   *        The base64 encoded string holding a public key.
>+   * @returns The wrapped symmetric key, base64 encoded
>+   *
>+   * For RSA, the unencoded public key is a PKCS#1 object.
>+   */
>+  ACString wrapSymmetricKey(in ACString aSymmetricKey,
>+                            in ACString aEncodedPublicKey);

Is it an NSS limitation that we can only encrypt/decrypt keys (as opposed to data) using RSA?  I know RSA only works for small amounts of data, but Weave currently uses the OpenSSL commands, which just take bulk data for RSA operations.

>+
>+  /**
>+   * Decrypts a symmetric key with a user's private key.
>+   *
>+   * @param aWrappedSymmetricKey
>+   *        The base64 encoded string holding an encrypted symmetric key.
>+   * @param aWrappedPrivateKey
>+   *        The base64 encoded string holdering an encrypted private key.
>+   * @param aPassphrase
>+   *        The passphrase to decrypt the private key.
>+   * @param aSalt
>+   *        The salt for the passphrase.
>+   * @param aIV
>+   *        The random IV used when unwrapping the private key.

Do we need to keep track of salt and iv out of band now?

...

>+void
>+WeaveCrypto::EncodeBase64(const char *aData, PRUint32 aLength, nsACString& retval)
>+{
>+  PRUint32 encodedLength = (aLength + 2) / 3 * 4;
>+  char encoded[encodedLength];
>+
>+  PL_Base64Encode(aData, aLength, encoded);
>+
>+  retval.Assign(encoded, encodedLength);
>+}

This function and others which are now void--they can actually fail due to out of memory errors, no?

>+  // Stack buffer for the output. We make it a bit bigger than the input, as
>+  // the encrypted output may have a bit of padding added.
>+  char outputBuffer[aClearText.Length() + 100];

The amt of padding depends on the block size, right? So this might not actually be enough padding for some ciphers... though I guess 100 bytes is pretty big.

>+ * Generates an RSA key pair, encrypts (wrapps) the private key with the
>+ * supplied passphrasem, and returns both to the caller.

(typo)

>+/*
>+ * DeriveKeyFromPassphrase
>+ *
>+ * Creates a symmertic key from a passphrase, using PKCS#5.
>+ */
>+nsresult
>+WeaveCrypto::DeriveKeyFromPassphrase(const nsACString& aPassphrase,
>+                                     const nsACString& aSalt,
>+                                     PK11SymKey **aSymKey)

...oh.  Is this exposed via idl?

>+  var salt = cryptoSvc.generateRandomBytes(16);
>+  do_check_eq(salt.length, 24);
>+
>+  var iv = cryptoSvc.generateRandomBytes(16);
>+  do_check_eq(iv.length, 24);

I think we might want methods in the interface for getting a random salt and random iv.  Or, at least, we should have a doc blurb in the idl about how to call generateRandomBytes to get a salt and iv.

Overall: nice work, and I like the test suite.  I think once we clarify/fix the idl, this will be good to commit.

The C bits are mostly over my head, as I don't really know NSS.  But they seem good too, modulo my few comments.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to comment #3)

> >+   * Default is AES_128_CBC.
> 
> Why not AES_256_CBC? (speed?)

I don't think there's any practical benefit, but neither should there be any harm. Will change, and might as well make it 2048-bit RSA too.

> encrypt() and decrypt() now take base-64 encoded symmetric keys instead of
> passphrases.  Perhaps it is a documentation problem, but it's not clear to me
> from the IDL what methods I would have to call if my starting point is a) some
> data to encrypt, and b) a passphrase to encrypt with.

Weave shouldn't need to do that, though. The user's passphrase unwraps their private key, when then unwraps the random symmetric key that encrypted the bulk data.

It would be easy enough to expose DeriveKeyFromPassphrase(), if there's a use for it.

> >+   * @param   iv
> >+   *          A base64-encoded initialization vector
> 
> Is this argument required now?

Yes.

> Can salt and iv be optional?  They will be encoded in the resulting module, no?

No, but I suppose it depends on what you mean by optional. :) I was thinking it might be easier to make them outparams (ie, generate a random salt/iv along with the RSA keys). Or even inout, so the caller can either pass in a specific  salt/iv, or empty-string to have them generated.

> >+  ACString wrapSymmetricKey(in ACString aSymmetricKey,
> >+                            in ACString aEncodedPublicKey);
> 
> Is it an NSS limitation that we can only encrypt/decrypt keys (as opposed to
> data) using RSA?  I know RSA only works for small amounts of data, but Weave
> currently uses the OpenSSL commands, which just take bulk data for RSA
> operations.

I presume the OpenSSL stuff is generating some ephemeral symmetric key as part of the process. Having is as a separate step should be useful for the sharing stuff, where we'll want to call this with the public key of other users, so they can also decrypt the shared data.

> Do we need to keep track of salt and iv out of band now?

Yeah.


> >+void
> >+WeaveCrypto::EncodeBase64
> 
> This function and others which are now void--they can actually fail due to out
> of memory errors, no?

Well, the only two parts that can fail are the stack allocation (in which case the process would just crash), or is string.Assign() fails. But the latter doesn't seem to be checked in Mozilla code, so I assumed it just crashes too.

> >+  char outputBuffer[aClearText.Length() + 100];
> 
> The amt of padding depends on the block size, right? So this might not actually
> be enough padding for some ciphers... though I guess 100 bytes is pretty big.

Yeah. AES's block size is 16 bytes. There's surely a NSS function to get the exact block size for a cipher, but I was lazy here. The callee does check for sufficient buffer size, though.

> >+ * Generates an RSA key pair, encrypts (wrapps) the private key with the
> >+ * supplied passphrasem, and returns both to the caller.
> 
> (typo)

Crappm.


> >+  var salt = cryptoSvc.generateRandomBytes(16);
> >+  do_check_eq(salt.length, 24);
> >+
> >+  var iv = cryptoSvc.generateRandomBytes(16);
> >+  do_check_eq(iv.length, 24);
> 
> I think we might want methods in the interface for getting a random salt and
> random iv.  Or, at least, we should have a doc blurb in the idl about how to
> call generateRandomBytes to get a salt and iv.

Yeah, the end result here of using magic numbers isn't so slick. :/

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Checkpointing work... Address the previous review comments, and adds a first rough pass of modifying Weave's JS code to use the WeaveCrypto module. [That's "rough" as known to be incomplete/wrong/broken, but enough to get a feel for how things start to glue together.]

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 325234 [details] [diff] [review]
Patch v.3

Pushed just the binary module pieces (src/*) in c3a1707707da, after approval from thunder over IRC.

Upcoming patch to make Weave actually use it...

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.4 (work in progress)

This almost, but not quite, works. Checkpointing work here anyway.

From a clean server, everything gets set up and bookmarks sync, but none of the other engines are getting called. :-( The last log output is:

2008-06-18 23:23:02	Async.Generator	WARN	Async method 'Engine__sync-42' may have outstanding callbacks.

Will look into bright and early tomorrow.

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Ok, seems to work and is thus ready for review. :-) I'd like to clean up some of the identity stuff more, but that could possibly be done separately.

I'm getting some unit test failures, though, which I'm not sure I understand. Will look at those next.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Justin, your patch v5 seems to be all JavaScript code.  I don't see any
c/c++ code being changed.  May I correctly infer from this that PSM 
already contained all the scriptable classes/methods that you needed to 
accomplish your tasks?  (that would be gratifying to learn)

Comment 10

[Justin Dolske [:Dolske]]

Unfortunately there very much is a binary component. I landed it in back in comment 6, since Dan had already reviewed it.

It's over in http://hg.mozilla.org/labs/weave/index.cgi/file/, the WeaveCrypto stuff under src/, with unit tests in tests/unit/test_crypto_*.js... Comments welcome!

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 325912 [details] [diff] [review]
Patch v.5

Oh, iirc, at the last meeting we decided Anant could review this too.

Comment 12

[Anant Narayanan [:anant]]

Comment on attachment 325912 [details] [diff] [review]
Patch v.5

Patch looks good and seems to work fine on a quick test. One of the unit tests fails for me though: test_crypto_keypair
WARNING: PK11_GenerateKeyPair failed: file WeaveCrypto.cpp, line 373

Also, a server-wipe is almost always necessary since most of the JSON formats have changed. We need to figure out what the upgrade path for users would be and work that in at some point in the future.

>-    switch (algorithm) {
>-    case "none":
>+    if ("none" == this.defaultAlgorithm) {
>+      this._log.debug("NOT encrypting data");
>       ret = data;
>-      break;
>-
>-    case "aes-128-cbc":
>-    case "aes-192-cbc":
>-    case "aes-256-cbc":
>-    case "bf-cbc":
>-    case "des-ede3-cbc":
>-      ret = this._opensslPBE("-e", algorithm, data, identity.password);
>-      break;
>-
>-    default:
>-      throw "Unknown encryption algorithm: " + algorithm;
>+    } else {
>+      let symkey = identity.bulkKey;
>+      let iv     = identity.bulkIV;
>+      ret = this._crypto.encrypt(data, symkey, iv);
>     }
>-
>-    if (algorithm != "none")
>-      this._log.debug("Done encrypting data");
> 
>     self.done(ret);
>   },

Ok, so as I understand it, we're not checking the preferences for the type of encryption anymore (except for when it is "none") and going with the default: AES-256-CBC everytime. This is fine for now because that's the only type of encryption the component provides, but we may want to keep in mind future expansions where the user can select a particular encryption scheme.

> function Identity(realm, username, password) {
>-  this._realm = realm;
>-  this._username = username;
>+  this.realm     = realm;
>+  this.username  = username;
>   this._password = password;
> }
> Identity.prototype = {
>-  get realm() { return this._realm; },
>-  set realm(value) { this._realm = value; },
>+  realm   : null,
> 
>-  get username() { return this._username; },
>-  set username(value) { this._username = value; },
>+  // Only the "WeaveCryptoID" realm uses these:
>+  privkey        : null,
>+  pubkey         : null,
>+  passphraseSalt : null,
>+  privkeyWrapIV  : null,
> 
>+  // Only the per-engine identity uses these:
>+  bulkKey : null,
>+  bulkIV  : null,
>+
>+  username : null,
>   get userHash() { return Utils.sha1(this.username); },
>-
>-  _privkey: null,
>-  get privkey() { return this._privkey; },
>-  set privkey(value) { this._privkey = value; },
>-
>-  // FIXME: get this from the privkey using crypto.js?
>-  _pubkey: null,
>-  get pubkey() { return this._pubkey; },
>-  set pubkey(value) { this._pubkey = value; },

Any particular reason for removing the underscores from the object properties and removing the getters/setters? It doesn't really affect functionality one way or the other, but removing the underscores implies that you will be accessing these properties directly from elsewhere - is that the intention? If so, why not use getters?

Excellent test suite!

r+ from me, thunder should be giving a more comprehensive review.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to comment #12)
> (From update of attachment 325912 [details] [diff] [review])
> Patch looks good and seems to work fine on a quick test. One of the unit tests
> fails for me though: test_crypto_keypair
> WARNING: PK11_GenerateKeyPair failed: file WeaveCrypto.cpp, line 373

Was that on Linux? It works for me on OS X, but fails on my Linux box. I looked into it a bit, and I think it might be an NSS bug (Hi Nelson! :)... The C_GenerateKeyPair call down there was failing with CKR_USER_NOT_LOGGED_IN, shades of bug 400238. I'll look into this some more to see if it's just a problem with out test environment (xpcshell), or if it's a real problem.

> Also, a server-wipe is almost always necessary since most of the JSON formats
> have changed. We need to figure out what the upgrade path for users would be
> and work that in at some point in the future.

Right, I think that is the plan. Hmm, might be worth checking what happens if one tries to sync with the new code but old server-side data.

> Ok, so as I understand it, we're not checking the preferences for the type of
> encryption anymore (except for when it is "none") and going with the default:
> AES-256-CBC everytime.

Right, although I'm not sure it was actually working before. I parts of the pref checking around if we want to fix it up to make it all work, but I'm also thinking it might be simpler to just have Weave use a fixed algorithm/keysize. I don't think this is a knob end users really need to be able to twiddle.


> > function Identity(realm, username, password) {
> >-  this._realm = realm;
> >-  this._username = username;
> >+  this.realm     = realm;
> >+  this.username  = username;
...

> Any particular reason for removing the underscores from the object properties
> and removing the getters/setters?

Just trying to simplify things. Having getters/settings that just wrap a property seems like a needless complication.

Comment 14

[Anant Narayanan [:anant]]

Also, the patch doesn't seem to apply cleanly on trunk anymore...

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Updated to trunk.

A couple tweaks to the tests to fix the previously mentioned problems.

Comment 17

[Justin Dolske [:Dolske]]

Pushed, per Dan and Chris.

http://hg.mozilla.org/labs/weave/index.cgi/rev/83cde0da90fb