602876 - Implement network client for credentials exchange via J-PAKE

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Network client v0.1 (wip)

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Tests v0.1 (wip)

Comment 3

[Philipp von Weitershausen [:philikon]]

The v0.1 wip doesn't not yet support the /report API on the server, and the test coverage could probably be better.

Comment 4

[Philipp von Weitershausen [:philikon]]

Things that still need to be addressed (mostly a todo list for myself):

* Support /report API
* Longer timeout for initial response (300 seconds), then 10 seconds for subsequent messages
* Do not use 1/l/O/o in generated secret (bug 610839)
* Make sure that no basic auth is sent for J-PAKE requests (I believe right now it just chooses the default authenticator which uses Sync credentials)

Comment 5

[Philipp von Weitershausen [:philikon]]

* Different derivation scheme for encryption and HMAC key (same as for Sync crypto):

  T(1) = HMAC-SHA256(input-key-string, "" + "sync" + 0x01)
  T(2) = HMAC-SHA256(input-key-string, T(1) + "sync" + 0x02)

* Consider using AES-128 crypto since that's the maximum strenght of the whole system.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Network client v0.2 (wip)

Changes from v0.1:

* Use JPAKE's HMAC-SHA256 key extractor.

* Expand the key to encryption key and hmac key as follows:

  encr = HMAC-SHA256(key_string, "" + "Sync-AES_256_CBC-HMAC256" + 0x01)
  hmac = HMAC-SHA256(key_string, encr + Sync-AES_256_CBC-HMAC256" + 0x02)

* Do not use 1,l,o,0 in the PIN

Still to do:

* Support /report API

* Longer timeout for initial response, shorter ones for subsequent messages

* Don't add basic auth headers

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: Network client v0.9

Changes since v0.2:

* When deleting a channel due to an error, report the error code in the X-KeyExchange-Log header. See https://wiki.mozilla.org/Services/Sync/SyncKey/J-PAKE#Failure_modes for the different error codes.

* The initial polling is done with a much larger timeout (300 polls => >~300 seconds), timeouts for subsequent messages are shorter (10 polls => >~10 seconds). This allows the user ~5 minutes to find the other device and enter the code while a device or network connectivity dying in the middle of an exchange will lead to a failure quickly.

* Ensure that basic auth headers aren't sent along with any J-PAKE requests.

* Any network or server errors that lead to a channel not being available get a special error code. This can then be used by the UI to automatically skip to the manual setup screen, whereas network or server errors in the actual exchange will provoke a retry in the UI.

This isn't ready for landing yet as it uses the js-ctypes J-PAKE implementation (bug 601645) as opposed to the proper C implementation (bug 609068). However switching that over should be rather small so I'm submitting this for review now to get the main parts reviewed.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: Tests v0.9

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 492246 [details] [diff] [review]
Network client v0.9

>diff --git a/services/sync/modules/constants.js b/services/sync/modules/constants.js

>+HMAC_INPUT:                            "Sync-AES_256_CBC-HMAC256",
>+JPAKE_ERROR_CHANNEL:                   "jpake.error.channel",
>+JPAKE_ERROR_NETWORK:                   "jpake.error.network",
>+JPAKE_ERROR_SERVER:                    "jpake.error.server",
>+JPAKE_ERROR_TIMEOUT:                   "jpake.error.timeout",
>+JPAKE_ERROR_INTERNAL:                  "jpake.error.internal",
>+JPAKE_ERROR_INVALID:                   "jpake.error.invalid",
>+JPAKE_ERROR_NODATA:                    "jpake.error.nodata",
>+JPAKE_ERROR_KEYMISMATCH:               "jpake.error.keymismatch",
>+JPAKE_ERROR_WRONGMESSAGE:              "jpake.error.wrongmessage",
>+JPAKE_SIGNERID_SENDER:                 "sender",
>+JPAKE_SIGNERID_RECEIVER:               "receiver",
>+JPAKE_LENGTH_MODULUS:                  3072,
>+JPAKE_LENGTH_SECRET:                   8,
>+JPAKE_LENGTH_CLIENTID:                 256,

I think these should probably just be consts on JPAKEClient, I don't think they're useful as global consts, and I'd rather not load these consts unless absolutely needed.

>diff --git a/services/sync/modules/jpakeclient.js b/services/sync/modules/jpakeclient.js

>+  _setClientID: function _setClientID() {
>+    let rng = Cc["@mozilla.org/security/random-generator;1"]
>+                .createInstance(Ci.nsIRandomGenerator);
>+    let bytes = rng.generateRandomBytes(JPAKE_LENGTH_CLIENTID / 2);
>+    this._clientID = [("0" + byte.toString(16)).slice(-2)
>+                      for each (byte in bytes)].join("");
>+  },
>+
>+  _createSecret: function _createSecret() {
>+    // 0-9a-z without 1,l,o,0
>+    const key = "23456789abcdefghijkmnpqrstuvwxyz";
>+    let rng = Cc["@mozilla.org/security/random-generator;1"]
>+                .createInstance(Ci.nsIRandomGenerator);
>+    let bytes = rng.generateRandomBytes(JPAKE_LENGTH_SECRET);

a part of me wants this rng call to be a helper (Utils.getRandomBytes(bytes)), since we use this in a bunch of places, including tests.  Followup is fine.

>+    return [key[Math.floor(byte * key.length/256)]
>+            for each (byte in bytes)].join("");

nit: spaces around operators, please.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> I think these should probably just be consts on JPAKEClient, I don't think
> they're useful as global consts, and I'd rather not load these consts unless
> absolutely needed.

Ok, we can do that for the ones that are not errors. The error ones are passed to UI code which can then act upon the error (kind of like it looks at Status.login etc.)

> a part of me wants this rng call to be a helper (Utils.getRandomBytes(bytes)),
> since we use this in a bunch of places, including tests.  Followup is fine.

That's definitely a good idea, rnewman and I came across this in the simplify crypto stuff as well.

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: Network client v1.0

Support the final report API. Address mconnor's review comments.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Tests v1.0

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 493319 [details] [diff] [review]
Network client v1.0

>+    resource.get(Utils.bind2(this, function (error, response) {

This needs to be a POST request instead of a GET.

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: Network client as-landed [Checked in: See comment 16]

Rebased on top of the nsISyncJPAKE API provided by bug 601645.

Comment 15

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/fx-sync/rev/2a06dffbca52
https://hg.mozilla.org/services/fx-sync/rev/8518be049f26

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 496706 [details] [diff] [review]
Network client as-landed
[Checked in: See comment 16]

http://hg.mozilla.org/mozilla-central/rev/e8883fb40d64
without Makefile part