Bug 1460767 - Return device ineligible when appropriate for U2F r=ttaubert
authorJ.C. Jones <jjones@mozilla.com>
Thu, 10 May 2018 16:36:18 -0700
changeset 418184 5166f4f5af706b3c37982ac1e94498d979b8198d
parent 418183 436f28e55ce42f53ac4b9e91b6635426d1f320b4
child 418185 5d368658145949915eaaa095275cc20a6fa76748
push id33995
push usernbeleuzu@mozilla.com
push dateMon, 14 May 2018 21:37:08 +0000
treeherdermozilla-central@a382f8feaba4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert
bugs1460767
milestone62.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 1460767 - Return device ineligible when appropriate for U2F r=ttaubert Summary: FIDO U2F's specification says that when the wrong security key responds to a signature, or when an already-registered key exists, that the UA should return error code 4, DEVICE_INELIGIBLE. We used to do that, but adjusted some things for WebAuthn and now we don't. This changes the soft token to return that at the appropriate times, and updates the expectations of U2F.cpp that it should use InvalidStateError as the signal to reutrn DEVICE_INELIGIBLE. Also, note that WebAuthn's specification says that if any authenticator returns "InvalidStateError" that it should be propagated, as it indicates that the authenticator obtained user consent and failed to complete its job [1]. This change to the Soft Token affects the WebAuthn tests, but in a good way. Reading the WebAuthn spec, we should not be returning NotAllowedError when there is consent from the user via the token (which the softtoken always deliveres). As such, this adjusts the affected WebAuthn tests, and adds a couple useful checks to test_webauthn_get_assertion.html for future purposes. [1] https://w3c.github.io/webauthn/#createCredential section 5.1.3 "Create a new credential", Step 20, Note 2: "If any authenticator returns an error status equivalent to "InvalidStateError"..." Test Plan: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2fc930f7fc8eea69b1ebc96748fe95e150a92a4 Reviewers: ttaubert Bug #: 1460767 Differential Revision: https://phabricator.services.mozilla.com/D1269
dom/u2f/U2F.cpp
dom/webauthn/U2FSoftTokenManager.cpp
dom/webauthn/tests/browser/browser_fido_appid_extension.js
dom/webauthn/tests/test_webauthn_authenticator_transports.html
dom/webauthn/tests/test_webauthn_get_assertion.html
dom/webauthn/tests/test_webauthn_loopback.html
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -59,17 +59,17 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(U2
 
 static ErrorCode
 ConvertNSResultToErrorCode(const nsresult& aError)
 {
   if (aError == NS_ERROR_DOM_TIMEOUT_ERR) {
     return ErrorCode::TIMEOUT;
   }
   /* Emitted by U2F{Soft,HID}TokenManager when we really mean ineligible */
-  if (aError == NS_ERROR_DOM_NOT_ALLOWED_ERR) {
+  if (aError == NS_ERROR_DOM_INVALID_STATE_ERR) {
     return ErrorCode::DEVICE_INELIGIBLE;
   }
   return ErrorCode::OTHER_ERROR;
 }
 
 static uint32_t
 AdjustedTimeoutMillis(const Optional<Nullable<int32_t>>& opt_aSeconds)
 {
--- a/dom/webauthn/U2FSoftTokenManager.cpp
+++ b/dom/webauthn/U2FSoftTokenManager.cpp
@@ -605,17 +605,17 @@ U2FSoftTokenManager::Register(const WebA
   // Optional exclusion list.
   for (const WebAuthnScopedCredential& cred: aInfo.ExcludeList()) {
     bool isRegistered = false;
     nsresult rv = IsRegistered(cred.id(), aInfo.RpIdHash(), isRegistered);
     if (NS_FAILED(rv)) {
       return U2FRegisterPromise::CreateAndReject(rv, __func__);
     }
     if (isRegistered) {
-      return U2FRegisterPromise::CreateAndReject(NS_ERROR_DOM_NOT_ALLOWED_ERR, __func__);
+      return U2FRegisterPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR, __func__);
     }
   }
 
   // We should already have a wrapping key
   MOZ_ASSERT(mWrappingKey);
 
   UniquePK11SlotInfo slot(PK11_GetInternalSlot());
   MOZ_ASSERT(slot.get());
@@ -757,17 +757,17 @@ U2FSoftTokenManager::Sign(const WebAuthn
     }
   }
 
   nsTArray<uint8_t> chosenAppId;
   nsTArray<uint8_t> keyHandle;
 
   // Fail if we can't find a valid key handle.
   if (!FindRegisteredKeyHandle(appIds, aInfo.AllowList(), keyHandle, chosenAppId)) {
-    return U2FSignPromise::CreateAndReject(NS_ERROR_DOM_NOT_ALLOWED_ERR, __func__);
+    return U2FSignPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR, __func__);
   }
 
   MOZ_ASSERT(mWrappingKey);
 
   UniquePK11SlotInfo slot(PK11_GetInternalSlot());
   MOZ_ASSERT(slot.get());
 
   if (NS_WARN_IF((aInfo.ClientDataHash().Length() != kParamLen) ||
--- a/dom/webauthn/tests/browser/browser_fido_appid_extension.js
+++ b/dom/webauthn/tests/browser/browser_fido_appid_extension.js
@@ -13,17 +13,17 @@ function arrivingHereIsBad(aResult) {
 function expectError(aType) {
   let expected = `${aType}Error`;
   return function (aResult) {
     is(aResult.slice(0, expected.length), expected, `Expecting a ${aType}Error`);
   };
 }
 
 let expectNotSupportedError = expectError("NotSupported");
-let expectNotAllowedError = expectError("NotAllowed");
+let expectInvalidStateError = expectError("InvalidState");
 let expectSecurityError = expectError("Security");
 
 function promiseU2FRegister(tab, app_id) {
   let challenge = crypto.getRandomValues(new Uint8Array(16));
   challenge = bytesToBase64UrlSafe(challenge);
 
   return ContentTask.spawn(tab.linkedBrowser, [app_id, challenge], function ([app_id, challenge]) {
     return new Promise(resolve => {
@@ -106,29 +106,32 @@ add_task(async function test_appid() {
   let keyHandle = await promiseU2FRegister(tab, appid);
 
   // The FIDO AppId extension can't be used for MakeCredential.
   await promiseWebAuthnRegister(tab, appid)
     .then(arrivingHereIsBad)
     .catch(expectNotSupportedError);
 
   // Using the keyHandle shouldn't work without the FIDO AppId extension.
+  // This will be an invalid state, because the softtoken will consent without
+  // having the correct "RP ID" via the FIDO extension.
   await promiseWebAuthnSign(tab, keyHandle)
     .then(arrivingHereIsBad)
-    .catch(expectNotAllowedError);
+    .catch(expectInvalidStateError);
 
   // Invalid app IDs (for the current origin) must be rejected.
   await promiseWebAuthnSign(tab, keyHandle, {appid: "https://bogus.com/appId"})
     .then(arrivingHereIsBad)
     .catch(expectSecurityError);
 
-  // Non-matching app IDs must be rejected.
+  // Non-matching app IDs must be rejected. Even when the user/softtoken
+  // consents, leading to an invalid state.
   await promiseWebAuthnSign(tab, keyHandle, {appid: appid + "2"})
     .then(arrivingHereIsBad)
-    .catch(expectNotAllowedError);
+    .catch(expectInvalidStateError);
 
   let rpId = new TextEncoder("utf-8").encode(appid);
   let rpIdHash = await crypto.subtle.digest("SHA-256", rpId);
 
   // Succeed with the right fallback rpId.
   await promiseWebAuthnSign(tab, keyHandle, {appid})
     .then(({authenticatorData, clientDataJSON, extensions}) => {
       is(extensions.appid, true, "appid extension was acted upon");
--- a/dom/webauthn/tests/test_webauthn_authenticator_transports.html
+++ b/dom/webauthn/tests/test_webauthn_authenticator_transports.html
@@ -22,16 +22,20 @@
     function arrivingHereIsBad(aResult) {
       ok(false, "Bad result! Received a: " + aResult);
     }
 
     function expectNotAllowedError(aResult) {
       ok(aResult.toString().startsWith("NotAllowedError"), "Expecting a NotAllowedError, got " + aResult);
     }
 
+    function expectInvalidStateError(aResult) {
+      ok(aResult.toString().startsWith("InvalidStateError"), "Expecting a InvalidStateError, got " + aResult);
+    }
+
     // Store the credential of the first successful make credential
     // operation so we can use it to get assertions later.
     let gCredential;
 
     add_task(() => {
       // Enable the softtoken.
       return SpecialPowers.pushPrefEnv({"set": [
         ["security.webauth.webauthn", true],
@@ -79,58 +83,64 @@
       await requestMakeCredential([{
         type: "public-key",
         id: crypto.getRandomValues(new Uint8Array(16)),
         transports: ["usb"],
       }]).then(arrivingHereIsGood)
          .catch(arrivingHereIsBad);
 
       // Pass gCredential with transport=usb.
+      // The credential already exists, and the softoken consents to create,
+      // so the error is InvalidState and not NotAllowed.
       await requestMakeCredential([{
         type: "public-key",
         id: gCredential,
         transports: ["usb"],
       }]).then(arrivingHereIsBad)
-         .catch(expectNotAllowedError);
+         .catch(expectInvalidStateError);
 
       // Pass gCredential with transport=nfc.
       // The softoken pretends to support all transports.
+      // Also, as above, the credential exists and the token indicates consent.
       await requestMakeCredential([{
         type: "public-key",
         id: gCredential,
         transports: ["nfc"],
       }]).then(arrivingHereIsBad)
-         .catch(expectNotAllowedError);
+         .catch(expectInvalidStateError);
 
       // Pass gCredential with an empty transports list.
+      // As above, the token indicates consent, so expect InvalidStateError.
       await requestMakeCredential([{
         type: "public-key",
         id: gCredential,
         transports: [],
       }]).then(arrivingHereIsBad)
-         .catch(expectNotAllowedError);
+         .catch(expectInvalidStateError);
     });
 
     // Test get assertion behavior.
     add_task(async () => {
       // Request an assertion for gCredential.
       await requestGetAssertion([{
         type: "public-key",
         id: gCredential,
         transports: ["usb"],
       }]).then(arrivingHereIsGood)
          .catch(arrivingHereIsBad);
 
-      // Request an assertion for a random credential.
+      // Request an assertion for a random credential. The token indicates
+      // consent even though this credential doesn't exist, so expect an
+      // InvalidStateError.
       await requestGetAssertion([{
         type: "public-key",
         id: crypto.getRandomValues(new Uint8Array(16)),
         transports: ["usb"],
       }]).then(arrivingHereIsBad)
-         .catch(expectNotAllowedError);
+         .catch(expectInvalidStateError);
 
       // Request an assertion for gCredential with transport=nfc.
       // The softoken pretends to support all transports.
       await requestGetAssertion([{
         type: "public-key",
         id: gCredential,
         transports: ["nfc"],
       }]).then(arrivingHereIsGood)
--- a/dom/webauthn/tests/test_webauthn_get_assertion.html
+++ b/dom/webauthn/tests/test_webauthn_get_assertion.html
@@ -24,68 +24,117 @@
     isnot(navigator.credentials.create, undefined, "CredentialManagement create API endpoint must exist");
     isnot(navigator.credentials.get, undefined, "CredentialManagement get API endpoint must exist");
 
     let gAssertionChallenge = new Uint8Array(16);
     window.crypto.getRandomValues(gAssertionChallenge);
 
     let invalidCred = {type: "Magic", id: base64ToBytes("AAA=")};
     let unknownCred = {type: "public-key", id: base64ToBytes("AAA=")};
+    let validCred = null;
 
     function requestGetAssertion(params) {
       return navigator.credentials.get(params);
     }
 
+    function arrivingHereIsGood(aResult) {
+      ok(true, "Good result! Received a: " + aResult);
+    }
+
     function arrivingHereIsBad(aResult) {
       ok(false, "Bad result! Received a: " + aResult);
     }
 
     function expectNotAllowedError(aResult) {
-      ok(aResult.toString().startsWith("NotAllowedError"), "Expecting a NotAllowedError");
+      ok(aResult.toString().startsWith("NotAllowedError"), "Expecting a NotAllowedError, got " + aResult);
+    }
+
+    function expectInvalidStateError(aResult) {
+      ok(aResult.toString().startsWith("InvalidStateError"), "Expecting a InvalidStateError, got " + aResult);
     }
 
     function expectTypeError(aResult) {
-      ok(aResult.toString().startsWith("TypeError"), "Expecting a TypeError");
+      ok(aResult.toString().startsWith("TypeError"), "Expecting a TypeError, got " + aResult);
     }
 
     function expectAbortError(aResult) {
       is(aResult.code, DOMException.ABORT_ERR, "Expecting an AbortError");
     }
 
     add_task(() => {
       return SpecialPowers.pushPrefEnv({"set": [
         ["security.webauth.webauthn", true],
         ["security.webauth.webauthn_enable_softtoken", true],
         ["security.webauth.webauthn_enable_usbtoken", false]
       ]});
     });
 
+    // Set up a valid credential
+    add_task(async () => {
+      let publicKey = {
+        rp: {id: document.domain, name: "none", icon: "none"},
+        user: {id: new Uint8Array(), name: "none", icon: "none", displayName: "none"},
+        challenge: crypto.getRandomValues(new Uint8Array(16)),
+        pubKeyCredParams: [{type: "public-key", alg: cose_alg_ECDSA_w_SHA256}],
+      };
+
+      return navigator.credentials.create({publicKey})
+      .then(res => validCred = {type: "public-key", id: res.rawId} );
+    });
+
     // Test basic good call, but without giving a credential so expect failures
     // this is OK by the standard, but not supported by U2F-backed authenticators
     // like the soft token in use here.
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge
       };
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
     });
 
-    // Test with an unexpected option
+    // Test with a valid credential
+    add_task(async () => {
+      let publicKey = {
+        challenge: gAssertionChallenge,
+        allowCredentials: [validCred]
+      };
+
+      await requestGetAssertion({publicKey})
+        .then(arrivingHereIsGood)
+        .catch(arrivingHereIsBad);
+    });
+
+    // Test with an unexpected option. That won't stop anything, and we'll
+    // fail with InvalidState just as if we had no valid credentials -- which
+    // we don't.
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge,
         unknownValue: "hi"
       };
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
+    });
+
+    // Test with an unexpected option but a valid credential
+    add_task(async () => {
+      let publicKey = {
+        challenge: gAssertionChallenge,
+        unknownValue: "hi",
+        allowCredentials: [validCred]
+      };
+
+      await requestGetAssertion({publicKey})
+        .then(arrivingHereIsGood)
+        .catch(arrivingHereIsBad);
     });
 
     // Test with an invalid credential
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge,
         allowCredentials: [invalidCred]
       };
@@ -99,42 +148,44 @@
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge,
         allowCredentials: [unknownCred]
       };
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
     });
 
     // Test with an unexpected option and an invalid credential
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge,
         unknownValue: "hi",
         allowCredentials: [invalidCred]
       };
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
         .catch(expectTypeError);
     });
 
     // Test with an empty credential list
+    // This will return InvalidStateError since the softotken consents, but
+    // there are no valid credentials.
     add_task(async () => {
       let publicKey = {
         challenge: gAssertionChallenge,
         allowCredentials: []
       };
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
     });
 
     add_task(() => {
       // Enable USB tokens.
       return SpecialPowers.pushPrefEnv({"set": [
         ["security.webauth.webauthn", true],
         ["security.webauth.webauthn_enable_softtoken", false],
         ["security.webauth.webauthn_enable_usbtoken", true]
--- a/dom/webauthn/tests/test_webauthn_loopback.html
+++ b/dom/webauthn/tests/test_webauthn_loopback.html
@@ -180,17 +180,17 @@ function() {
     };
     credm.create({publicKey: makeCredentialOptions})
     .then(function() {
       // We should have errored here!
       ok(false, "The excludeList didn't stop a duplicate being created!");
       SimpleTest.finish();
     })
     .catch(function(aReason) {
-      ok(aReason.toString().startsWith("NotAllowedError"), "Expect NotAllowedError, got " + aReason);
+      ok(aReason.toString().startsWith("InvalidStateError"), "Expect InvalidStateError, got " + aReason);
       testAssertion(aCredInfo);
     });
   }
 
   function testAssertion(aCredInfo) {
     let newCredential = {
       type: "public-key",
       id: new Uint8Array(aCredInfo.rawId),