Bug 1333592 - Fix a regression with U2F sign() called with multiple keys r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Thu, 26 Jan 2017 15:18:50 -0700
changeset 380270 456d27bbbe7429efb8c7df47820353af7dabd424
parent 380269 9d7658b29d31cecb1b04383567f42658408a7e2b
child 380271 c17df99bfb44be87842cec08676276b964f1cc09
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
bugs1333592
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 1333592 - Fix a regression with U2F sign() called with multiple keys r=keeler Add a test that U2F supports signing with multiple keys. Reorder the WaitGroupDone calls to ensure they always fire, even if there are multiple actions in flight. Also, change the status lanbda captures to capture by reference, and disable the copy constructor that would let the by-value work. Interestingly, the compiler is choosing by-reference by default -- at least logs show that the behavior is correct without this change, but still - this is the right thing to do. Updated: Fix the unit tests and write a README that explains why they have to use an iframe, while WebAuthn tests do not. MozReview-Commit-ID: AqSyxU5N4yu
dom/u2f/U2F.cpp
dom/u2f/U2F.h
dom/u2f/tests/README.md
dom/u2f/tests/frame_multiple_keys.html
dom/u2f/tests/mochitest.ini
dom/u2f/tests/test_multiple_keys.html
dom/u2f/tests/u2futil.js
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -632,38 +632,38 @@ U2FRegisterRunnable::Run()
       prepPromiseList.AppendElement(compTask->Execute());
     }
 
     // Treat each call to Promise::All as a work unit, as it completes together
     status->WaitGroupAdd();
 
     U2FPrepPromise::All(mAbstractMainThread, prepPromiseList)
     ->Then(mAbstractMainThread, __func__,
-      [status] (const nsTArray<Authenticator>& aTokens) {
+      [&status] (const nsTArray<Authenticator>& aTokens) {
         MOZ_LOG(gU2FLog, LogLevel::Debug,
                 ("ALL: None of the RegisteredKeys were recognized. n=%d",
                  aTokens.Length()));
 
         status->WaitGroupDone();
       },
-      [status] (ErrorCode aErrorCode) {
+      [&status] (ErrorCode aErrorCode) {
         status->Stop(aErrorCode);
         status->WaitGroupDone();
     });
   }
 
   // Wait for all the IsRegistered tasks to complete
   status->WaitGroupWait();
 
   // Check to see whether we're supposed to stop, because one of the keys was
   // recognized.
   if (status->IsStopped()) {
     status->WaitGroupAdd();
     mAbstractMainThread->Dispatch(NS_NewRunnableFunction(
-      [status, this] () {
+      [&status, this] () {
         RegisterResponse response;
         response.mErrorCode.Construct(
             static_cast<uint32_t>(status->GetErrorCode()));
         SendResponse(response);
         status->WaitGroupDone();
       }
     ));
 
@@ -708,45 +708,43 @@ U2FRegisterRunnable::Run()
       RefPtr<U2FRegisterTask> registerTask = new U2FRegisterTask(mOrigin, mAppId,
                                                                  token, appParam,
                                                                  challengeParam,
                                                                  req,
                                                                  mAbstractMainThread);
       status->WaitGroupAdd();
 
       registerTask->Execute()->Then(mAbstractMainThread, __func__,
-        [status, this] (nsString aResponse) {
-          if (status->IsStopped()) {
-            return;
+        [&status, this] (nsString aResponse) {
+          if (!status->IsStopped()) {
+            status->Stop(ErrorCode::OK, aResponse);
           }
-          status->Stop(ErrorCode::OK, aResponse);
           status->WaitGroupDone();
         },
-        [status, this] (ErrorCode aErrorCode) {
-          if (status->IsStopped()) {
-            return;
+        [&status, this] (ErrorCode aErrorCode) {
+          if (!status->IsStopped()) {
+            status->Stop(aErrorCode);
           }
-          status->Stop(aErrorCode);
           status->WaitGroupDone();
      });
     }
   }
 
   // Wait until the first key is successfuly generated
   status->WaitGroupWait();
 
   // If none of the tasks completed, then nothing could satisfy.
   if (!status->IsStopped()) {
     status->Stop(ErrorCode::BAD_REQUEST);
   }
 
   // Transmit back to the JS engine from the Main Thread
   status->WaitGroupAdd();
   mAbstractMainThread->Dispatch(NS_NewRunnableFunction(
-    [status, this] () {
+    [&status, this] () {
       RegisterResponse response;
       if (status->GetErrorCode() == ErrorCode::OK) {
         response.Init(status->GetResponse());
       } else {
         response.mErrorCode.Construct(
             static_cast<uint32_t>(status->GetErrorCode()));
       }
       SendResponse(response);
@@ -896,45 +894,43 @@ U2FSignRunnable::Run()
       RefPtr<U2FSignTask> signTask = new U2FSignTask(mOrigin, mAppId,
                                                      key.mVersion, token,
                                                      appParam, challengeParam,
                                                      mClientData, keyHandle,
                                                      mAbstractMainThread);
       status->WaitGroupAdd();
 
       signTask->Execute()->Then(mAbstractMainThread, __func__,
-        [status, this] (nsString aResponse) {
-          if (status->IsStopped()) {
-            return;
+        [&status, this] (nsString aResponse) {
+          if (!status->IsStopped()) {
+            status->Stop(ErrorCode::OK, aResponse);
           }
-          status->Stop(ErrorCode::OK, aResponse);
           status->WaitGroupDone();
         },
-        [status, this] (ErrorCode aErrorCode) {
-          if (status->IsStopped()) {
-            return;
+        [&status, this] (ErrorCode aErrorCode) {
+          if (!status->IsStopped()) {
+            status->Stop(aErrorCode);
           }
-          status->Stop(aErrorCode);
           status->WaitGroupDone();
       });
     }
   }
 
   // Wait for the authenticators to finish
   status->WaitGroupWait();
 
   // If none of the tasks completed, then nothing could satisfy.
   if (!status->IsStopped()) {
     status->Stop(ErrorCode::DEVICE_INELIGIBLE);
   }
 
   // Transmit back to the JS engine from the Main Thread
   status->WaitGroupAdd();
   mAbstractMainThread->Dispatch(NS_NewRunnableFunction(
-    [status, this] () {
+    [&status, this] () {
       SignResponse response;
       if (status->GetErrorCode() == ErrorCode::OK) {
         response.Init(status->GetResponse());
       } else {
         response.mErrorCode.Construct(
           static_cast<uint32_t>(status->GetErrorCode()));
       }
       SendResponse(response);
--- a/dom/u2f/U2F.h
+++ b/dom/u2f/U2F.h
@@ -165,16 +165,17 @@ private:
 };
 
 // Mediate inter-thread communication for multiple authenticators being queried
 // in concert. Operates as a cyclic buffer with a stop-work method.
 class U2FStatus
 {
 public:
   U2FStatus();
+  U2FStatus(const U2FStatus&) = delete;
 
   void WaitGroupAdd();
   void WaitGroupDone();
   void WaitGroupWait();
 
   void Stop(const ErrorCode aErrorCode);
   void Stop(const ErrorCode aErrorCode, const nsAString& aResponse);
   bool IsStopped();
new file mode 100644
--- /dev/null
+++ b/dom/u2f/tests/README.md
@@ -0,0 +1,8 @@
+Note:
+
+While conceptually similar to the tests for Web Authentication (dom/webauthn),
+the tests for U2F require an iframe while `window.u2f` remains hidden behind a
+preference, though WebAuthn does not. The reason is that the `window` object
+doesn't mutate upon a call by SpecialPowers.setPrefEnv() the way that the
+`navigator` objects do, rather you have to load a different page with a different
+`window` object for the preference change to be honored.
new file mode 100644
--- /dev/null
+++ b/dom/u2f/tests/frame_multiple_keys.html
@@ -0,0 +1,83 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<head>
+  <script type="text/javascript" src="u2futil.js"></script>
+</head>
+<body>
+
+<script class="testbody" type="text/javascript">
+"use strict";
+
+function keyHandleFromRegResponse(aRegResponse) {
+  // Parse the response data from the U2F token
+  var registrationData = base64ToBytesUrlSafe(aRegResponse.registrationData);
+  local_is(registrationData[0], 0x05, "Reserved byte is correct")
+
+  var keyHandleLength = registrationData[66];
+  var keyHandleBytes = registrationData.slice(67, 67 + keyHandleLength);
+
+  return {
+    version: "U2F_V2",
+    keyHandle: bytesToBase64UrlSafe(keyHandleBytes),
+  };
+}
+
+local_setParentOrigin("https://example.com");
+local_expectThisManyTests(1);
+
+// Ensure the SpecialPowers push worked properly
+local_isnot(window.u2f, undefined, "U2F API endpoint must exist");
+
+var challenge = new Uint8Array(16);
+window.crypto.getRandomValues(challenge);
+
+var regRequest = {
+  version: "U2F_V2",
+  challenge: bytesToBase64UrlSafe(challenge),
+};
+
+var testState = {
+  key1: null,
+  key2: null,
+}
+
+// Get two valid keys and present them
+window.u2f.register(window.location.origin, [regRequest], [], function(aRegResponse) {
+  testState.key1 = keyHandleFromRegResponse(aRegResponse);
+  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);
+    testSignSingleKey();
+  });
+}
+
+// It should also work with just one key
+function testSignSingleKey() {
+  window.u2f.sign(window.location.origin, bytesToBase64UrlSafe(challenge),
+                  [testState.key1], function(aSignResponse) {
+    local_is(aSignResponse.errorCode, 0, "The signing did not error with one key");
+    local_isnot(aSignResponse.clientData, undefined, "The signing provided clientData with one key");
+
+    testSignDual();
+  });
+}
+
+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");
+
+    local_completeTest();
+  });
+}
+</script>
+
+</body>
+</html>
\ No newline at end of file
--- a/dom/u2f/tests/mochitest.ini
+++ b/dom/u2f/tests/mochitest.ini
@@ -1,16 +1,17 @@
 [DEFAULT]
 support-files =
   frame_appid_facet.html
   frame_appid_facet_insecure.html
   frame_appid_facet_subdomain.html
   frame_no_token.html
   frame_register.html
   frame_register_sign.html
+  frame_multiple_keys.html
   pkijs/asn1.js
   pkijs/common.js
   pkijs/x509_schema.js
   pkijs/x509_simpl.js
   u2futil.js
 
 # Feature does not function on e10s (Disabled in Bug 1297552)
 [test_util_methods.html]
@@ -22,8 +23,11 @@ skip-if = !e10s
 [test_register_sign.html]
 skip-if = !e10s
 [test_appid_facet.html]
 skip-if = !e10s
 [test_appid_facet_insecure.html]
 skip-if = !e10s
 [test_appid_facet_subdomain.html]
 skip-if = !e10s
+[test_multiple_keys.html]
+skip-if = !e10s
+scheme = https
new file mode 100644
--- /dev/null
+++ b/dom/u2f/tests/test_multiple_keys.html
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<head>
+  <title>Test for Multiple Keys for FIDO U2F</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="u2futil.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+
+<h1>Test Multiple Keys for FIDO U2F</h1>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1333592">Mozilla Bug 1333592</a>
+
+<script class="testbody" type="text/javascript">
+"use strict";
+
+ /** Test for XBL scope behavior. **/
+SimpleTest.waitForExplicitFinish();
+
+// Embed the real test. It will take care of everything else.
+//
+// This is necessary since the U2F object on window is hidden behind a preference
+// and window won't pick up changes by pref without a reload.
+SpecialPowers.pushPrefEnv({"set": [["security.webauth.u2f", true],
+                                   ["security.webauth.u2f_enable_softtoken", true],
+                                   ["security.webauth.u2f_enable_usbtoken", false]]},
+function() {
+  // listen for messages from the test harness
+  window.addEventListener("message", handleEventMessage);
+  document.getElementById('testing_frame').src = "frame_multiple_keys.html";
+});
+</script>
+
+<div id="framediv">
+  <iframe id="testing_frame"></iframe>
+</div>
+
+</body>
+</html>
--- a/dom/u2f/tests/u2futil.js
+++ b/dom/u2f/tests/u2futil.js
@@ -1,11 +1,16 @@
 // Used by local_addTest() / local_completeTest()
 var _countCompletions = 0;
 var _expectedCompletions = 0;
+var _parentOrigin = "http://mochi.test:8888";
+
+function local_setParentOrigin(aOrigin) {
+  _parentOrigin = aOrigin;
+}
 
 function handleEventMessage(event) {
   if ("test" in event.data) {
     let summary = event.data.test + ": " + event.data.msg;
     log(event.data.status + ": " + summary);
     ok(event.data.status, summary);
   } else if ("done" in event.data) {
     SimpleTest.finish();
@@ -35,17 +40,17 @@ function local_isnot(value, expected, me
     local_ok(true, message);
   } else {
     local_ok(false, message + " unexpectedly: " + value + " === " + expected);
   }
 }
 
 function local_ok(expression, message) {
   let body = {"test": this.location.pathname, "status":expression, "msg": message}
-  parent.postMessage(body, "http://mochi.test:8888");
+  parent.postMessage(body, _parentOrigin);
 }
 
 function local_doesThrow(fn, name) {
   let gotException = false;
   try {
     fn();
   } catch (ex) { gotException = true; }
   local_ok(gotException, name);
@@ -65,17 +70,17 @@ function local_completeTest() {
     local_finished();
   }
   if (_countCompletions > _expectedCompletions) {
     local_ok(false, "Error: local_completeTest called more than local_addTest.");
   }
 }
 
 function local_finished() {
-  parent.postMessage({"done":true}, "http://mochi.test:8888");
+  parent.postMessage({"done":true}, _parentOrigin);
 }
 
 function string2buffer(str) {
   return (new Uint8Array(str.length)).map((x, i) => str.charCodeAt(i));
 }
 
 function buffer2string(buf) {
   let str = "";