Bug 1335899 - Tolerate token failures in U2F.cpp r=keeler draft
authorJ.C. Jones <jjones@mozilla.com>
Wed, 01 Feb 2017 15:00:34 -0700
changeset 469315 f55918f76117abb0f120b21a742c3705c2640225
parent 468867 1d025ac534a6333a8170a59a95a8a3673d4028ee
child 544156 2cd0380b5ac7da49578561b96f8537b2184b184a
push id43681
push userbmo:jjones@mozilla.com
push dateThu, 02 Feb 2017 01:21:35 +0000
reviewerskeeler
bugs1335899
milestone54.0a1
Bug 1335899 - Tolerate token failures in U2F.cpp r=keeler If there's a second token (say, USB anyone?) that fails early, U2F.cpp's U2FStatus object should not be told to "stop" unless it's actually done. So basically, in the promise failures for U2F::Sign and U2F::Register, don't call Stop - let the stop come implicitly when no tokens respond correctly. This changes U2FStatus to be used the same way WebAuthn does its WebAuthnRequest object, for the same purpose. - Review updates from Keeler; thanks! MozReview-Commit-ID: HaTKopFakDB
dom/u2f/U2F.cpp
dom/u2f/tests/frame_multiple_keys.html
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -715,19 +715,19 @@ U2FRegisterRunnable::Run()
       registerTask->Execute()->Then(mAbstractMainThread, __func__,
         [&status, this] (nsString aResponse) {
           if (!status->IsStopped()) {
             status->Stop(ErrorCode::OK, aResponse);
           }
           status->WaitGroupDone();
         },
         [&status, this] (ErrorCode aErrorCode) {
-          if (!status->IsStopped()) {
-            status->Stop(aErrorCode);
-          }
+          // Ignore the failing error code, as we only want the first success.
+          // U2F devices don't provide much for error codes anyway, so if
+          // they all fail we'll return DEVICE_INELIGIBLE.
           status->WaitGroupDone();
      });
     }
   }
 
   // Wait until the first key is successfuly generated
   status->WaitGroupWait();
 
@@ -901,19 +901,19 @@ U2FSignRunnable::Run()
       signTask->Execute()->Then(mAbstractMainThread, __func__,
         [&status, this] (nsString aResponse) {
           if (!status->IsStopped()) {
             status->Stop(ErrorCode::OK, aResponse);
           }
           status->WaitGroupDone();
         },
         [&status, this] (ErrorCode aErrorCode) {
-          if (!status->IsStopped()) {
-            status->Stop(aErrorCode);
-          }
+          // Ignore the failing error code, as we only want the first success.
+          // U2F devices don't provide much for error codes anyway, so if
+          // they all fail we'll return DEVICE_INELIGIBLE.
           status->WaitGroupDone();
       });
     }
   }
 
   // Wait for the authenticators to finish
   status->WaitGroupWait();
 
--- a/dom/u2f/tests/frame_multiple_keys.html
+++ b/dom/u2f/tests/frame_multiple_keys.html
@@ -47,16 +47,27 @@ window.u2f.register(window.location.orig
   registerSecondKey();
 });
 
 // Get the second key...
 // It's OK to repeat the regRequest; not material for this test
 function registerSecondKey() {
   window.u2f.register(window.location.origin, [regRequest], [], function(aRegResponse) {
     testState.key2 = keyHandleFromRegResponse(aRegResponse);
+
+    registerWithInvalidAndValidKey();
+  });
+}
+
+function registerWithInvalidAndValidKey() {
+  window.u2f.register(window.location.origin, [regRequest],
+                      [invalidKey, testState.key1], function(aRegResponse) {
+    // Expect a failure response since key1 is already registered
+    local_is(aRegResponse.errorCode, 4, "The register should have skipped since there was a valid key");
+
     testSignSingleKey();
   });
 }
 
 // It should also work with just one key
 function testSignSingleKey() {
   window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
                   [testState.key1], function(aSignResponse) {
@@ -69,15 +80,42 @@ function testSignSingleKey() {
 
 function testSignDual() {
   // It's OK to sign with either one
   window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
                   [testState.key1, testState.key2], function(aSignResponse) {
     local_is(aSignResponse.errorCode, 0, "The signing did not error with two keys");
     local_isnot(aSignResponse.clientData, undefined, "The signing provided clientData with two keys");
 
+    testSignWithInvalidKey();
+  });
+}
+
+// Just a key that came from a random profile; syntactically valid but not
+// unwrappable.
+var invalidKey = {
+  "version": "U2F_V2",
+  "keyHandle": "rQdreHgHrmKfsnGPAElEP9yfTx6eq2eU3_Y8n0RRsGKML0DY2d1_a8_-sOtxDr3"
+};
+
+function testSignWithInvalidKey() {
+  window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
+                  [invalidKey, testState.key2], function(aSignResponse) {
+    local_is(aSignResponse.errorCode, 0, "The signing did not error when given an invalid key");
+    local_isnot(aSignResponse.clientData, undefined, "The signing provided clientData even when given an invalid key");
+
+    testSignWithInvalidKeyReverse();
+  });
+}
+
+function testSignWithInvalidKeyReverse() {
+  window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
+                  [testState.key2, invalidKey], function(aSignResponse) {
+    local_is(aSignResponse.errorCode, 0, "The signing did not error when given an invalid key");
+    local_isnot(aSignResponse.clientData, undefined, "The signing provided clientData even when given an invalid key");
+
     local_completeTest();
   });
 }
 </script>
 
 </body>
 </html>
\ No newline at end of file