610839 - To reduce potential user error remove 1/l/0/o from sync keys and jpake codes

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The characters 1 and l, and the characters 0 and o are often confused by users when they are entering automatically generated codes.  We should not include these 4 characters in Sync Keys that we generate, or in j-pake codes.

Comment 1

[Richard Newman [:rnewman]]

The amended functions below will be going into my Big Crypto Patch (Bug 603489). I can split this change out into a separate patch, but I don't know if it's worthwhile/meaningful to do so.

Note that there's still a UI component here: when users are entering a sync key we should probably restrict the set of input characters, amongst other things.


  /**
   * Generate 26 characters.
   */
  generatePassphrase: function generatePassphrase() {
    // Note that this is a different base32 alphabet to the 
    // one we use internally. It's lowercase, uses different letters,
    // and isn't intended for decoding!
    const key = "abcdefghijkmnpqrstuvwxyz23456789";
    let rng = Cc["@mozilla.org/security/random-generator;1"]
                .createInstance(Ci.nsIRandomGenerator);
    let bytes = rng.generateRandomBytes(26);
    return [key[Math.floor(byte * 32 / 256)]
            for each (byte in bytes)].join("");
  },
  
  isPassphrase: function(s) {
    if (s) {
      let normalized = Utils.normalizePassphrase(s);
      return normalized.match(/^[abcdefghijkmnpqrstuvwxyz23456789]{26}$/);
    }
    return false;
  },

  /**
   * Hyphenate a passphrase (26 characters) into groups.
   * aabbbbccccddddeeeeffffgggg
   * =>
   * aa-bbbb-cccc-dddd-eeee-ffff-gggg
   */
  hyphenatePassphrase: function(passphrase) {
    return Utils.prettifyBase32(passphrase);
  },


E.g.,

  Utils.generatePassphrase();
  => 64pxrpcc5uipd8dxi7grkzh33d

  Utils.hyphenatePassphrase(Utils.generatePassphrase());
  => eg-sk68-d3q9-5yr6-ibxi-mdcm-i9st

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> Note that there's still a UI component here: when users are entering a sync key
> we should probably restrict the set of input characters, amongst other things.

Actually users will no longer be able to enter a custom sync key.

>   isPassphrase: function(s) {

Out of curiousity, what do we need this for?

Comment 3

[Richard Newman [:rnewman]]

> Actually users will no longer be able to enter a custom sync key.

So our only avenue for setting up a new machine is J-PAKE?

If that's not true, then you're still going to have users typing... and that means we have an opportunity to make it easier for them.

 
> >   isPassphrase: function(s) {
> 
> Out of curiousity, what do we need this for?

During a potential upgrade. We look at the local passphrase (which isn't versioned) and see if it looks like a passphrase. If not, we take it to PBKDF2. This avoids any hairiness of possibly doing the key bootstrap twice.

Snippet:

  upgradeSyncKey: function upgradeSyncKey(syncID) {
    let p = this.passphrase;
    
    // Check whether it's already a key that we generated.
    if (isPassphrase(p)) {
      this._log.info("Sync key is up-to-date: no need to upgrade.");
      return;
    }
    
    // Maybe it's one the user entered with the wrong case...
    if (isPassphrase(p.toLowerCase())) {
      this._log.info("Sync key would be a passphrase if downcased; doing so.");
      this.passphrase = p.toLowerCase();
      this.persistLogin();
      return;
    }
    
    // Otherwise, let's upgrade it.
    ...

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> > Actually users will no longer be able to enter a custom sync key.
> 
> So our only avenue for setting up a new machine is J-PAKE?

No, they can still enter it manually when setting up a second device. I was talking about the initial setup and changing the sync key. In both cases you can provide your own sync key right now, but we're not going to allow that. It will always be auto-generated.

> During a potential upgrade. We look at the local passphrase (which isn't
> versioned) and see if it looks like a passphrase. If not, we take it to PBKDF2.
> This avoids any hairiness of possibly doing the key bootstrap twice.

Makes sense. Thanks!

Comment 5

[Tarek Ziadé (:tarek)]

Done in the JPake server at http://hg.mozilla.org/services/server-key-exchange/rev/a74c150161d1

Comment 6

[Richard Newman [:rnewman]]

This logic is in the approved patches for Bug 603489. When that goes into the tree (it's in try now), I consider the first half of this bug done.

Tarek and Philipp, is the J-PAKE part of this also complete? If so, I'll hold my finger over the 'RESOLVED' trigger...

Comment 7

[Tracy Walker [:tracy]]

verified with recent nightly builds against stage-auth