Closed Bug 610749 Opened 10 years ago Closed 10 years ago

Add pure-JS PBKDF2 implementation

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch First swipe at patch and tests. (obsolete) — Splinter Review
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 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)
  },
This blocks bug 603489 as we need it on Firefox 3.5/3.6 in lieu of Svc.Crypto.derivePassphraseFromKey().
Blocks: 603489
Found the bugs. This one generates the same output.

If this is approved, I'll commit it and start the integration with New Crypto...
Assignee: nobody → rnewman
Attachment #489249 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #490892 - Flags: review?(philipp)
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.
Attachment #490892 - Flags: review?(philipp) → review-
(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 ;)
Attachment #490892 - Attachment is obsolete: true
Attachment #490923 - Flags: review?(philipp)
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
Attachment #490923 - Flags: review?(philipp) → review+
Oh, and perhaps explain what the arguments of pbkdf2Generate mean in the comment above it? Also, one letter variables and arguments FTL :/
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.