Implement network client for credentials exchange via J-PAKE

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
9 years ago
9 months ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+, fennec2.0b3+)

Details

Attachments

(2 attachments, 6 obsolete attachments)

No description provided.
Depends on: 603301
Blocks: 605740
No longer blocks: 601644
Posted patch Network client v0.1 (wip) (obsolete) — Splinter Review
Assignee: nobody → philipp
Posted patch Tests v0.1 (wip) (obsolete) — Splinter Review
The v0.1 wip doesn't not yet support the /report API on the server, and the test coverage could probably be better.
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)
* 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.
Posted patch Network client v0.2 (wip) (obsolete) — Splinter Review
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
Attachment #487804 - Attachment is obsolete: true
Posted patch Network client v0.9 (obsolete) — Splinter Review
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.
Attachment #491793 - Attachment is obsolete: true
Attachment #492246 - Flags: review?(mconnor)
Posted patch Tests v0.9 (obsolete) — Splinter Review
Attachment #487805 - Attachment is obsolete: true
Attachment #492247 - Flags: review?(mconnor)

Updated

9 years ago
blocking2.0: --- → beta8+
tracking-fennec: --- → 2.0b3+
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.
Attachment #492246 - Flags: review?(mconnor) → review+
(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.
Attachment #492247 - Flags: review?(mconnor) → review+
Posted patch Network client v1.0 (obsolete) — Splinter Review
Support the final report API. Address mconnor's review comments.
Attachment #492246 - Attachment is obsolete: true
Posted patch Tests v1.0Splinter Review
Attachment #492247 - Attachment is obsolete: true
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.
Whiteboard: [has patch][has reviews]
Blocks: 616251
No longer blocks: 616251
Rebased on top of the nsISyncJPAKE API provided by bug 601645.
Attachment #493319 - Attachment is obsolete: true
https://hg.mozilla.org/services/fx-sync/rev/2a06dffbca52
https://hg.mozilla.org/services/fx-sync/rev/8518be049f26
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Blocks: 617531
Status: RESOLVED → VERIFIED
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
Attachment #496706 - Attachment description: Network client as-landed → Network client as-landed [Checked in: See comment 16]
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.