Bug 1309284 - WebAuthn JS API [part 2]: Bugfixes from testing r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Fri, 16 Dec 2016 10:44:56 -0700
changeset 328784 353ff937f3e0476d2d6a41db9e95566284cea796
parent 328783 6f8f397d5cd18f71c8398d1611082300711eaac8
child 328785 482e54376042ed35d176c5499fd1cf725d16ee08
push id85546
push userkwierso@gmail.com
push dateWed, 11 Jan 2017 02:36:30 +0000
treeherdermozilla-inbound@c5bce4cd684a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1309284
milestone53.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 1309284 - WebAuthn JS API [part 2]: Bugfixes from testing r=keeler Add more debugging information to signing operations for the NSS Soft Token. Bugfixes in WebAuthentication.cpp: - Calculate ArrayBuffer/View before using. - Fix an instance where we should return NotSupportedError. - Fix several instances where we should return Out Of Memory. - Fix a MozPromise assertion that occurs in GetAssertion if you coerce an early return. - Mark all constructors explicit. MozReview-Commit-ID: DQWHqZIlau9
dom/u2f/ScopedCredential.h
dom/u2f/ScopedCredentialInfo.h
dom/u2f/WebAuthentication.cpp
dom/u2f/WebAuthentication.h
dom/u2f/WebAuthnAssertion.h
dom/u2f/WebAuthnAttestation.h
dom/u2f/tests/u2futil.js
security/manager/ssl/nsNSSU2FToken.cpp
--- a/dom/u2f/ScopedCredential.h
+++ b/dom/u2f/ScopedCredential.h
@@ -22,17 +22,17 @@ namespace dom {
 class ScopedCredential final : public nsISupports
                              , public nsWrapperCache
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ScopedCredential)
 
 public:
