601645 - JavaScript API for NSS J-PAKE

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Needs to be compatible with what OpenSSL does (for Fx Home)

Comment 1

[Philipp von Weitershausen [:philikon]]

So we need big int arithmetic for this. This can be done (relatively) easily by using js-ctypes and NSS (freebl3 actually), though that means the method is restricted to Firefox 4. Are we ok with not doing this on Firefox 3.5/3.6?

Comment 2

[Mike Connor [:mconnor]]

I think so, given the timeline for getting this up and running and the looming end of 3.x support.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: Prereq 1 (v1): Move some general purpose utilities to crypto

Move some general purpose utilities to resource://services-crypto/util.js

This allows us to use them without having to import the world with resource://services-sync/util.js. This is completely backwards compatible to resource://services-sync/util.js

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: Prereq 2 (v1): Implement Utils.sha256()

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Implement J-PAKE

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for J-PAKE implementation

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: Implement J-PAKE (v2)

Fix random failures/crashes caused by GC issues (thanks to dwitte for spotting the problem).

Sadly we won't be able to use this implementation at all because the relevant functions aren't exposed by the shared libraries. It works on OSX pretty much by accident.

Comment 8

[Philipp von Weitershausen [:philikon]]

Had a chat with bsmith earlier, jotting down some of his concerns:

* Right now we use SHA1 in the zero-knowledge proofs which apparently isn't optimal. We might want to provide at least the option to use SHA256 here, even if OpenSSL doesn't support it right now. Then again, OpenSSL compatibility might not be such a big issue as we could always fork their jpake.c for Fx Home.

* Currently we use SHA256 to turn the key resulting from the J-PAKE mechanism into something with uniformly distributed randomness. It has been suggested to use an HMAC as the key extraction mechanism instead. See bug 609079.

* As of now we use the Diffie-Hellmann group with the smallest modulus (1024 bit) which roughly corresponds to an 80 bit key. We should probably use the 3072 bit one if we're looking to get a 256 bit AES key in the end. This will have some  perf impact, particularly on mobile, but we don't have to worry so much about blocking since this setup UI would be modal anyway. In the worst case we'll have to use the ChromeWorker approach here, too.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Moved to beta8 due to a hard dependency on this being available at the same time Fennec 2 Beta 3 ships.

Comment 10

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

When changing from SHA-1 to SHA-256 as explained above, switch from using FreeBL's SHA*_* functions directly to using nsICryptoHash. Also, use nsICryptoHMAC instead of directly using FreeBL's HMAC functions or PKCS#11 HMAC functions.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to comment #8)
> * Currently we use SHA256 to turn the key resulting from the J-PAKE mechanism
> into something with uniformly distributed randomness. It has been suggested to
> use an HMAC as the key extraction mechanism instead. See bug 609079.

We will use HMAC-SHA256() with a fixed 256 bit key.

> * As of now we use the Diffie-Hellmann group with the smallest modulus (1024
> bit) which roughly corresponds to an 80 bit key. We should probably use the
> 3072 bit one if we're looking to get a 256 bit AES key in the end. This will
> have some  perf impact, particularly on mobile, but we don't have to worry so
> much about blocking since this setup UI would be modal anyway. In the worst
> case we'll have to use the ChromeWorker approach here, too.

The 3072 bit modulus group is available and just has to be used by the consumer of the J-PAKE API (in our case bug 602876).

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 481839 [details] [diff] [review]
Prereq 1 (v1): Move some general purpose utilities to crypto

