610749 - Add pure-JS PBKDF2 implementation

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Attachment: First swipe at patch and tests.

First attempt (using NSS HMAC) doesn't pass tests, whereas its origin does.

That implies an error in my integration.

Here's a patch.

Comment 1

[Philipp von Weitershausen [:philikon]]

Comment on attachment 489249 [details] [diff] [review]
First swipe at patch and tests.

>+  // We can't use Utils.sha1HMAC() for this: it decodes its input as UTF-8.
>+  // TODO: this looks really inefficient when called a few thousand times...
>+  // Accepts an nsIKeyObject as input.
>+  _sha1HMAC: function _sha1HMAC(data, keyStr) {
>+    let bytes = [byte.charCodeAt() for each (byte in data)];
>+    let hasher = Cc["@mozilla.org/security/hmac;1"]
>+                   .createInstance(Ci.nsICryptoHMAC);
>+    hasher.init(hasher.SHA1, this._makeHMACKey(keyStr));
>+    hasher.update(bytes, bytes.length);
>+    return hasher.finish(false);
>+  },

We might need to UTF-8 encode after all... Try:

  _sha1HMAC: function _sha1HMAC(data, keyStr) {
    let hasher = Cc["@mozilla.org/security/hmac;1"]
                   .createInstance(Ci.nsICryptoHMAC);
    hasher.init(hasher.SHA1, this._makeHMACKey(keyStr))
    return Utils.digest(data, hasher)
  },

Comment 2

[Philipp von Weitershausen [:philikon]]

This blocks bug 603489 as we need it on Firefox 3.5/3.6 in lieu of Svc.Crypto.derivePassphraseFromKey().

Comment 3

[Richard Newman [:rnewman]]

Attachment: Updated patch. Passes same tests as NSS version.

Found the bugs. This one generates the same output.

If this is approved, I'll commit it and start the integration with New Crypto...

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 490892 [details] [diff] [review]
Updated patch. Passes same tests as NSS version.

Looks good! Only changes I have are about how to add this code to the codebase. Requires a rather different looking patch, though, so r- for now.

>--- /dev/null
>+++ b/services/sync/modules/pbkdf2.js

In the spirit of reducing the amount of files we have (bug 609421), let's add this as a function to Utils (util.js).

>@@ -0,0 +1,154 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please always use the boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ for new files, retaining the format. Initial Developer should always be "Mozilla Foundation". Don't forget to add yourself to contributors. :)

>+/*
>+ * Chopped up.
>+ */

?

>+  // Accepts an nsIKeyObject as input.
>+  // Pass in a hasher for a little extra efficiency.
>+  _sha1HMAC: function _sha1HMAC(key, data, h) {
>+    let hasher = h || Cc["@mozilla.org/security/hmac;1"].createInstance(Ci.nsICryptoHMAC);
>+    hasher.init(hasher.SHA1, key);
>+    
>+    // No UTF-8 encoding for you, sunshine.
>+    let bytes = [b.charCodeAt() for each (b in data)];
>+    hasher.update(bytes, bytes.length);
>+    return hasher.finish(false);
>+  },
>+  
>+  _sha1HMACString: function _sha1HMACString(keyStr, data, h) {
>+    return this._sha1HMAC(this._makeHMACKey(keyStr), data, h);
>+  },

Let's expose this as Utils.sha1HMACBytes or similar. That way we can test it independently.

>+  _makeHMACKey: function _makeHMACKey(str) {
>+    return Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, str);
>+  },

This used in several places in the Sync code, right? So maybe expose it in Utils as well?

>+  base64 : function base64(octets) {
>+    return btoa(octets);
>+  },
>+  
>+  base32 : function base32(octets) {
>+    return Utils.encodeBase32(octets);
>+  }

I think we can get rid of those and just use btoa and Utils.encodeBase32 inline. As far as I can tell you only use those in the test anyway.

>diff --git a/services/sync/tests/unit/test_js_pbkdf2.js b/services/sync/tests/unit/test_js_pbkdf2.js
>new file mode 100644
>index 0000000..f5919e8
>--- /dev/null
>+++ b/services/sync/tests/unit/test_js_pbkdf2.js

As said above, pbkdf2 should be part of Utils so this should be test_utils_pbkdf2.js or similar.

>@@ -0,0 +1,97 @@
>+Cu.import("resource://services-sync/pbkdf2.js");
>+Cu.import("resource://services-sync/util.js");
>+
>+  function _stringToHex(str) {

Use Utils.bytesAsHex(str)?

>+function test_sha1_hmac() {

As said above, let's expose the sha1-hmac routine as a public function in Utils, so let's move this to test_utils_sha1hmac.js or similar.

Comment 5

[Richard Newman [:rnewman]]

(In reply to comment #4)

> Please always use the boilerplate from
> http://www.mozilla.org/MPL/boilerplate-1.1/ for new files, retaining the
> format. Initial Developer should always be "Mozilla Foundation". Don't forget
> to add yourself to contributors. :)

Will do.

> >+/*
> >+ * Chopped up.
> >+ */
> 
> ?

... from weave-chromium. I'll strip it out :)
 
> Let's expose this as Utils.sha1HMACBytes or similar. That way we can test it
> independently.

Sure.
 
> >+  _makeHMACKey: function _makeHMACKey(str) {
> >+    return Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, str);
> >+  },
> 
> This used in several places in the Sync code, right? So maybe expose it in
> Utils as well?

Makes sense.
 

> Use Utils.bytesAsHex(str)?

Didn't spot that. Utils too big ;)

Comment 6

[Richard Newman [:rnewman]]

Attachment: Post-review patch.

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 490923 [details] [diff] [review]
Post-review patch.

Just more cleaning up after the somewhat icky code layout of the original code...

>+  /**
>+   * PBKDF2 implementation in Javascript.
>+   */
>+  /* For HMAC-SHA-1 */
>+  _hLen : 20,

Specific to pbkdf2Generate, so let's make that a const in pbkdf2Generate.

>+  _arrayToString : function _arrayToString(arr) {
...
>+  _XOR : function _XOR(a, b, isA) {
...
>+  _F : function _F(PK, S, c, i, h) {

Same here, move them into pbkdf2Generate so that it's self-contained.

  pbkdf2Generate : function pbkdf2Generate(P, S, c, dkLen) {
    const HLEN = 20;
    function arrayToString(arr) {...}
    function XOR(a, b, isA) {...}
    function F(PK, S, c, i, h) {...}
    ...
  },

Just beware of changing 'this' objects inside functions, so you want to access sha1HMACBytes as Utils.sha1HMACBytes rather than this.sha1HMACBytes.

>+function run_test() {
>+  test_sha1_hmac();
>+}

Nit: just rename test_sha1_hmac to run_test?

r=me with those fixes

Comment 8

[Philipp von Weitershausen [:philikon]]

Oh, and perhaps explain what the arguments of pbkdf2Generate mean in the comment above it? Also, one letter variables and arguments FTL :/

Comment 9

[Richard Newman [:rnewman]]

Committed (in two parts, thanks to lack of competence on my part):

http://hg.mozilla.org/services/fx-sync/rev/d5cf19b3470c
http://hg.mozilla.org/services/fx-sync/rev/12189166cd01

85 passing tests on my machine.

Thanks, Philipp, for the review.