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

--HG--
extra : transplant_source : M%5B%93%81%29%7E%B2%E8%24%05%A6%96%8BUN%C9%FB%3E%B3h

dom/u2f/U2F.cpp

@@ -64,7 +64,7 @@ ConvertNSResultToErrorCode(const nsresult& aError)
     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;

dom/webauthn/U2FSoftTokenManager.cpp

@@ -610,7 +610,7 @@ U2FSoftTokenManager::Register(const WebAuthnMakeCredentialInfo& aInfo)
       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__);
     }
   }
 
@@ -762,7 +762,7 @@ U2FSoftTokenManager::Sign(const WebAuthnGetAssertionInfo& aInfo)
 
   // 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);

dom/webauthn/tests/browser/browser_fido_appid_extension.js

@@ -18,7 +18,7 @@ function expectError(aType) {
 }
 
 let expectNotSupportedError = expectError("NotSupported");
-let expectNotAllowedError = expectError("NotAllowed");
+let expectInvalidStateError = expectError("InvalidState");
 let expectSecurityError = expectError("Security");
 
 function promiseU2FRegister(tab, app_id) {
@@ -111,19 +111,22 @@ add_task(async function test_appid() {
     .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);

dom/webauthn/tests/test_webauthn_authenticator_transports.html

@@ -27,6 +27,10 @@
       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;
@@ -84,29 +88,33 @@
          .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.
@@ -119,13 +127,15 @@
       }]).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.

dom/webauthn/tests/test_webauthn_get_assertion.html

@@ -29,21 +29,30 @@
 
     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) {
@@ -58,6 +67,19 @@
       ]});
     });
 
+    // 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.
@@ -68,10 +90,24 @@
 
       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,
@@ -80,7 +116,20 @@
 
       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
@@ -104,7 +153,7 @@
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
     });
 
     // Test with an unexpected option and an invalid credential
@@ -121,6 +170,8 @@
     });
 
     // 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,
@@ -129,7 +180,7 @@
 
       await requestGetAssertion({publicKey})
         .then(arrivingHereIsBad)
-        .catch(expectNotAllowedError);
+        .catch(expectInvalidStateError);
     });
 
     add_task(() => {

dom/webauthn/tests/test_webauthn_loopback.html

@@ -185,7 +185,7 @@ function() {
       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);
     });
   }