Bug 1309284 - WebAuthn JS API [part 2]: Bugfixes from testing r=keeler draft
authorJ.C. Jones <jjones@mozilla.com>
Fri, 16 Dec 2016 10:44:56 -0700
changeset 457781 52151480b4b2b600a598c06678b28f6be581e5e6
parent 457780 e9ae0c730523c04df89b24de93846e006db6268b
child 457782 1d1cbdabfa79097be5ad14a5e4ecb887883453e4
push id40890
push userjjones@mozilla.com
push dateMon, 09 Jan 2017 22:16:19 +0000
reviewerskeeler
bugs1309284
milestone53.0a1
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;