Bug 1381126: Resume requiring WebAuthn RP ID to be a Domain String r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Mon, 11 Sep 2017 09:06:28 -0700
changeset 429667 9584975d84e0f443d83828be340b92391bf68377
parent 429666 89abc58564c87354f3d135c00afccab5aaa0d510
child 429668 d2d647e40002272e255ef95934e1d44b8aaa29b6
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1381126, 1380421
milestone57.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 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);