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().

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 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.

(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 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

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.