-  ScopedCredential(WebAuthentication* aParent);
+  explicit ScopedCredential(WebAuthentication* aParent);
 
 protected:
   ~ScopedCredential();
 
 public:
   WebAuthentication*
   GetParentObject() const
   {
--- a/dom/u2f/ScopedCredentialInfo.h
+++ b/dom/u2f/ScopedCredentialInfo.h
@@ -30,17 +30,17 @@ namespace dom {
 class ScopedCredentialInfo final : public nsISupports
                                  , public nsWrapperCache
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ScopedCredentialInfo)
 
 public:
-  ScopedCredentialInfo(WebAuthentication* aParent);
+  explicit ScopedCredentialInfo(WebAuthentication* aParent);
 
 protected:
   ~ScopedCredentialInfo();
 
 public:
   WebAuthentication*
   GetParentObject() const
   {
--- a/dom/u2f/WebAuthentication.cpp
+++ b/dom/u2f/WebAuthentication.cpp
@@ -106,21 +106,25 @@ AssembleClientData(const nsAString& aOri
 static nsresult
 ScopedCredentialGetData(const ScopedCredentialDescriptor& aSCD,
                         /* out */ uint8_t** aBuf, /* out */ uint32_t* aBufLen)
 {
   MOZ_ASSERT(aBuf);
   MOZ_ASSERT(aBufLen);
 
   if (aSCD.mId.IsArrayBufferView()) {
-    *aBuf = aSCD.mId.GetAsArrayBufferView().Data();
-    *aBufLen = aSCD.mId.GetAsArrayBufferView().Length();
+    const ArrayBufferView& view = aSCD.mId.GetAsArrayBufferView();
+    view.ComputeLengthAndData();
+    *aBuf = view.Data();
+    *aBufLen = view.Length();
   } else if (aSCD.mId.IsArrayBuffer()) {
-    *aBuf = aSCD.mId.GetAsArrayBuffer().Data();
-    *aBufLen = aSCD.mId.GetAsArrayBuffer().Length();
+    const ArrayBuffer& buffer = aSCD.mId.GetAsArrayBuffer();
+    buffer.ComputeLengthAndData();
+    *aBuf = buffer.Data();
+    *aBufLen = buffer.Length();
   } else {
     MOZ_ASSERT(false);
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
 }
 
@@ -700,17 +704,17 @@ WebAuthentication::MakeCredential(JSCont
 
     // TODO: relax the same-origin restriction
 
     rpId.Assign(NS_ConvertUTF16toUTF8(aOptions.mRpId.Value()));
   }
 
   CryptoBuffer rpIdHash;
   if (!rpIdHash.SetLength(SHA256_LENGTH, fallible)) {
-    promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
+    promise->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
     return promise.forget();
   }
 
   nsresult srv;
   nsCOMPtr<nsICryptoHash> hashService =
     do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &srv);
   if (NS_WARN_IF(NS_FAILED(srv))) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
@@ -751,31 +755,40 @@ WebAuthentication::MakeCredential(JSCont
 
     // 4.1.1.4.d Add a new object of type ScopedCredentialParameters to
     // normalizedParameters, with type set to current.type and algorithm set to
     // normalizedAlgorithm.
     ScopedCredentialParameters normalizedObj;
     normalizedObj.mType = aCryptoParameters[a].mType;
     normalizedObj.mAlgorithm.SetAsString().Assign(algName);
 
-    normalizedParams.AppendElement(normalizedObj);
+    if (!normalizedParams.AppendElement(normalizedObj, mozilla::fallible)){
+      promise->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
+      return promise.forget();
+    }
   }
 
   // 4.1.1.5 If normalizedAlgorithm is empty and cryptoParameters was not empty,
   // cancel the timer started in step 2, reject promise with a DOMException
   // whose name is "NotSupportedError", and terminate this algorithm.
+  if (normalizedParams.IsEmpty() && !aCryptoParameters.IsEmpty()) {
+    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
+    return promise.forget();
+  }
 
   // 4.1.1.6 If excludeList is undefined, set it to the empty list.
 
   // 4.1.1.7 If extensions was specified, process any extensions supported by
   // this client platform, to produce the extension data that needs to be sent
   // to the authenticator. If an error is encountered while processing an
   // extension, skip that extension and do not produce any extension data for
   // it. Call the result of this processing clientExtensions.
 
+  // Currently no extensions are supported
+
   // 4.1.1.8 Use attestationChallenge, callerOrigin and rpId, along with the
   // token binding key associated with callerOrigin (if any), to create a
   // ClientData structure representing this request. Choose a hash algorithm for
   // hashAlg and compute the clientDataJSON and clientDataHash.
 
   CryptoBuffer challenge;
   if (!challenge.Assign(aChallenge)) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
@@ -945,53 +958,56 @@ WebAuthentication::GetAssertion(const Ar
   }
 
   srv = HashCString(hashService, clientDataJSON, clientDataHash);
   if (NS_WARN_IF(NS_FAILED(srv))) {
     promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
     return promise.forget();
   }
 
+  // Note: we only support U2F-style authentication for now, so we effectively
+  // require an AllowList.
+  if (!aOptions.mAllowList.WasPassed()) {
+    promise->MaybeReject(NS_ERROR_DOM_NOT_ALLOWED_ERR);
+    return promise.forget();
+  }
+
+  const Sequence<ScopedCredentialDescriptor>& allowList =
+    aOptions.mAllowList.Value();
+
   // 4.1.2.6 Initialize issuedRequests to an empty list.
   RefPtr<AssertionPromise> monitorPromise = requestMonitor->Ensure();
 
   // 4.1.2.7 For each authenticator currently available on this platform,
   // perform the following steps:
   for(Authenticator u2ftoken : mAuthenticators) {
     // 4.1.2.7.a If allowList is undefined or empty, let credentialList be an
     // empty list. Otherwise, execute a platform-specific procedure to determine
     // which, if any, credentials listed in allowList might be present on this
     // authenticator, and set credentialList to this filtered list. If no such
     // filtering is possible, set credentialList to an empty list.
 
     nsTArray<CryptoBuffer> credentialList;
 
-    // Note: we only support U2F-style authentication for now, so we effectively
-    // require an AllowList.
-    if (!aOptions.mAllowList.WasPassed()) {
-      promise->MaybeReject(NS_ERROR_DOM_NOT_ALLOWED_ERR);
-      return promise.forget();
-    }
-
-    const Sequence<ScopedCredentialDescriptor>& allowList =
-      aOptions.mAllowList.Value();
-
     for (const ScopedCredentialDescriptor& scd : allowList) {
       CryptoBuffer buf;
-      if (!buf.Assign(scd.mId)) {
-        promise->MaybeReject(NS_ERROR_DOM_UNKNOWN_ERR);
-        return promise.forget();
+      if (NS_WARN_IF(!buf.Assign(scd.mId))) {
+        continue;
       }
 
       // 4.1.2.7.b For each credential C within the credentialList that has a
       // non- empty transports list, optionally use only the specified
       // transports to get assertions using credential C.
 
       // TODO: Filter using Transport
-      credentialList.AppendElement(buf);
+      if (!credentialList.AppendElement(buf, mozilla::fallible)) {
+        requestMonitor->CancelNow();
+        promise->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
+        return promise.forget();
+      }
     }
 
     // 4.1.2.7.c If the above filtering process concludes that none of the
     // credentials on allowList can possibly be on this authenticator, do not
     // perform any of the following steps for this authenticator, and proceed to
     // the next authenticator (if any).
     if (credentialList.IsEmpty()) {
       continue;
--- a/dom/u2f/WebAuthentication.h
+++ b/dom/u2f/WebAuthentication.h
@@ -49,17 +49,17 @@ typedef MozPromise<AssertionPtr, nsresul
 class WebAuthentication final : public nsISupports
                               , public nsWrapperCache
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebAuthentication)
 
 public:
-  WebAuthentication(nsPIDOMWindowInner* aParent);
+  explicit WebAuthentication(nsPIDOMWindowInner* aParent);
 
 protected:
   ~WebAuthentication();
 
 public:
   nsPIDOMWindowInner*
   GetParentObject() const
   {
--- a/dom/u2f/WebAuthnAssertion.h
+++ b/dom/u2f/WebAuthnAssertion.h
@@ -28,17 +28,17 @@ namespace dom {
 class WebAuthnAssertion final : public nsISupports
                               , public nsWrapperCache
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebAuthnAssertion)
 
 public:
-  WebAuthnAssertion(WebAuthentication* aParent);
+  explicit WebAuthnAssertion(WebAuthentication* aParent);
 
 protected:
   ~WebAuthnAssertion();
 
 public:
   WebAuthentication*
   GetParentObject() const
   {
--- a/dom/u2f/WebAuthnAttestation.h
+++ b/dom/u2f/WebAuthnAttestation.h
@@ -21,17 +21,17 @@ namespace dom {
 class WebAuthnAttestation final : public nsISupports
                                 , public nsWrapperCache
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(WebAuthnAttestation)
 
 public:
-  WebAuthnAttestation(WebAuthentication* aParent);
+  explicit WebAuthnAttestation(WebAuthentication* aParent);
 
 protected:
   ~WebAuthnAttestation();
 
 public:
   WebAuthentication*
   GetParentObject() const
   {
--- a/dom/u2f/tests/u2futil.js
+++ b/dom/u2f/tests/u2futil.js
@@ -190,15 +190,17 @@ function verifySignature(key, data, derS
   let sigS = new Uint8Array(sigAsn1.result.value_block.value[1].value_block.value_hex);
 
   // The resulting R and S values from the ASN.1 Sequence must be fit into 32
   // bytes. Sometimes they have leading zeros, sometimes they're too short, it
   // all depends on what lib generated the signature.
   let R = sanitizeSigArray(sigR);
   let S = sanitizeSigArray(sigS);
 
+  console.log("Verifying these bytes: " + bytesToBase64UrlSafe(data));
+
   let sigData = new Uint8Array(R.length + S.length);
   sigData.set(R);
   sigData.set(S, R.length);
 
   let alg = {name: "ECDSA", hash: "SHA-256"};
   return crypto.subtle.verify(alg, key, sigData, data);
 }
--- a/security/manager/ssl/nsNSSU2FToken.cpp
+++ b/security/manager/ssl/nsNSSU2FToken.cpp
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* 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 "nsNSSU2FToken.h"
 
 #include "CryptoBuffer.h"
+#include "mozilla/Base64.h"
 #include "mozilla/Casting.h"
 #include "nsNSSComponent.h"
 #include "pk11pub.h"
 #include "prerror.h"
 #include "secerr.h"
 #include "WebCryptoCommon.h"
 
 using namespace mozilla;
@@ -717,16 +718,28 @@ nsNSSU2FToken::Sign(uint8_t* aApplicatio
 
   // It's OK to ignore the return values here because we're writing into
   // pre-allocated space
   signedDataBuf.AppendElements(aApplication, aApplicationLen, mozilla::fallible);
   signedDataBuf.AppendElement(0x01, mozilla::fallible);
   signedDataBuf.AppendSECItem(counterItem);
   signedDataBuf.AppendElements(aChallenge, aChallengeLen, mozilla::fallible);
 
+  if (MOZ_LOG_TEST(gNSSTokenLog, LogLevel::Debug)) {
+    nsAutoCString base64;
+    nsresult rv = Base64URLEncode(signedDataBuf.Length(), signedDataBuf.Elements(),
+                                  Base64URLEncodePaddingPolicy::Omit, base64);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return NS_ERROR_FAILURE;
+    }
+
+    MOZ_LOG(gNSSTokenLog, LogLevel::Debug,
+            ("U2F Token signing bytes (base64): %s", base64.get()));
+  }
+
   ScopedAutoSECItem signatureItem;
   SECStatus srv = SEC_SignData(&signatureItem, signedDataBuf.Elements(),
                                signedDataBuf.Length(), privKey.get(),
                                SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE);
   if (srv != SECSuccess) {
     MOZ_LOG(gNSSTokenLog, LogLevel::Warning,
             ("Signature failure: %d", PORT_GetError()));
     return NS_ERROR_FAILURE;