Fix race and remove unused features in cryptotoken extension

BUG=378965

Review URL: https://codereview.chromium.org/308173002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274016 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/resources/cryptotoken/background.js

@@ -31,15 +31,14 @@ var ENROLL_HELPER_FACTORY = new UsbEnrollHelperFactory(GNUBBY_FACTORY);
  */
 function handleWebPageRequest(toleratesMultipleResponses, request, sender,
     sendResponse) {
-  var enforceAppIdValid = true;
   switch (request.type) {
     case GnubbyMsgTypes.ENROLL_WEB_REQUEST:
       return handleEnrollRequest(ENROLL_HELPER_FACTORY, sender, request,
-          enforceAppIdValid, sendResponse, toleratesMultipleResponses);
+          sendResponse, toleratesMultipleResponses);
 
     case GnubbyMsgTypes.SIGN_WEB_REQUEST:
       return handleSignRequest(SIGN_HELPER_FACTORY, sender, request,
-          enforceAppIdValid, sendResponse, toleratesMultipleResponses);
+          sendResponse, toleratesMultipleResponses);
 
     default:
       var response = formatWebPageResponse(

chrome/browser/resources/cryptotoken/enroller.js

@@ -13,16 +13,14 @@
  * @param {!EnrollHelperFactory} factory Factory to create an enroll helper.
  * @param {MessageSender} sender The sender of the message.
  * @param {Object} request The web page's enroll request.
- * @param {boolean} enforceAppIdValid Whether to enforce that the appId in the
- *     request matches the sender's origin.
  * @param {Function} sendResponse Called back with the result of the enroll.
  * @param {boolean} toleratesMultipleResponses Whether the sendResponse
  *     callback can be called more than once, e.g. for progress updates.
  * @return {Closeable} A handler object to be closed when the browser channel
  *     closes.
  */
-function handleEnrollRequest(factory, sender, request, enforceAppIdValid,
-    sendResponse, toleratesMultipleResponses) {
+function handleEnrollRequest(factory, sender, request, sendResponse,
+    toleratesMultipleResponses) {
   var sentResponse = false;
   function sendResponseOnce(r) {
     if (enroller) {
@@ -119,7 +117,7 @@ function handleEnrollRequest(factory, sender, request, enforceAppIdValid,
   var timer = new CountdownTimer(timeoutMillis);
   var enroller = new Enroller(factory, timer, origin, sendErrorResponse,
       sendSuccessResponse, sendNotification, sender.tlsChannelId, logMsgUrl);
-  enroller.doEnroll(enrollChallenges, signData, enforceAppIdValid);
+  enroller.doEnroll(enrollChallenges, signData);
   return /** @type {Closeable} */ (enroller);
 }
 
@@ -232,30 +230,18 @@ Enroller.DEFAULT_TIMEOUT_MILLIS = 30 * 1000;
  * @param {Array.<Object>} enrollChallenges A set of enroll challenges
  * @param {Array.<Object>} signChallenges A set of sign challenges for existing
  *     enrollments for this user and appId
- * @param {boolean} enforceAppIdValid Whether to enforce that appId is valid
  */
-Enroller.prototype.doEnroll =
-    function(enrollChallenges, signChallenges, enforceAppIdValid) {
+Enroller.prototype.doEnroll = function(enrollChallenges, signChallenges) {
   this.setEnrollChallenges_(enrollChallenges);
   this.setSignChallenges_(signChallenges);
 
-  if (!enforceAppIdValid) {
-    // If not enforcing app id validity, begin enrolling right away.
-    this.helper_.doEnroll(this.encodedEnrollChallenges_,
-        this.encodedSignChallenges_);
-  }
-  // Whether or not enforcing app id validity, begin fetching/checking the
-  // app ids.
+  // Begin fetching/checking the app ids.
   var enrollAppIds = [];
   for (var i = 0; i < enrollChallenges.length; i++) {
     enrollAppIds.push(enrollChallenges[i]['appId']);
   }
   var self = this;
   this.checkAppIds_(enrollAppIds, signChallenges, function(result) {
-    if (!enforceAppIdValid) {
-      // Nothing to do, move along.
-      return;
-    }
     if (result) {
       self.helper_.doEnroll(self.encodedEnrollChallenges_,
           self.encodedSignChallenges_);

chrome/browser/resources/cryptotoken/gnubbies.js

@@ -189,7 +189,6 @@ Gnubbies.prototype.inactivityTimeout_ = function() {
       console.warn(namespace + ' device ' + deviceId +
           ' still open after inactivity, closing');
       this.openDevs_[namespace][deviceId].destroy();
-      this.removeOpenDevice({namespace: namespace, device: deviceId});
     }
   }
 };
@@ -210,7 +209,7 @@ Gnubbies.prototype.addClient = function(which, who, cb) {
     if (gnubby.closing) {
       // Device is closing or already closed.
       self.removeClient(gnubby, who);
-      if (cb) { cb(-llGnubby.GONE); }
+      if (cb) { cb(-llGnubby.NODEVICE); }
     } else {
       gnubby.registerClient(who);
       if (cb) { cb(-llGnubby.OK, gnubby); }
@@ -275,7 +274,7 @@ Gnubbies.prototype.addClient = function(which, who, cb) {
     if (!this.pendingOpens_.hasOwnProperty(which.namespace)) {
       this.pendingOpens_[which.namespace] = {};
     }
-    if (this.pendingOpens_[which.namespace].hasOwnProperty(which)) {
+    if (this.pendingOpens_[which.namespace].hasOwnProperty(which.device)) {
       this.pendingOpens_[which.namespace][which.device].push(opener);
     } else {
       this.pendingOpens_[which.namespace][which.device] = [opener];

chrome/browser/resources/cryptotoken/gnubby-u2f.js

@@ -7,7 +7,6 @@
  */
 'use strict';
 
-// Commands and flags of the Gnubby applet at
 /** Enroll */
 usbGnubby.U2F_ENROLL = 0x01;
 /** Request signature */
@@ -28,6 +27,12 @@ usbGnubby.P1_TUP_TESTONLY = 0x04;
 /** Attest with device key */
 usbGnubby.P1_INDIVIDUAL_KEY = 0x80;
 
+// Version values
+/** V1 of the applet. */
+usbGnubby.U2F_V1 = 'U2F_V1';
+/** V2 of the applet. */
+usbGnubby.U2F_V2 = 'U2F_V2';
+
 /** Perform enrollment
  * @param {ArrayBuffer|Uint8Array} challenge Enrollment challenge
  * @param {ArrayBuffer|Uint8Array} appIdHash Hashed application id
@@ -63,33 +68,51 @@ usbGnubby.prototype.enroll = function(challenge, appIdHash, cb) {
  */
 usbGnubby.prototype.sign = function(challengeHash, appIdHash, keyHandle, cb,
                                     opt_nowink) {
-  var apdu = new Uint8Array(
-      [0x00,
-       usbGnubby.U2F_SIGN,
-       usbGnubby.P1_TUP_REQUIRED | usbGnubby.P1_TUP_CONSUME,
-       0x00, 0x00, 0x00,
-      challengeHash.length + appIdHash.length + keyHandle.length]);
-  if (opt_nowink) {
-    // A signature request that does not want winking.
-    // These are used during enroll to figure out whether a gnubby was already
-    // enrolled.
-    // Tell applet to not actually produce a signature, even
-    // if already touched.
-    apdu[2] |= usbGnubby.P1_TUP_TESTONLY;
-  }
-  var u8 = new Uint8Array(apdu.length + challengeHash.length +
-      appIdHash.length + keyHandle.length + 2);
-  for (var i = 0; i < apdu.length; ++i) u8[i] = apdu[i];
-  for (var i = 0; i < challengeHash.length; ++i) u8[i + apdu.length] =
-    challengeHash[i];
-  for (var i = 0; i < appIdHash.length; ++i) {
-    u8[i + apdu.length + challengeHash.length] = appIdHash[i];
-  }
-  for (var i = 0; i < keyHandle.length; ++i) {
-    u8[i + apdu.length + challengeHash.length + appIdHash.length] =
-        keyHandle[i];
-  }
-  this.apduReply_(u8.buffer, cb, opt_nowink);
+  var self = this;
+  // The sign command's format is ever-so-slightly different between V1 and V2,
+  // so get this gnubby's version prior to sending it.
+  this.version(function(rc, opt_data) {
+    if (rc) {
+      cb(rc);
+      return;
+    }
+    var version = UTIL_BytesToString(new Uint8Array(opt_data || []));
+    var apduDataLen =
+      challengeHash.length + appIdHash.length + keyHandle.length;
+    if (version != usbGnubby.U2F_V1) {
+      // The V2 sign command includes a length byte for the key handle.
+      apduDataLen++;
+    }
+    var apdu = new Uint8Array(
+        [0x00,
+         usbGnubby.U2F_SIGN,
+         usbGnubby.P1_TUP_REQUIRED | usbGnubby.P1_TUP_CONSUME,
+         0x00, 0x00, 0x00,
+         apduDataLen]);
+    if (opt_nowink) {
+      // A signature request that does not want winking.
+      // These are used during enroll to figure out whether a gnubby was already
+      // enrolled.
+      // Tell applet to not actually produce a signature, even
+      // if already touched.
+      apdu[2] |= usbGnubby.P1_TUP_TESTONLY;
+    }
+    var u8 = new Uint8Array(apdu.length + apduDataLen + 2);
+    for (var i = 0; i < apdu.length; ++i) u8[i] = apdu[i];
+    for (var i = 0; i < challengeHash.length; ++i) u8[i + apdu.length] =
+      challengeHash[i];
+    for (var i = 0; i < appIdHash.length; ++i) {
+      u8[i + apdu.length + challengeHash.length] = appIdHash[i];
+    }
+    var keyHandleOffset = apdu.length + challengeHash.length + appIdHash.length;
+    if (version != usbGnubby.U2F_V1) {
+      u8[keyHandleOffset++] = keyHandle.length;
+    }
+    for (var i = 0; i < keyHandle.length; ++i) {
+      u8[i + keyHandleOffset] = keyHandle[i];
+    }
+    self.apduReply_(u8.buffer, cb, opt_nowink);
+  });
 };
 
 /** Request version information
@@ -97,14 +120,23 @@ usbGnubby.prototype.sign = function(challengeHash, appIdHash, keyHandle, cb,
  */
 usbGnubby.prototype.version = function(cb) {
   if (!cb) cb = usbGnubby.defaultCallback;
+  if (this.version_) {
+    cb(-llGnubby.OK, this.version_);
+    return;
+  }
+  var self = this;
   var apdu = new Uint8Array([0x00, usbGnubby.U2F_VERSION, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00]);
   this.apduReply_(apdu.buffer, function(rc, data) {
     if (rc == 0x6d00) {
       // Command not implemented. Pretend this is v1.
-      var v1 = new Uint8Array(UTIL_StringToBytes('U2F_V1'));
+      var v1 = new Uint8Array(UTIL_StringToBytes(usbGnubby.U2F_V1));
+      self.version_ = v1.buffer;
       cb(-llGnubby.OK, v1.buffer);
     } else {
+      if (!rc) {
+        self.version_ = data;
+      }
       cb(rc, data);
     }
   });

chrome/browser/resources/cryptotoken/gnubby.js

@@ -44,7 +44,7 @@ usbGnubby.setGnubbies = function(gnubbies) {
 usbGnubby.prototype.enumerate = function(cb) {
   if (!cb) cb = usbGnubby.defaultCallback;
   if (this.closed) {
-    cb(-llGnubby.GONE);
+    cb(-llGnubby.NODEVICE);
     return;
   }
   if (!usbGnubby.gnubbies_) {
@@ -58,14 +58,15 @@ usbGnubby.prototype.enumerate = function(cb) {
 /**
  * Opens the gnubby with the given index, or the first found gnubby if no
  * index is specified.
- * @param {llGnubbyDeviceId|undefined} opt_which The device to open.
+ * @param {llGnubbyDeviceId} which The device to open. If null, the first
+ *     gnubby found is opened.
  * @param {function(number)|undefined} opt_cb Called with result of opening the
  *     gnubby.
  */
-usbGnubby.prototype.open = function(opt_which, opt_cb) {
+usbGnubby.prototype.open = function(which, opt_cb) {
   var cb = opt_cb ? opt_cb : usbGnubby.defaultCallback;
   if (this.closed) {
-    cb(-llGnubby.GONE);
+    cb(-llGnubby.NODEVICE);
     return;
   }
   this.closingWhenIdle = false;
@@ -78,30 +79,42 @@ usbGnubby.prototype.open = function(opt_which, opt_cb) {
   }
 
   var self = this;
-  function addSelfAsClient(which) {
+
+  function setCid(which) {
     self.cid &= 0x00ffffff;
     self.cid |= ((which.device + 1) << 24);  // For debugging.
+  }
 
+  var enumerateRetriesRemaining = 3;
+  function enumerated(rc, devs) {
+    if (!devs.length)
+      rc = -llGnubby.NODEVICE;
+    if (rc) {
+      cb(rc);
+      return;
+    }
+    which = devs[0];
+    setCid(which);
     usbGnubby.gnubbies_.addClient(which, self, function(rc, device) {
+      if (rc == -llGnubby.NODEVICE && enumerateRetriesRemaining-- > 0) {
+        // We were trying to open the first device, but now it's not there?
+        // Do over.
+        usbGnubby.gnubbies_.enumerate(enumerated);
+        return;
+      }
       self.dev = device;
       cb(rc);
     });
   }
 
-  if (!usbGnubby.gnubbies_) {
-    cb(-llGnubby.NODEVICE);
-    return;
-  }
-  if (opt_which) {
-    addSelfAsClient(opt_which);
-  } else {
-    usbGnubby.gnubbies_.enumerate(function(rc, devs) {
-      if (rc || !devs.length) {
-        cb(-llGnubby.NODEVICE);
-        return;
-      }
-      addSelfAsClient(devs[0]);
+  if (which) {
+    setCid(which);
+    usbGnubby.gnubbies_.addClient(which, self, function(rc, device) {
+      self.dev = device;
+      cb(rc);
     });
+  } else {
+    usbGnubby.gnubbies_.enumerate(enumerated);
   }
 };
 
@@ -217,7 +230,7 @@ usbGnubby.prototype.readFrame_ = function() {
  */
 usbGnubby.prototype.read_ = function(cmd, timeout, cb) {
   if (this.closed) { cb(-llGnubby.GONE); return; }
-  if (!this.dev) { cb(-llGnubby.NODEVICE); return; }
+  if (!this.dev) { cb(-llGnubby.GONE); return; }
 
   var tid = null;  // timeout timer id.
   var callback = cb;
@@ -436,7 +449,7 @@ usbGnubby.prototype.exchange_ = function(cmd, data, timeout, cb) {
 
 /** Default callback for commands. Simply logs to console.
  * @param {number} rc Result status code
- * @param {*} data Result data
+ * @param {(ArrayBuffer|Uint8Array|Array.<number>|null)} data Result data
  */
 usbGnubby.defaultCallback = function(rc, data) {
   var msg = 'defaultCallback(' + rc;

chrome/browser/resources/cryptotoken/llgnubby.js

@@ -105,6 +105,6 @@ llGnubby.prototype.hasClient = function(who) {};
  * If queue was empty, initiate the write.
  * @param {number} cid The client's channel ID.
  * @param {number} cmd The command to send.
- * @param {ArrayBuffer} data Command data
+ * @param {ArrayBuffer|Uint8Array} data Command data
  */
 llGnubby.prototype.queueCommand = function(cid, cmd, data) {};

chrome/browser/resources/cryptotoken/llhidgnubby.js

@@ -40,6 +40,8 @@ llHidGnubby.NAMESPACE = 'hid';
 llHidGnubby.prototype.destroy = function() {
   if (!this.dev) return;  // Already dead.
 
+  this.gnubbies_.removeOpenDevice(
+      {namespace: llHidGnubby.NAMESPACE, device: this.id});
   this.closing = true;
 
   console.log(UTIL_fmt('llHidGnubby.destroy()'));
@@ -68,15 +70,9 @@ llHidGnubby.prototype.destroy = function() {
   var dev = this.dev;
   this.dev = null;
 
-  var self = this;
-
-  function onClosed() {
+  chrome.hid.disconnect(dev.connectionId, function() {
     console.log(UTIL_fmt('Device ' + dev.handle + ' closed'));
-    self.gnubbies_.removeOpenDevice(
-        {namespace: llHidGnubby.NAMESPACE, device: self.id});
-  }
-
-  chrome.hid.disconnect(dev.connectionId, onClosed);
+  });
 };
 
 /**
@@ -103,17 +99,6 @@ llHidGnubby.prototype.publishFrame_ = function(f) {
   if (changes) this.clients = remaining;
 };
 
-/**
- * @return {boolean} whether this device is open and ready to use.
- * @private
- */
-llHidGnubby.prototype.readyToUse_ = function() {
-  if (this.closing) return false;
-  if (!this.dev) return false;
-
-  return true;
-};
-
 /**
  * Register a client for this gnubby.
  * @param {*} who The client.
@@ -299,7 +284,7 @@ llHidGnubby.prototype.updateLock_ = function(cid, cmd, arg) {
  * If queue was empty, initiate the write.
  * @param {number} cid The client's channel ID.
  * @param {number} cmd The command to send.
- * @param {ArrayBuffer} data Command arguments
+ * @param {ArrayBuffer|Uint8Array} data Command arguments
  */
 llHidGnubby.prototype.queueCommand = function(cid, cmd, data) {
   if (!this.dev) return;

chrome/browser/resources/cryptotoken/llusbgnubby.js

@@ -46,6 +46,8 @@ llUsbGnubby.NAMESPACE = 'usb';
 llUsbGnubby.prototype.destroy = function() {
   if (!this.dev) return;  // Already dead.
 
+  this.gnubbies_.removeOpenDevice(
+      {namespace: llUsbGnubby.NAMESPACE, device: this.id});
   this.closing = true;
 
   console.log(UTIL_fmt('llUsbGnubby.destroy()'));
@@ -74,18 +76,11 @@ llUsbGnubby.prototype.destroy = function() {
   var dev = this.dev;
   this.dev = null;
 
-  var self = this;
-
-  function onClosed() {
-    console.log(UTIL_fmt('Device ' + dev.handle + ' closed'));
-    self.gnubbies_.removeOpenDevice(
-        {namespace: llUsbGnubby.NAMESPACE, device: self.id});
-  }
-
-  // Release first.
   chrome.usb.releaseInterface(dev, 0, function() {
     console.log(UTIL_fmt('Device ' + dev.handle + ' released'));
-    chrome.usb.closeDevice(dev, onClosed);
+    chrome.usb.closeDevice(dev, function() {
+      console.log(UTIL_fmt('Device ' + dev.handle + ' closed'));
+    });
   });
 };
 
@@ -343,7 +338,7 @@ llUsbGnubby.prototype.updateLock_ = function(cid, cmd, arg) {
  * If queue was empty, initiate the write.
  * @param {number} cid The client's channel ID.
  * @param {number} cmd The command to send.
- * @param {ArrayBuffer} data Command argument data
+ * @param {ArrayBuffer|Uint8Array} data Command argument data
  */
 llUsbGnubby.prototype.queueCommand = function(cid, cmd, data) {
   if (!this.dev) return;

chrome/browser/resources/cryptotoken/signhelper.js

@@ -37,12 +37,10 @@ function SignHelperFactory() {}
  *     interested in the result of a sign request.
  * @param {function(number, boolean)} errorCb Called when a sign request fails
  *     with an error code and whether any gnubbies were found.
- * @param {function(SignHelperChallenge, string)} successCb Called with the
- *     signature produced by a successful sign request.
- * @param {(function(number, boolean)|undefined)} opt_progressCb Called with
- *     progress updates to the sign request.
+ * @param {function(SignHelperChallenge, string, string=)} successCb Called with
+ *     the signature produced by a successful sign request.
  * @param {string=} opt_logMsgUrl A URL to post log messages to.
  * @return {SignHelper} The newly created helper.
  */
 SignHelperFactory.prototype.createHelper =
-    function(timer, errorCb, successCb, opt_progressCb, opt_logMsgUrl) {};
+    function(timer, errorCb, successCb, opt_logMsgUrl) {};

chrome/browser/resources/cryptotoken/usbsignhelper.js

@@ -15,23 +15,20 @@ var CORRUPT_sign = false;
  *     interested in the result of a sign request.
  * @param {function(number, boolean)} errorCb Called when a sign request fails
  *     with an error code and whether any gnubbies were found.
- * @param {function(SignHelperChallenge, string)} successCb Called with the
- *     signature produced by a successful sign request.
- * @param {(function(number, boolean)|undefined)} opt_progressCb Called with
- *     progress updates to the sign request.
+ * @param {function(SignHelperChallenge, string, string=)} successCb Called with
+ *     the signature produced by a successful sign request.
  * @param {string=} opt_logMsgUrl A URL to post log messages to.
  * @constructor
  * @implements {SignHelper}
  */
-function UsbSignHelper(factory, timer, errorCb, successCb, opt_progressCb,
-    opt_logMsgUrl) {
+function UsbSignHelper(factory, timer, errorCb, successCb, opt_logMsgUrl) {
   /** @private {!GnubbyFactory} */
   this.factory_ = factory;
   /** @private {Countdown} */
   this.timer_ = timer;
   /** @private {function(number, boolean)} */
   this.errorCb_ = errorCb;
-  /** @private {function(SignHelperChallenge, string)} */
+  /** @private {function(SignHelperChallenge, string, string=)} */
   this.successCb_ = successCb;
   /** @private {string|undefined} */
   this.logMsgUrl_ = opt_logMsgUrl;
@@ -189,7 +186,8 @@ UsbSignHelper.prototype.notifySuccess_ = function(gnubby, challenge, info) {
   encodedChallenge['appIdHash'] = B64_encode(challenge['appIdHash']);
   encodedChallenge['keyHandle'] = B64_encode(challenge['keyHandle']);
   this.successCb_(
-      /** @type {SignHelperChallenge} */ (encodedChallenge), B64_encode(info));
+      /** @type {SignHelperChallenge} */ (encodedChallenge), B64_encode(info),
+      'USB');
 };
 
 /**
@@ -329,15 +327,13 @@ function UsbSignHelperFactory(gnubbyFactory) {
  *     with an error code and whether any gnubbies were found.
  * @param {function(SignHelperChallenge, string)} successCb Called with the
  *     signature produced by a successful sign request.
- * @param {(function(number, boolean)|undefined)} opt_progressCb Called with
- *     progress updates to the sign request.
  * @param {string=} opt_logMsgUrl A URL to post log messages to.
  * @return {UsbSignHelper} the newly created helper.
  */
 UsbSignHelperFactory.prototype.createHelper =
-    function(timer, errorCb, successCb, opt_progressCb, opt_logMsgUrl) {
+    function(timer, errorCb, successCb, opt_logMsgUrl) {
   var helper =
       new UsbSignHelper(this.gnubbyFactory_, timer, errorCb, successCb,
-          opt_progressCb, opt_logMsgUrl);
+          opt_logMsgUrl);
   return helper;
 };