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
--- 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;