Bug 1335899 - Tolerate token failures in U2F.cpp r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Wed, 01 Feb 2017 15:00:34 -0700
changeset 381165 f898ad6da18457dae6b4e88b963c7f9312fd4992
parent 381164 268c3d74fba32bd3f4b4d2d3f239771e758ab0b6
child 381166 4057a8f658fc04c77d06fd4b1999285c0c8c18f1
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1335899
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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