># HG changeset patch
># Parent bcff2c38b53eb70cbf8a175ca1f87bd727eb5c67
># User Philipp von Weitershausen <[e-mail]>
>Bug 601645 - Move some general purpose utilities to resource://services-crypto/util.js
>
>This allows us to use them without having to import the world with resource://services-sync/util.js.
>This is completely backwards compatible to resource://services-sync/util.js
>
>diff --git a/Makefile b/Makefile
>--- a/Makefile
>+++ b/Makefile
>@@ -172,16 +172,17 @@
> 	$(SLINK) $(TOPSRCDIR)/ui/firefox/AboutWeaveTabs.js $(stage_dir)/components/AboutWeaveTabs.js
> 	
> 	$(MKDIR) $(stage_dir)/chrome/locale/en-US/services
> 	$(SLINK) $(TOPSRCDIR)/services/sync/locales/en-US/errors.properties $(stage_dir)/chrome/locale/en-US/services/errors.properties
> 	$(SLINK) $(TOPSRCDIR)/services/sync/locales/en-US/sync.properties $(stage_dir)/chrome/locale/en-US/services/sync.properties
> 	
> 	$(MKDIR) $(stage_dir)/crypto-modules
> 	$(SLINK) $(TOPSRCDIR)/services/crypto/modules/WeaveCrypto.js $(stage_dir)/crypto-modules/WeaveCrypto.js
>+	$(SLINK) $(TOPSRCDIR)/services/crypto/modules/util.js $(stage_dir)/crypto-modules/util.js
> 	$(MKDIR) $(stage_dir)/modules
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/auth.js $(stage_dir)/modules/auth.js
> 	$(MKDIR) $(stage_dir)/modules/base_records
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/collection.js $(stage_dir)/modules/base_records/collection.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/crypto.js $(stage_dir)/modules/base_records/crypto.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/keys.js $(stage_dir)/modules/base_records/keys.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/wbo.js $(stage_dir)/modules/base_records/wbo.js
> 	$(SUBST) $(TOPSRCDIR)/services/sync/modules/constants.js > $(stage_dir)/modules/constants.js
>diff --git a/services/crypto/modules/util.js b/services/crypto/modules/util.js
>new file mode 100644
>--- /dev/null
>+++ b/services/crypto/modules/util.js
>@@ -0,0 +1,183 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Firefox Sync.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Dan Mills <[e-mail]>
>+ *  Philipp von Weitershausen <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+const EXPORTED_SYMBOLS = ['Utils'];
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+let Utils = {
>+
>+  deepEquals: function eq(a, b) {
>+    // If they're triple equals, then it must be equals!
>+    if (a === b)
>+      return true;
>+
>+    // If they weren't equal, they must be objects to be different
>+    if (typeof a != "object" || typeof b != "object")
>+      return false;
>+
>+    // But null objects won't have properties to compare
>+    if (a === null || b === null)
>+      return false;
>+
>+    // Make sure all of a's keys have a matching value in b
>+    for (let k in a)
>+      if (!eq(a[k], b[k]))
>+        return false;
>+
>+    // Do the same for b's keys but skip those that we already checked
>+    for (let k in b)
>+      if (!(k in a) && !eq(a[k], b[k]))
>+        return false;
>+
>+    return true;
>+  },
>+
>+  digest: function digest(message, hasher) {
>+    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+      createInstance(Ci.nsIScriptableUnicodeConverter);
>+    converter.charset = "UTF-8";
>+
>+    let data = converter.convertToByteArray(message, {});
>+    hasher.update(data, data.length);
>+    return hasher.finish(false);
>+  },
>+
>+  bytesAsHex: function bytesAsHex(bytes) {
>+    // Convert each hashed byte into 2-hex strings then combine them
>+    return [("0" + byte.charCodeAt().toString(16)).slice(-2)
>+            for each (byte in bytes)].join("");
>+  },
>+
>+  sha1Bytes: function sha1Bytes(message) {
>+    let hasher = Cc["@mozilla.org/security/hash;1"].
>+      createInstance(Ci.nsICryptoHash);
>+    hasher.init(hasher.SHA1);
>+    return Utils.digest(message, hasher);
>+  },
>+
>+  sha1: function sha1(message) {
>+    return Utils.bytesAsHex(Utils.sha1Bytes(message));
>+  },
>+
>+  sha1Base32: function sha1Base32(message) {
>+    return Utils.encodeBase32(Utils.sha1Bytes(message));
>+  },
>+
>+  /**
>+   * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>+   */
>+  sha256HMAC: function sha256HMAC(message, key) {
>+    let hasher = Cc["@mozilla.org/security/hmac;1"].
>+      createInstance(Ci.nsICryptoHMAC);
>+    hasher.init(hasher.SHA256, key);
>+    return Utils.bytesAsHex(Utils.digest(message, hasher));
>+  },
>+
>+  /**
>+   * Base32 encode (RFC 4648) a string
>+   */
>+  encodeBase32: function encodeBase32(bytes) {
>+    const key = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
>+    let quanta = Math.floor(bytes.length / 5);
>+    let leftover = bytes.length % 5;
>+
>+    // Pad the last quantum with zeros so the length is a multiple of 5.
>+    if (leftover) {
>+      quanta += 1;
>+      for (let i = leftover; i < 5; i++)
>+        bytes += "\0";
>+    }
>+
>+    // Chop the string into quanta of 5 bytes (40 bits). Each quantum
>+    // is turned into 8 characters from the 32 character base.
>+    let ret = "";
>+    for (let i = 0; i < bytes.length; i += 5) {
>+      let c = [byte.charCodeAt() for each (byte in bytes.slice(i, i + 5))];
>+      ret += key[c[0] >> 3]
>+           + key[((c[0] << 2) & 0x1f) | (c[1] >> 6)]
>+           + key[(c[1] >> 1) & 0x1f]
>+           + key[((c[1] << 4) & 0x1f) | (c[2] >> 4)]
>+           + key[((c[2] << 1) & 0x1f) | (c[3] >> 7)]
>+           + key[(c[3] >> 2) & 0x1f]
>+           + key[((c[3] << 3) & 0x1f) | (c[4] >> 5)]
>+           + key[c[4] & 0x1f];
>+    }
>+
>+    switch (leftover) {
>+      case 1:
>+        return ret.slice(0, -6) + "======";
>+      case 2:
>+        return ret.slice(0, -4) + "====";
>+      case 3:
>+        return ret.slice(0, -3) + "===";
>+      case 4:
>+        return ret.slice(0, -1) + "=";
>+      default:
>+        return ret;
>+    }
>+  },
>+
>+  encodeUTF8: function encodeUTF8(str) {
>+    try {
>+      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>+                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>+      unicodeConverter.charset = "UTF-8";
>+      str = unicodeConverter.ConvertFromUnicode(str);
>+      return str + unicodeConverter.Finish();
>+    } catch(ex) {
>+      return null;
>+    }
>+  },
>+
>+  decodeUTF8: function encodeUTF8(str) {
>+    try {
>+      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>+                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>+      unicodeConverter.charset = "UTF-8";
>+      str = unicodeConverter.ConvertToUnicode(str);
>+      return str + unicodeConverter.Finish();
>+    } catch(ex) {
>+      return null;
>+    }
>+  }
>+
>+};
>diff --git a/services/sync/tests/unit/test_utils_deepEquals.js b/services/crypto/tests/unit/test_utils_deepEquals.js
>rename from services/sync/tests/unit/test_utils_deepEquals.js
>rename to services/crypto/tests/unit/test_utils_deepEquals.js
>--- a/services/sync/tests/unit/test_utils_deepEquals.js
>+++ b/services/crypto/tests/unit/test_utils_deepEquals.js
>@@ -1,10 +1,10 @@
> _("Make sure Utils.deepEquals correctly finds items that are deeply equal");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let data = '[NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3]]';
>   _("Generating two copies of data:", data);
>   let d1 = eval(data);
>   let d2 = eval(data);
> 
>   d1.forEach(function(a) {
>diff --git a/services/sync/tests/unit/test_utils_encodeBase32.js b/services/crypto/tests/unit/test_utils_encodeBase32.js
>rename from services/sync/tests/unit/test_utils_encodeBase32.js
>rename to services/crypto/tests/unit/test_utils_encodeBase32.js
>--- a/services/sync/tests/unit/test_utils_encodeBase32.js
>+++ b/services/crypto/tests/unit/test_utils_encodeBase32.js
>@@ -1,9 +1,9 @@
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   // Test vectors from RFC 4648
>   do_check_eq(Utils.encodeBase32(""), "");
>   do_check_eq(Utils.encodeBase32("f"), "MY======");
>   do_check_eq(Utils.encodeBase32("fo"), "MZXQ====");
>   do_check_eq(Utils.encodeBase32("foo"), "MZXW6===");
>   do_check_eq(Utils.encodeBase32("foob"), "MZXW6YQ=");
>diff --git a/services/sync/tests/unit/test_utils_sha1.js b/services/crypto/tests/unit/test_utils_sha1.js
>rename from services/sync/tests/unit/test_utils_sha1.js
>rename to services/crypto/tests/unit/test_utils_sha1.js
>--- a/services/sync/tests/unit/test_utils_sha1.js
>+++ b/services/crypto/tests/unit/test_utils_sha1.js
>@@ -1,10 +1,10 @@
> _("Make sure sha1 digests works with various messages");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let mes1 = "hello";
>   let mes2 = "world";
> 
>   _("Make sure right sha1 digests are generated");
>   let dig1 = Utils.sha1(mes1);
>   do_check_eq(dig1, "aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d");
>diff --git a/services/sync/tests/unit/test_utils_sha256HMAC.js b/services/crypto/tests/unit/test_utils_sha256HMAC.js
>rename from services/sync/tests/unit/test_utils_sha256HMAC.js
>rename to services/crypto/tests/unit/test_utils_sha256HMAC.js
>--- a/services/sync/tests/unit/test_utils_sha256HMAC.js
>+++ b/services/crypto/tests/unit/test_utils_sha256HMAC.js
>@@ -1,14 +1,16 @@
> _("Make sure sha256 hmac works with various messages and keys");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>-  let key1 = Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key1");
>-  let key2 = Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key2");
>+  let keyFactory = Cc["@mozilla.org/security/keyobjectfactory;1"]
>+                     .getService(Ci.nsIKeyObjectFactory);
>+  let key1 = keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key1");
>+  let key2 = keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key2");
> 
>   let mes1 = "message 1";
>   let mes2 = "message 2";
> 
>   _("Make sure right sha256 hmacs are generated");
>   let hmac11 = Utils.sha256HMAC(mes1, key1);
>   do_check_eq(hmac11, "54f035bfbd6b44445d771c7c430e0753f1c00823974108fb4723703782552296");
>   let hmac12 = Utils.sha256HMAC(mes1, key2);
>diff --git a/services/sync/tests/unit/test_utils_utf8.js b/services/crypto/tests/unit/test_utils_utf8.js
>rename from services/sync/tests/unit/test_utils_utf8.js
>rename to services/crypto/tests/unit/test_utils_utf8.js
>--- a/services/sync/tests/unit/test_utils_utf8.js
>+++ b/services/crypto/tests/unit/test_utils_utf8.js
>@@ -1,8 +1,8 @@
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let str = "Umlaute: \u00FC \u00E4\n"; // Umlaute: ü ä
>   let encoded = Utils.encodeUTF8(str);
>   let decoded = Utils.decodeUTF8(encoded);
>   do_check_eq(decoded, str);
> }
>diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js
>--- a/services/sync/modules/util.js
>+++ b/services/sync/modules/util.js
>@@ -43,21 +43,26 @@
> 
> Cu.import("resource://services-sync/constants.js");
> Cu.import("resource://services-sync/ext/Observers.js");
> Cu.import("resource://services-sync/ext/Preferences.js");
> Cu.import("resource://services-sync/ext/StringBundle.js");
> Cu.import("resource://services-sync/ext/Sync.js");
> Cu.import("resource://services-sync/log4moz.js");
> 
>+let Crypto = {};
>+Cu.import("resource://services-crypto/util.js", Crypto);
>+
> /*
>  * Utility functions
>  */
> 
> let Utils = {
>+  __proto__: Crypto.Utils,
>+
>   /**
>    * Wrap a function to catch all exceptions and log them
>    *
>    * @usage MyObj._catch = Utils.catch;
>    *        MyObj.foo = function() { this._catch(func)(); }
>    */
>   catch: function Utils_catch(func) {
>     let thisArg = this;
>@@ -378,42 +383,16 @@
>     dest.__defineGetter__(prop, getter);
>   },
> 
>   lazyStrings: function Weave_lazyStrings(name) {
>     let bundle = "chrome://weave/locale/services/" + name + ".properties";
>     return function() new StringBundle(bundle);
>   },
> 
>-  deepEquals: function eq(a, b) {
>-    // If they're triple equals, then it must be equals!
>-    if (a === b)
>-      return true;
>-
>-    // If they weren't equal, they must be objects to be different
>-    if (typeof a != "object" || typeof b != "object")
>-      return false;
>-
>-    // But null objects won't have properties to compare
>-    if (a === null || b === null)
>-      return false;
>-
>-    // Make sure all of a's keys have a matching value in b
>-    for (let k in a)
>-      if (!eq(a[k], b[k]))
>-        return false;
>-
>-    // Do the same for b's keys but skip those that we already checked
>-    for (let k in b)
>-      if (!(k in a) && !eq(a[k], b[k]))
>-        return false;
>-
>-    return true;
>-  },
>-
>   deepCopy: function Weave_deepCopy(thing, noSort) {
>     if (typeof(thing) != "object" || thing == null)
>       return thing;
>     let ret;
> 
>     if (Utils.isArray(thing)) {
>       ret = [];
>       for (let i = 0; i < thing.length; i++)
>@@ -497,102 +476,16 @@
> 
>     return false;
>   },
> 
>   ensureStatus: function Weave_ensureStatus(args) {
>     if (!Utils.checkStatus.apply(Utils, arguments))
>       throw 'checkStatus failed';
>   },
>-
>-  digest: function digest(message, hasher) {
>-    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>-      createInstance(Ci.nsIScriptableUnicodeConverter);
>-    converter.charset = "UTF-8";
>-
>-    let data = converter.convertToByteArray(message, {});
>-    hasher.update(data, data.length);
>-    return hasher.finish(false);
>-  },
>-
>-  bytesAsHex: function bytesAsHex(bytes) {
>-    // Convert each hashed byte into 2-hex strings then combine them
>-    return [("0" + byte.charCodeAt().toString(16)).slice(-2)
>-            for each (byte in bytes)].join("");
>-  },
>-
>-  _sha1: function _sha1(message) {
>-    let hasher = Cc["@mozilla.org/security/hash;1"].
>-      createInstance(Ci.nsICryptoHash);
>-    hasher.init(hasher.SHA1);
>-    return Utils.digest(message, hasher);
>-  },
>-
>-  sha1: function sha1(message) {
>-    return Utils.bytesAsHex(Utils._sha1(message));
>-  },
>-
>-  sha1Base32: function sha1Base32(message) {
>-    return Utils.encodeBase32(Utils._sha1(message));
>-  },
>-
>-  /**
>-   * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>-   */
>-  sha256HMAC: function sha256HMAC(message, key) {
>-    let hasher = Cc["@mozilla.org/security/hmac;1"].
>-      createInstance(Ci.nsICryptoHMAC);
>-    hasher.init(hasher.SHA256, key);
>-    return Utils.bytesAsHex(Utils.digest(message, hasher));
>-  },
>-
>-  /**
>-   * Base32 encode (RFC 4648) a string
>-   */
>-  encodeBase32: function encodeBase32(bytes) {
>-    const key = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
>-    let quanta = Math.floor(bytes.length / 5);
>-    let leftover = bytes.length % 5;
>-
>-    // Pad the last quantum with zeros so the length is a multiple of 5.
>-    if (leftover) {
>-      quanta += 1;
>-      for (let i = leftover; i < 5; i++)
>-        bytes += "\0";
>-    }
>-
>-    // Chop the string into quanta of 5 bytes (40 bits). Each quantum
>-    // is turned into 8 characters from the 32 character base.
>-    let ret = "";
>-    for (let i = 0; i < bytes.length; i += 5) {
>-      let c = [byte.charCodeAt() for each (byte in bytes.slice(i, i + 5))];
>-      ret += key[c[0] >> 3]
>-           + key[((c[0] << 2) & 0x1f) | (c[1] >> 6)]
>-           + key[(c[1] >> 1) & 0x1f]
>-           + key[((c[1] << 4) & 0x1f) | (c[2] >> 4)]
>-           + key[((c[2] << 1) & 0x1f) | (c[3] >> 7)]
>-           + key[(c[3] >> 2) & 0x1f]
>-           + key[((c[3] << 3) & 0x1f) | (c[4] >> 5)]
>-           + key[c[4] & 0x1f];
>-    }
>-
>-    switch (leftover) {
>-      case 1:
>-        return ret.slice(0, -6) + "======";
>-      case 2:
>-        return ret.slice(0, -4) + "====";
>-      case 3:
>-        return ret.slice(0, -3) + "===";
>-      case 4:
>-        return ret.slice(0, -1) + "=";
>-      default:
>-        return ret;
>-    }
>-  },
>-
>   makeURI: function Weave_makeURI(URIString) {
>     if (!URIString)
>       return null;
>     try {
>       return Svc.IO.newURI(URIString, null, null);
>     } catch (e) {
>       let log = Log4Moz.repository.getLogger("Service.Util");
>       log.debug("Could not create URI: " + Utils.exceptionStr(e));
>@@ -801,40 +694,16 @@
>   readStream: function Weave_readStream(is) {
>     let ret = "", str = {};
>     while (is.readString(4096, str) != 0) {
>       ret += str.value;
>     }
>     return ret;
>   },
> 
>-  encodeUTF8: function(str) {
>-    try {
>-      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>-                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>-      unicodeConverter.charset = "UTF-8";
>-      str = unicodeConverter.ConvertFromUnicode(str);
>-      return str + unicodeConverter.Finish();
>-    } catch(ex) {
>-      return null;
>-    }
>-  },
>-
>-  decodeUTF8: function(str) {
>-    try {
>-      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>-                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>-      unicodeConverter.charset = "UTF-8";
>-      str = unicodeConverter.ConvertToUnicode(str);
>-      return str + unicodeConverter.Finish();
>-    } catch(ex) {
>-      return null;
>-    }
>-  },
>-
>   /**
>    * Generate 20 random characters a-z
>    */
>   generatePassphrase: function() {
>     let rng = Cc["@mozilla.org/security/random-generator;1"]
>                 .createInstance(Ci.nsIRandomGenerator);
>     let bytes = rng.generateRandomBytes(20);
>     return [String.fromCharCode(97 + Math.floor(byte * 26 / 256))

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 481840 [details] [diff] [review]
Prereq 2 (v1): Implement Utils.sha256()

># HG changeset patch
># Parent 097b68f44f00d6d8da06dbe031ef1d2fee93815e
># User Philipp von Weitershausen <[e-mail]>
>Bug 601645 - Implement Utils.sha256()
>
>diff --git a/services/crypto/modules/util.js b/services/crypto/modules/util.js
>--- a/services/crypto/modules/util.js
>+++ b/services/crypto/modules/util.js
>@@ -97,16 +97,27 @@
>   sha1: function sha1(message) {
>     return Utils.bytesAsHex(Utils.sha1Bytes(message));
>   },
> 
>   sha1Base32: function sha1Base32(message) {
>     return Utils.encodeBase32(Utils.sha1Bytes(message));
>   },
> 
>+  sha256Bytes: function sha256Bytes(message) {
>+    let hasher = Cc["@mozilla.org/security/hash;1"].
>+      createInstance(Ci.nsICryptoHash);
>+    hasher.init(hasher.SHA256);
>+    return Utils.digest(message, hasher);
>+  },
>+
>+  sha256: function sha256(message) {
>+    return Utils.bytesAsHex(Utils.sha256Bytes(message));
>+  },
>+
>   /**
>    * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>    */
>   sha256HMAC: function sha256HMAC(message, key) {
>     let hasher = Cc["@mozilla.org/security/hmac;1"].
>       createInstance(Ci.nsICryptoHMAC);
>     hasher.init(hasher.SHA256, key);
>     return Utils.bytesAsHex(Utils.digest(message, hasher));
>diff --git a/services/crypto/tests/unit/test_utils_sha256.js b/services/crypto/tests/unit/test_utils_sha256.js
>new file mode 100644
>--- /dev/null
>+++ b/services/crypto/tests/unit/test_utils_sha256.js
>@@ -0,0 +1,15 @@
>+Cu.import("resource://services-crypto/util.js");
>+
>+function run_test() {
>+  _("Hex variant");
>+  do_check_eq(Utils.sha256(""),
>+              "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
>+  do_check_eq(Utils.sha256("Bacon is a vegetable"),
>+              "684f507d70b0f5145967e77f6a12142fdc90a48d06658ece12f45fbbd404ceb2");
>+
>+  _("Bytes variant");
>+  do_check_eq(Utils.sha256Bytes("Cookies are delicious delicacies"),
>+              "\xd1U9ah\x06\t'\xd8\xbfH\xe3\xcal\xc9\xc2\xd0\xdd\xbaC\xe9\xef`6\x18hS\xf12\x9a\x95\xce");
>+  do_check_eq(Utils.sha256Bytes("Gort! Klaatu barada nikto!"),
>+              "'\xe8q\x07;\x1c{y\xf7l\xf5\x06\xce\xebY'u\x03\x11\xfb\xfe\x92X\xf9\xf7\xd8\xc6z\xc1af2");
>+}

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: Implement J-PAKE (v3)

Changes compared to v2 patch:

* Support SHA256 in addition to SHA1 when computing the Schnorr signature for zero-knowledge proofs.

* Support HMAC-SHA256 as a key extraction algorithm in addition to SHA256, using a fixed 256 bit key.

* Avoid using SHA1 functions from freebl, instead use nsICryptoHash.

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for J-PAKE implementation (v3)

Tests updated to reflect changes in v3 patch. Also added more test vectors generated with python-jpake (3072 bit modulus with SHA1 and SHA256 hashing in the Schnorr signature)

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: Implement J-PAKE (v4)

Use zeros as extraction key in the HMAC-SHA256 extraction (feedback from bsmith)

Comment 17

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for J-PAKE implementation (v4)

Updated hmac-sha256 generated test vectors.

Comment 18

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

Use these instructions to build a new version of NSS that supports J-PAKE using the patches from Bug 609068, Bug 609076, and Bug 614076:

cvs co NSPR NSS
cd mozilla/security/nss
patch -p0 < jpake-freebl-bsmith-2.patch
patch -p0 < jpake-freebl-test-bsmith-1.patch
patch -p0 < jpake-softoken-bsmith-1.patch
patch -p0 < jpake-util-bsmith-1.patch
patch -p0 < jpake-pk11mode-bsmith-1.patch
patch -p0 < jpake-bug-609076-hkdf-bsmith-1.patch

Hopefully, you can also be able to apply these patches directly to the NSS that is in mozilla-central with minimal changes.

Look at the changes in jpake-pk11mode-bsmith-1.patch to see what you need to do in Sync to use the NSS J-PAKE implementation. It won't be exactly the same because you need to use the pk11wrap functions instead of directly calling the PKCS#11 functions. To find the corresponding pk11wrap function, I suggest grepping through the pk11wrap directory for the name of the PKCS#11 function, and then look for the functions with a capital PK11_* prefix that call it (directly or indirectly). I will help you with this tomorrow.

Comment 19

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

Attachment: Patch for testing J-PAKE in mozilla-central until the new NSS release is checked in

This patch is NOT designed to be checked into mozilla-central. I just created it so that we could start working on the Sync JS changes needed to interface with it, so that the Sync code is ready to go once the NSS release is checked into mozilla-central.

Comment 20

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

Attachment: Rough sketch of what a pk11wrap-based J-PAKE application would look like. (v2) Not for check-in

Comment 21

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

Comment on attachment 495211 [details] [diff] [review]
Patch for testing J-PAKE in mozilla-central until the new NSS release is checked in

Use "python client.py update_nss NSS_3_12_BRANCH" instead.

Comment 22

[Philipp von Weitershausen [:philikon]]

(In reply to comment #21)
> Use "python client.py update_nss NSS_3_12_BRANCH" instead.

Thanks, that works like a charm. I can now start rewriting the JS implementation on top of NSS using your example code now.

Comment 23

[Philipp von Weitershausen [:philikon]]

Updating the summary to better reflect what the bug is about now that J-PAKE landed in NSS. Also assigning back to me.

Comment 24

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

Attachment: Rough sketch of what a pk11wrap-based J-PAKE application would look like. (v3) Not for check-in

Comment 25

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

Removed bug 609068, bug 609076, and bug 614076 from the blocker list because the code that Sync needs has been checked into the 3.12.9 release. Those bugs will remain open until the automated tests for the new code to be added but that will probably happen after the NSS 3.12.9 release.

Comment 26

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

Removed bug 609068, bug 609076, and bug 614076 from the blocker list because the code that Sync needs has been checked into the 3.12.9 release. Those bugs will remain open until the automated tests for the new code to be added but that will probably happen after the NSS 3.12.9 release.

Comment 27

[Philipp von Weitershausen [:philikon]]

Attachment: IDL and XPCOM shell for nsISyncJPAKE (not for check in)

Comment 28

[Philipp von Weitershausen [:philikon]]

Attachment: IDL and XPCOM shell for nsISyncJPAKE v2 (not for check in)

Dave, this is an empty C++ XPCOM shell that we're going to add an implementation to soon. Could you take a look and see if this makes sense? The idea is to instantiate one of these objects via

 Cc["@mozilla.org/services-crypto/sync-jpake;1"].createInstance(Ci.nsISyncJPAKE)

Thanks!

Comment 29

[Philipp von Weitershausen [:philikon]]

Comment on attachment 491941 [details] [diff] [review]
Implement J-PAKE (v4)

This mpi+ctypes implemetation is now obsolete.

Comment 30

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for XPCOM J-PAKE implementation

JPAKE tests rewritten against the API of the XPCOM component that Brian is working on.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 495693 [details] [diff] [review]
IDL and XPCOM shell for nsISyncJPAKE v2 (not for check in)

Looks generally fine, made a couple of alternate naming choices since you may or may not have just been following old form, feel free to ignore them, naming is up to you and the module owner.

>diff --git a/services/crypto/Makefile.in b/services/crypto/Makefile.in
>--- a/services/crypto/Makefile.in
>+++ b/services/crypto/Makefile.in
>@@ -43,16 +43,28 @@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE = services-crypto
> XPIDL_MODULE = services-crypto

Might be better just to call this crypto rather than prepending services everywhere.

> XPIDLSRCS = \
>   IWeaveCrypto.idl \
>+  nsISyncJPAKE.idl \

Kind of old form to use nsI but you could equally use syncIJPAKE or maybe since this is the crypto code cryptIJPAKE or something.

>diff --git a/services/crypto/nsISyncJPAKE.idl b/services/crypto/nsISyncJPAKE.idl

I'm not really reviewing anything in here.

>diff --git a/services/crypto/nsSyncJPAKE.cpp b/services/crypto/nsSyncJPAKE.cpp

>+#include "nsISyncJPAKE.h"
>+
>+/*** TODO should this go into a separate nsSyncJPAKE.h file? ***/

Yes that is standard and probably best to stick to that

>+#define NS_SYNCJPAKE_IID \
>+  {0x5ab02a98, 0x5122, 0x4b90, \
>+    { 0x93, 0xcd, 0xf2, 0x59, 0xc4, 0xb4, 0x2e, 0x3a }}

Shouldn't need to define this, it should be in nsISyncJPAKE.h. NS_GET_IID(nsISyncJPAKE) should make it if you need it.

>+#define NS_SYNCJPAKE_CONTRACTID \
>+  "@mozilla.org/services-crypto/sync-jpake;1"

I might suggest just "@mozilla.org/crypto/jpake;1"

>+class nsSyncJPAKE : public nsISyncJPAKE
>+{
>+public:
>+  nsSyncJPAKE();
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSISYNCJPAKE
>+
>+private:
>+  ~nsSyncJPAKE();
>+}

Here is where nsSyncJPAKE.h ends

>+NS_IMPL_ISUPPORTS1(nsSyncJPAKE, nsISyncJPAKE)
>+
>+/*** TODO implementation goes here ***/

>+/*** TODO should this go into a separate nsServicesCryptoModule.cpp file? ***/

Kind of a judgement call. There is only one component so there is not much point right now. If you add more in the future then you'll definitely want to split it out then for clarity so I guess if you reasonably suspect you'll add any more in the future then might as well split it now and hg blame will be cleaner for it.

>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsSyncJPAKE)

How badly would the implementation break if people created multiple instances of it? You may want to consider using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR to make that impossible though it is a little extra work on your part.

>+NS_DEFINE_NAMED_CID(NS_ISYNCJPAKE_CID);
>+
>+static const mozilla::Module::CIDEntry kServicesCryptoCIDs[] = {
>+  { &kNS_ISYNCJPAKE_CID, false, NULL, nsSyncJPAKEConstructor },
>+  { NULL }
>+};
>+
>+static const mozilla::Module::ContractIDEntry kServicesCryptoContracts[] = {
>+  { NS_SYNCJPAKE_CONTRACTID, &kNS_ISYNCJPAKE_CID },
>+  { NULL }
>+};
>+
>+static const mozilla::Module kServicesCryptoModule = {
>+  mozilla::Module::kVersion,
>+  kServicesCryptoCIDs,
>+  kServicesCryptoContracts,
>+  kServicesCryptoCategories

I don't see kServicesCryptoCategories defined anywhere so I'm guessing this doesn't compile yet. You don't need it anyway normally.

Comment 32

[Philipp von Weitershausen [:philikon]]

Thanks a lot for the feedback, Dave!

(In reply to comment #31)
> Looks generally fine, made a couple of alternate naming choices since you may
> or may not have just been following old form, feel free to ignore them, naming
> is up to you and the module owner.

Ok, I'll have mconnor have the last word here. ;)

> >+#define NS_SYNCJPAKE_CONTRACTID \
> >+  "@mozilla.org/services-crypto/sync-jpake;1"
> 
> I might suggest just "@mozilla.org/crypto/jpake;1"

I had that initially. But this is a particular JPAKE API that's 100% tailoured to Sync's credentials exchange, so I think the naming should reflect that.

> >+NS_GENERIC_FACTORY_CONSTRUCTOR(nsSyncJPAKE)
> 
> How badly would the implementation break if people created multiple instances
> of it? You may want to consider using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
> to make that impossible though it is a little extra work on your part.

Actually, we specifically want to allow multiple instances of this.

> >+static const mozilla::Module kServicesCryptoModule = {
> >+  mozilla::Module::kVersion,
> >+  kServicesCryptoCIDs,
> >+  kServicesCryptoContracts,
> >+  kServicesCryptoCategories
> 
> I don't see kServicesCryptoCategories defined anywhere so I'm guessing this
> doesn't compile yet. You don't need it anyway normally.

Good point, I just copied it from other modules ;). So what to use here, NULL?

Comment 33

[Philipp von Weitershausen [:philikon]]

Attachment: IDL and XPCOM shell for nsISyncJPAKE v3 (not for check in)

Incorporate Mossop's feedback. This one actually compiles! \o/

Comment 34

[Dave Townsend [:mossop]]

(In reply to comment #32)
> > >+static const mozilla::Module kServicesCryptoModule = {
> > >+  mozilla::Module::kVersion,
> > >+  kServicesCryptoCIDs,
> > >+  kServicesCryptoContracts,
> > >+  kServicesCryptoCategories
> > 
> > I don't see kServicesCryptoCategories defined anywhere so I'm guessing this
> > doesn't compile yet. You don't need it anyway normally.
> 
> Good point, I just copied it from other modules ;). So what to use here, NULL?

You just shouldn't need to include it at all.

Comment 35

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

Attachment: IDL, XPCOM shell, and implementation for nsISyncJPAKE (not for check in)

Comment 36

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

Attachment: IDL, XPCOM shell, and implementation for nsISyncJPAKE (not for check in) (v3)

Switched to nsStringAPI, switched to 2-space indent, and added some missing return value logic.

At this point, it compiles but it doesn't link. I'm not sure where to add the references to the necessary libraries (pk11wrap and the string library).

Comment 37

[Philipp von Weitershausen [:philikon]]

(In reply to comment #36)
> Switched to nsStringAPI, switched to 2-space indent, and added some missing
> return value logic.

Thanks for the quick fix!

> At this point, it compiles but it doesn't link. I'm not sure where to add the
> references to the necessary libraries (pk11wrap and the string library).

Needed to add this to the very *bottom* of Makefile.in:

EXTRA_DSO_LDOPTS += \
	$(XPCOM_GLUE_LDOPTS) \
	$(NSPR_LIBS) \
	$(NSS_LIBS) \
	$(NULL)

Now builds fine, but the unit test fails with:

WARNING: NS_ENSURE_TRUE(slot != __null) failed: file /Users/philipp/dev/mozilla-weave/mc-jpake/services/crypto/nsSyncJPAKE.cpp, line 166
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISyncJPAKE.round1]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: /Users/philipp/dev/mozilla-weave/mc-jpake/obj-ff-dbg/_tests/xpcshell/services/crypto/tests/unit/test_jpake.js :: test_same_signerids :: line 25"  data: no]

Comment 38

[Mike Connor [:mconnor]]

Comment on attachment 495729 [details] [diff] [review]
Tests for XPCOM J-PAKE implementation

these look good, but definitely would like Brian's feedback as well.

Comment 39

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

Attachment: IDL, XPCOM shell, and implementation for nsISyncJPAKE (v4)

This depends on the branch version of the patch in bug 617492.

TEST-PASS | c:\p\mc\ff-dbg\_tests\xpcshell\services\crypto\tests\unit\test_jpake.js | test passed
INFO | Result summary:
INFO | Passed: 4
INFO | Failed: 0

Comment 40

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

Comment on attachment 495729 [details] [diff] [review]
Tests for XPCOM J-PAKE implementation

1. Add the PSM initialization to the top of the file: Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

2. Move test_success() to be the first sub-test. This way, if we get errors from NSS during the first sub-test, it probably means there is a bug in this code or in NSS; if the first sub-test passes, and then we get errors, then it probably means that the code is probably correct.

Comment 41

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

(In reply to comment #39)
> Created attachment 496105 [details] [diff] [review]
> 
> This depends on the branch version of the patch in bug 617492.

... and the first change to the test (to initialize PSM) mentioned in comment 39.

Comment 42

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

Attachment: IDL, XPCOM shell, and implementation for nsISyncJPAKE (v6)

With base64 -> base16 for key confirmation + build system changes + philiKON's build system changes.

Comment 43

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for XPCOM J-PAKE implementation (v2)

Incorporate bsmith's feedback:
* Initialise PSM at the beginning of the test
* Change order of tests

Comment 44

[Philipp von Weitershausen [:philikon]]

Attachment: Tests for XPCOM J-PAKE implementation (v3)

Update tests: we're going to get rid of the aKeyVerification parameter in the final() method.

Comment 45

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

Attachment: IDL, XPCOM shell, and implementation for nsISyncJPAKE (v7)

With changes to support new key confirmation.

Comment 46

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

Firefox and Firefox are not deriving the same keys from the J-PAKE exchange. Stefan, philiKON, and I are debugging that now.

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 496345 [details] [diff] [review]
IDL, XPCOM shell, and implementation for nsISyncJPAKE (v7)

Overall this looks fine, though the style is a bit odd in places.  Don't really care right now though.

>diff --git a/services/crypto/nsISyncJPAKE.idl b/services/crypto/nsISyncJPAKE.idl

>+  /**
>+   * Perform first round of the JPAKE exchange.

round2 is second round, I hope.

>+  void round2(in ACString aPeerID,

>diff --git a/services/crypto/nsSyncJPAKE.cpp b/services/crypto/nsSyncJPAKE.cpp

>+#define NUM_ELEM(x) (sizeof(x) / sizeof (x)[0])
>+
>+// TODO: validate PQG

Is there a bug on this and other TODO comments?  If so, please note in the comment, if not, file and note in the comment.

>+//NS_DEFINE_STATIC_IID_ACCESSOR(nsISyncJPAKE, NS_ISYNCJPAKE_IID)

if not needed, please remove

Comment 48

[Philipp von Weitershausen [:philikon]]

Attachment: Patch on top of v7 to build as part of libxul

This gets us to build as part of libxul, despite being an app tier, by adding some declarations to confvars.sh and build.mk. Needed changes to nsSyncCrypto because we're now using interal strings (diverging APIs FTL).

Thanks a lot of mwu for pointing me to the right bits.

Comment 49

[Matt Brubeck (:mbrubeck)]

(In reply to comment #48)
> This gets us to build as part of libxul, despite being an app tier, by adding
> some declarations to confvars.sh and build.mk.

If you want to do the same for Fennec, you will need to modify confvars.sh in the mobile-browser repo too.

Comment 50

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

Attachment: Updated pk11mode.c that I'm using to test HKDF implementation against expected values

It would be very useful if we could replicate at least one of the standard test vectors in the Objective-C implementation. For example, test vector (prk3, info3, expectedValue3). It is very strange that Python, Obj-C, and Javascript agree with each other and the NSS C implementation agrees with the spec test vectors, but the test case for interop NSS <-> Obj-C fails. Maybe there is some disagreement between the algorithm description on our wiki (used for the scripting language implementations) and the actual spec (which I based the NSS implementation on).

If somebody wants to look over my test to make sure I am not making some dumb mistake that is causing a false test failure, please do so. This file is mozilla/security/nss/cmd/pk11mode.c where I am adding the tests for the HKDF. The function with the actual tests is PKM_HKDFTest.

Comment 51

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

Comment on attachment 496413 [details]
Updated pk11mode.c that I'm using to test HKDF implementation against expected values

It seems like the cause of the interoperability problem has been identified and a separate bug has been filed about it. No changes to NSS's HKDF implementation will be needed. I will upload an updated versions of my patches shortly.

Comment 52

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

My patch needs the following change:

+  PK11SymKey * keyMaterial = PK11_Derive(key, CKM_NSS_JPAKE_FINAL_SHA256,
+                                         &paramsItem, CKM_NSS_HKDF_SHA256,
+                                         CKA_DERIVE, 0);
+  PK11SymKey * expanded = NULL;

   if (keyMaterial == NULL)
     rv = mapErrno();

+  if (rv == NS_OK) {

I will fix this in the next version after we have verified interoperability with FF home in the morning.

Comment 53

[Philipp von Weitershausen [:philikon]]

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

Requesting feedback from Mark Finkle to ensure the Fennec guys are happy with this.

Also requesting feedback from Mike Hommey. Mike has experience with Firefox on top of XULRunner. Basically this is an XPCOM component that lives in services (part of app-tier) but should be in libxul. We're using the same tricks that Thunderbird/SeaMonkey do to make that happen.

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

The problem I can see, with the patch as is, is that the services/crypto directory is not going to be built in ffx-on-xr cases. But there is currently nothing allowing that to happen in the build system.

I do wonder, however, if there isn't an interest in making this component part of the platform instead of the app (and here I mean the crypto component, not the whole sync stack).

Comment 55

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

Fennec would be willing to take these changes, in order to expedite getting JPAKE in the product. I do agree with Mike about moving the component into the platform. What are the downsides to that?

Comment 56

[Mike Connor [:mconnor]]

Moving just the crypto component is probably okay.  Not ideal, but okay.  What's the best way to do that?

Comment 57

[Benjamin Smedberg]

Why is this part of libxul? I helped you *not* make it part of libxul yesterday, no? I don't think we should do the hybrid approach.

Comment 58

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

> I do wonder, however, if there isn't an interest in making this component part
> of the platform instead of the app (and here I mean the crypto component, not
> the whole sync stack).

The crypto component is pretty Sync-specific because of the string parameter encoding/decoding and the API is generally unstable. I don't care where it goes but if it will be considered part of platform then after B8 we need to change the interface to make it suitable as a platform API.

Comment 59

[Benjamin Smedberg]

As decided on IRC: we're going to make this particular binary component part of libxul for b8 (and perhaps for final): it should be part of tier_toolkit.

Comment 60

[Philipp von Weitershausen [:philikon]]

Attachment: Patch on top of v7 to build as part of toolkit

Comment 61

[Benjamin Smedberg]

Comment on attachment 496549 [details] [diff] [review]
Patch on top of v7 to build as part of toolkit

I think that EXTRA_DSO_LDOPTS is probably incorrect here: you want to keep it after rules.mk, and it should be MOZ_COMPONENT_LIBS. But it would only matter in non-libxul builds.

But the critical problem here is that the ordering of toolkit-tiers.mk is definitely wrong: you're building services/crypto *after* toolkit/library, and that's definitely not what you want. Did you do a tryserver or clean build of this patch?

Comment 62

[Philipp von Weitershausen [:philikon]]

Attachment: Build as part of toolkit (v2)

Incorporated bsmedberg's review comment (quite right about the ordering!)

Comment 63

[Benjamin Smedberg]

Comment on attachment 496557 [details] [diff] [review]
Build as part of toolkit (v2)

nit, there's a hard tab in services/crypto/Makefile.in which should be removed

Comment 64

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

Attachment: nsISyncJPake component (v8) with all build system changes

I am having some trouble getting the merged patch to build but I think I fixed my problem. Please take a look at this to make sure I picked up all your recent changes.

Comment 65

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

Attachment: nsISyncJPake component (v9) with build system changes

Posted wrong version of the patch. Here's the latest I have but there is an error in linking libxul:

   Creating library xul.lib and object xul.exp
nsStaticXULComponents.obj : error LNK2001: unresolved external symbol "struct mozilla::Module const
* const nsServicesCryptoModule_NSModule" (?nsServicesCryptoModule_NSModule@@3QBUModule@mozilla@@B)
xul.dll : fatal error LNK1120: 1 unresolved externals

Comment 66

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

Attachment: nsISyncJPake component (v10) with build system changes

v9 failed because it was missing changes to libxul-config.mk.

Now everything builds and unit tests pass (on Windows, at least).

Comment 67

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

Attachment: nsISyncJPake component (v11) with build system changes

This is the same as v10 except the typo in the IDL was fixed ("first" -> "second").

(In reply to comment #47)
> >+// TODO: validate PQG
> 
> Is there a bug on this and other TODO comments?  If so, please note in the
> comment, if not, file and note in the comment.

If Sync interoperates with FF home then that is enough validation for me. Aldo, the OOM worries were handled by the change from nsStringAPI.h to nsString.h. I removed the comments.

> >+//NS_DEFINE_STATIC_IID_ACCESSOR(nsISyncJPAKE, NS_ISYNCJPAKE_IID)
> 
> if not needed, please remove

Gone.

Comment 68

[Philipp von Weitershausen [:philikon]]

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

Tests pass, try build on (rev 40de05bd875f) is very green. I'd say this is ready to land.

Comment 69

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch

This patch makes the same build change for mobile-browser.

Comment 70

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #59)
> As decided on IRC: we're going to make this particular binary component part of
> libxul for b8 (and perhaps for final): it should be part of tier_toolkit.

So, why are we doing this?  If I'm reading the patch correctly it is going to break Firefox (and Fennec) on Xulrunner. (Xulrunner is built without MOZ_SERVICES_SYNC, so the contents of services/crypto will be absent.)

If you really really want to split sync across the tiers (which I don't care for, but I'll defer to bsmedberg) you should put the JPAKE stuff into another directory (say, services/jpake) that is built unconditionally as part of tier_platform and then leave everything else alone.

Comment 71

[Mike Connor [:mconnor]]

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

gah, thought I marked this earlier!

bombs away, as it were.

Comment 72

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

Please address comment 70 (even if "addressing" consists of filing followup bugs with a solid plan to fix Firefox on XR) before landing.

Comment 73

[Philipp von Weitershausen [:philikon]]

(In reply to comment #70)
> If you really really want to split sync across the tiers (which I don't care
> for, but I'll defer to bsmedberg) you should put the JPAKE stuff into another
> directory (say, services/jpake) that is built unconditionally as part of
> tier_platform and then leave everything else alone.

Or we just build services/crypto unconditionally as part of tier_platform. I would say let's to do that in a follow-up bug. Mike already volunteered on IRC to look into it tomorrow. But I don't think it should block progress now.

Comment 74

[Philipp von Weitershausen [:philikon]]

(In reply to comment #72)
> Please address comment 70 (even if "addressing" consists of filing followup
> bugs with a solid plan to fix Firefox on XR) before landing.

Filed bug 618195 as a follow-up

Comment 75

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/7916ff577de1
https://hg.mozilla.org/mozilla-central/rev/006396b0b990

Comment 76

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/6761114011ed