Bug 1381126: Resume requiring WebAuthn RP ID to be a Domain String r?keeler draft
authorJ.C. Jones <jjones@mozilla.com>
Mon, 11 Sep 2017 09:06:28 -0700
changeset 662626 b3a78a3ead193931af25811eef178329d2d74c7b
parent 661321 50857982881ae7803ceb438fee90650a282f7f05
child 730923 f86352ccbca9b65240e2b4f26b57e4ccb835ca0a
push id79140
push userbmo:jjones@mozilla.com
push dateMon, 11 Sep 2017 21:29:33 +0000
reviewerskeeler
bugs1381126, 1380421
milestone57.0a1
Bug 1381126: Resume requiring WebAuthn RP ID to be a Domain String r?keeler In Bug 1380421 we reverted some behavior that required Web Authentication's RP ID to be domain string to permit it to be an origin, too, for interop testing. That is no longer needed, so this patch resumes enforcement that RP ID be a domain string. It also adds a needed test that the RP ID hash is calculated correctly. MozReview-Commit-ID: 8dDjzo5kQKP
dom/webauthn/WebAuthnManager.cpp
dom/webauthn/tests/test_webauthn_loopback.html
dom/webauthn/tests/test_webauthn_sameorigin.html
--- a/dom/webauthn/WebAuthnManager.cpp
+++ b/dom/webauthn/WebAuthnManager.cpp
@@ -2,17 +2,16 @@
 /* vim:set ts=2 sw=2 sts=2 et cindent: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "hasht.h"
 #include "nsICryptoHash.h"
 #include "nsNetCID.h"
-#include "nsNetUtil.h" // Used by WD-05 compat support (Remove in Bug 1381126)
 #include "nsThreadUtils.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/dom/AuthenticatorAttestationResponse.h"
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/WebAuthnCBORUtil.h"
 #include "mozilla/dom/WebAuthnManager.h"
 #include "mozilla/dom/WebAuthnUtil.h"
 #include "mozilla/dom/PWebAuthnTransaction.h"
@@ -186,35 +185,17 @@ RelaxSameOrigin(nsPIDOMWindowInner* aPar
   if (!document || !document->IsHTMLDocument()) {
     return NS_ERROR_FAILURE;
   }
   nsHTMLDocument* html = document->AsHTMLDocument();
   if (NS_WARN_IF(!html)) {
     return NS_ERROR_FAILURE;
   }
 
-  // WD-05 origin compatibility support - aInputRpId might be a URI/origin,
-  // so catch that (Bug 1380421). Remove in Bug 1381126.
-  nsAutoString inputRpId(aInputRpId);
-  nsCOMPtr<nsIURI> inputUri;
-  if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(inputUri), aInputRpId))) {
-    // If we parsed the input as a URI, then pull out the host and use it as the
-    // input
-    nsAutoCString uriHost;
-    if (NS_FAILED(inputUri->GetHost(uriHost))) {
-      return NS_ERROR_FAILURE;
-    }
-    CopyUTF8toUTF16(uriHost, inputRpId);
-    MOZ_LOG(gWebAuthnManagerLog, LogLevel::Debug,
-            ("WD-05 Fallback: Parsed input %s URI into host %s",
-             NS_ConvertUTF16toUTF8(aInputRpId).get(), uriHost.get()));
-  }
-  // End WD-05 origin compatibility support (Bug 1380421)
-
-  if (!html->IsRegistrableDomainSuffixOfOrEqualTo(inputRpId, originHost)) {
+  if (!html->IsRegistrableDomainSuffixOfOrEqualTo(aInputRpId, originHost)) {
     return NS_ERROR_DOM_SECURITY_ERR;
   }
 
   aRelaxedRpId.Assign(NS_ConvertUTF16toUTF8(aInputRpId));
   return NS_OK;
 }
 
 static void
@@ -486,20 +467,18 @@ WebAuthnManager::MakeCredential(nsPIDOMW
   // and compute the clientDataJSON and clientDataHash.
 
   CryptoBuffer challenge;
   if (!challenge.Assign(aOptions.mChallenge)) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
     return promise.forget();
   }
 
-  // WD-05 vs. WD-06: In WD-06, the first parameter should be "origin". Fix
-  // this in Bug 1384776
   nsAutoCString clientDataJSON;
-  srv = AssembleClientData(NS_ConvertUTF8toUTF16(rpId), challenge, clientDataJSON);
+  srv = AssembleClientData(origin, challenge, clientDataJSON);
   if (NS_WARN_IF(NS_FAILED(srv))) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
     return promise.forget();
   }
 
   CryptoBuffer clientDataHash;
   if (!clientDataHash.SetLength(SHA256_LENGTH, fallible)) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
@@ -650,19 +629,17 @@ WebAuthnManager::GetAssertion(nsPIDOMWin
   // the clientDataJSON and clientDataHash.
   CryptoBuffer challenge;
   if (!challenge.Assign(aOptions.mChallenge)) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
     return promise.forget();
   }
 
   nsAutoCString clientDataJSON;
-  // WD-05 vs. WD-06: In WD-06, the first parameter should be "origin". Fix
-  // this in Bug 1384776
-  srv = AssembleClientData(NS_ConvertUTF8toUTF16(rpId), challenge, clientDataJSON);
+  srv = AssembleClientData(origin, challenge, clientDataJSON);
   if (NS_WARN_IF(NS_FAILED(srv))) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
     return promise.forget();
   }
 
   CryptoBuffer clientDataHash;
   if (!clientDataHash.SetLength(SHA256_LENGTH, fallible)) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
--- a/dom/webauthn/tests/test_webauthn_loopback.html
+++ b/dom/webauthn/tests/test_webauthn_loopback.html
@@ -58,24 +58,32 @@ function() {
 
     ok(aCredInfo.rawId === aCredInfo.rawId, "PublicKeyCredential.RawID is SameObject");
     ok(aCredInfo.response === aCredInfo.response, "PublicKeyCredential.Response is SameObject");
     ok(aCredInfo.response.clientDataJSON === aCredInfo.response.clientDataJSON, "PublicKeyCredential.Response.ClientDataJSON is SameObject");
     ok(aCredInfo.response.attestationObject === aCredInfo.response.attestationObject, "PublicKeyCredential.Response.AttestationObject is SameObject");
 
     let clientData = JSON.parse(buffer2string(aCredInfo.response.clientDataJSON));
     is(clientData.challenge, bytesToBase64UrlSafe(gCredentialChallenge), "Challenge is correct");
-    // WD-05 vs. WD-06: In WD-06, the second parameter should be "window.location.origin". Fix
-    // this in Bug 1384776
-    is(clientData.origin, document.domain, "Origin is correct");
+    is(clientData.origin, window.location.origin, "Origin is correct");
     is(clientData.hashAlg, "SHA-256", "Hash algorithm is correct");
 
     return webAuthnDecodeCBORAttestation(aCredInfo.response.attestationObject.buffer)
     .then(function(decodedResult) {
-      ok(decodedResult.flags == (flag_TUP | flag_AT), "User presence and Attestation Object must both be set");
+      // Make sure the RP ID hash matches what we calculate.
+      return crypto.subtle.digest("SHA-256", string2buffer(document.domain))
+      .then(function(calculatedHash) {
+        is(bytesToBase64(new Uint8Array(calculatedHash)), bytesToBase64(decodedResult.rpIdHash),
+           "Calculated RP ID hash must match what the browser derived.");
+        return Promise.resolve(decodedResult);
+      });
+    })
+    .then(function(decodedResult) {
+      ok(decodedResult.flags == (flag_TUP | flag_AT),
+         "User presence and Attestation Object must both be set");
 
       aCredInfo.clientDataObj = clientData;
       aCredInfo.publicKeyHandle = decodedResult.publicKeyHandle;
       aCredInfo.attestationObject = decodedResult.attestationObject;
       return aCredInfo;
     });
   }
 
@@ -96,19 +104,17 @@ function() {
     is(aAssertion.id, bytesToBase64UrlSafe(aAssertion.rawId), "Encoded Key ID and Raw Key ID match");
 
     ok(aAssertion.response.authenticatorData === aAssertion.response.authenticatorData, "AuthenticatorAssertionResponse.AuthenticatorData is SameObject");
     ok(aAssertion.response.signature === aAssertion.response.signature, "AuthenticatorAssertionResponse.Signature is SameObject");
 
     ok(aAssertion.response.authenticatorData.length > 0, "Authenticator data exists");
     let clientData = JSON.parse(buffer2string(aAssertion.response.clientDataJSON));
     is(clientData.challenge, bytesToBase64UrlSafe(gAssertionChallenge), "Challenge is correct");
-    // WD-05 vs. WD-06: In WD-06, the second parameter should be "window.location.origin". Fix
-    // this in Bug 1384776
-    is(clientData.origin, document.domain, "Origin is correct");
+    is(clientData.origin, window.location.origin, "Origin is correct");
     is(clientData.hashAlg, "SHA-256", "Hash algorithm is correct");
 
     return webAuthnDecodeAttestation(aAssertion.response.authenticatorData)
     .then(function(decodedResult) {
       ok(decodedResult.flags == flag_TUP, "User presence must be the only flag set");
       is(decodedResult.counter.length, 4, "Counter must be 4 bytes");
       return deriveAppAndChallengeParam(window.location.host, aAssertion.response.clientDataJSON, decodedResult)
     })
--- a/dom/webauthn/tests/test_webauthn_sameorigin.html
+++ b/dom/webauthn/tests/test_webauthn_sameorigin.html
@@ -226,40 +226,37 @@
             challenge: chall,
             rpId: "alt.test",
             allowList: [gTrackedCredential["basic"]]
           };
           return credm.get({publicKey: publicKeyCredentialRequestOptions})
                       .then(arrivingHereIsBad)
                       .catch(expectSecurityError);
         },
-        // These next two tests should be removed in Bug 1381126.
         function () {
           // Test basic good Create call but using an origin (Bug 1380421)
           let rp = {id: window.origin};
           let makeCredentialOptions = {
             rp: rp, user: user, challenge: chall, parameters: [param]
           };
           return credm.create({publicKey: makeCredentialOptions})
-                      .then(keepThisPublicKeyCredential("origin"))
-                      .then(arrivingHereIsGood)
-                      .catch(arrivingHereIsBad);
+                      .then(arrivingHereIsBad)
+                      .catch(expectSecurityError);
         },
         function () {
           // Test basic good Get call but using an origin (Bug 1380421)
           let publicKeyCredentialRequestOptions = {
             challenge: chall,
             rpId: window.origin,
-            allowList: [gTrackedCredential["origin"]]
+            allowList: [gTrackedCredential["basic"]]
           };
           return credm.get({publicKey: publicKeyCredentialRequestOptions})
-                      .then(arrivingHereIsGood)
-                      .catch(arrivingHereIsBad);
+                      .then(arrivingHereIsBad)
+                      .catch(expectSecurityError);
         }
-        // End remove in Bug 1381126.
       ];
       var i = 0;
       var runNextTest = () => {
         if (i == testFuncs.length) {
           SimpleTest.finish();
           return;
         }
         console.log(i, testFuncs[i], testFuncs.length);