Closed Bug 610839 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: faaborg, Assigned: rnewman)

References

Details

(Keywords: ux-error-prevention)

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.
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
Status: NEW → ASSIGNED
Assignee: nobody → rnewman
(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?
> 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.
    ...
(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!
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...
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified with recent nightly builds against stage-auth
Status: RESOLVED → VERIFIED
blocking2.0: --- → beta8+
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.