Bug 1244960 - FIDO u2f NSSToken (Part 3): Review updates. r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Wed, 13 Apr 2016 10:20:37 -0700
changeset 331368 d7edcf25d8396f3af13337cb717b706e84b7712c
parent 331367 f7c7877b38a2adaf8c1bb948ee4fe22555da122c
child 331369 2c274b2733a321944958c422dbea5a279b6d1cc0
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1244960
milestone48.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 1244960 - FIDO u2f NSSToken (Part 3): Review updates. r=keeler MozReview-Commit-ID: FkPHy9GGarU
dom/crypto/CryptoBuffer.cpp
dom/crypto/CryptoBuffer.h
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
dom/u2f/U2F.cpp
--- a/dom/crypto/CryptoBuffer.cpp
+++ b/dom/crypto/CryptoBuffer.cpp
@@ -187,26 +187,30 @@ CryptoBuffer::ToSECItem(PLArenaPool *aAr
 
 JSObject*
 CryptoBuffer::ToUint8Array(JSContext* aCx) const
 {
   return Uint8Array::Create(aCx, Length(), Elements());
 }
 
 bool
-CryptoBuffer::ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const
+CryptoBuffer::ToNewUnsignedBuffer(uint8_t** aBuf, uint32_t* aBufLen) const
 {
-  uint8_t* tmp = reinterpret_cast<uint8_t*>(moz_xmalloc(Length()));
-  if (!tmp) {
+  MOZ_ASSERT(aBuf);
+  MOZ_ASSERT(aBufLen);
+
+  uint32_t dataLen = Length();
+  uint8_t* tmp = reinterpret_cast<uint8_t*>(moz_xmalloc(dataLen));
+  if (NS_WARN_IF(!tmp)) {
     return false;
   }
 
-  memcpy(tmp, Elements(), Length());
-  *buf = tmp;
-  *bufLen = Length();
+  memcpy(tmp, Elements(), dataLen);
+  *aBuf = tmp;
+  *aBufLen = dataLen;
   return true;
 }
 
 // "BigInt" comes from the WebCrypto spec
 // ("unsigned long" isn't very "big", of course)
 // Likewise, the spec calls for big-endian ints
 bool
 CryptoBuffer::GetBigIntValue(unsigned long& aRetVal)
--- a/dom/crypto/CryptoBuffer.h
+++ b/dom/crypto/CryptoBuffer.h
@@ -42,17 +42,17 @@ public:
     aArray.ComputeLengthAndData();
     return Assign(aArray.Data(), aArray.Length());
   }
 
   nsresult FromJwkBase64(const nsString& aBase64);
   nsresult ToJwkBase64(nsString& aBase64);
   bool ToSECItem(PLArenaPool* aArena, SECItem* aItem) const;
   JSObject* ToUint8Array(JSContext* aCx) const;
-  bool ToNewUnsignedBuffer(uint8_t** buf, uint32_t* bufLen) const;
+  bool ToNewUnsignedBuffer(uint8_t** aBuf, uint32_t* aBufLen) const;
 
   bool GetBigIntValue(unsigned long& aRetVal);
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_CryptoBuffer_h
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -4243,86 +4243,96 @@ ContentParent::RecvSetURITitle(const URI
   nsCOMPtr<IHistory> history = services::GetHistoryService();
   if (history) {
     history->SetURITitle(ourURI, title);
   }
   return true;
 }
 
 bool
-ContentParent::RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,
-                                                  bool* isCompatible)
-{
+ContentParent::RecvNSSU2FTokenIsCompatibleVersion(const nsString& aVersion,
+                                                  bool* aIsCompatible)
+{
+  MOZ_ASSERT(aIsCompatible);
+
   nsCOMPtr<nsINSSU2FToken> nssToken(do_GetService(NS_NSSU2FTOKEN_CONTRACTID));
   if (NS_WARN_IF(!nssToken)) {
     return false;
   }
 
-  nsresult rv = nssToken->IsCompatibleVersion(version, isCompatible);
+  nsresult rv = nssToken->IsCompatibleVersion(aVersion, aIsCompatible);
   return NS_SUCCEEDED(rv);
 }
 
 bool
-ContentParent::RecvNSSU2FTokenIsRegistered(nsTArray<uint8_t>&& keyHandle,
-                                           bool* isValidKeyHandle)
-{
+ContentParent::RecvNSSU2FTokenIsRegistered(nsTArray<uint8_t>&& aKeyHandle,
+                                           bool* aIsValidKeyHandle)
+{
+  MOZ_ASSERT(aIsValidKeyHandle);
+
   nsCOMPtr<nsINSSU2FToken> nssToken(do_GetService(NS_NSSU2FTOKEN_CONTRACTID));
   if (NS_WARN_IF(!nssToken)) {
     return false;
   }
 
-  nsresult rv = nssToken->IsRegistered(keyHandle.Elements(), keyHandle.Length(),
-                                       isValidKeyHandle);
+  nsresult rv = nssToken->IsRegistered(aKeyHandle.Elements(), aKeyHandle.Length(),
+                                       aIsValidKeyHandle);
   return  NS_SUCCEEDED(rv);
 }
 
 bool
-ContentParent::RecvNSSU2FTokenRegister(nsTArray<uint8_t>&& application,
-                                       nsTArray<uint8_t>&& challenge,
-                                       nsTArray<uint8_t>* registration)
-{
+ContentParent::RecvNSSU2FTokenRegister(nsTArray<uint8_t>&& aApplication,
+                                       nsTArray<uint8_t>&& aChallenge,
+                                       nsTArray<uint8_t>* aRegistration)
+{
+  MOZ_ASSERT(aRegistration);
+
   nsCOMPtr<nsINSSU2FToken> nssToken(do_GetService(NS_NSSU2FTOKEN_CONTRACTID));
   if (NS_WARN_IF(!nssToken)) {
     return false;
   }
   uint8_t* buffer;
   uint32_t bufferlen;
-  nsresult rv = nssToken->Register(application.Elements(), application.Length(),
-                                   challenge.Elements(), challenge.Length(),
+  nsresult rv = nssToken->Register(aApplication.Elements(), aApplication.Length(),
+                                   aChallenge.Elements(), aChallenge.Length(),
                                    &buffer, &bufferlen);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return false;
   }
 
-  registration->ReplaceElementsAt(0, registration->Length(), buffer, bufferlen);
+  MOZ_ASSERT(buffer);
+  aRegistration->ReplaceElementsAt(0, aRegistration->Length(), buffer, bufferlen);
   free(buffer);
   return NS_SUCCEEDED(rv);
 }
 
 bool
-ContentParent::RecvNSSU2FTokenSign(nsTArray<uint8_t>&& application,
-                                   nsTArray<uint8_t>&& challenge,
-                                   nsTArray<uint8_t>&& keyHandle,
-                                   nsTArray<uint8_t>* signature)
-{
+ContentParent::RecvNSSU2FTokenSign(nsTArray<uint8_t>&& aApplication,
+                                   nsTArray<uint8_t>&& aChallenge,
+                                   nsTArray<uint8_t>&& aKeyHandle,
+                                   nsTArray<uint8_t>* aSignature)
+{
+  MOZ_ASSERT(aSignature);
+
   nsCOMPtr<nsINSSU2FToken> nssToken(do_GetService(NS_NSSU2FTOKEN_CONTRACTID));
   if (NS_WARN_IF(!nssToken)) {
     return false;
   }
   uint8_t* buffer;
   uint32_t bufferlen;
-  nsresult rv = nssToken->Sign(application.Elements(), application.Length(),
-                               challenge.Elements(), challenge.Length(),
-                               keyHandle.Elements(), keyHandle.Length(),
+  nsresult rv = nssToken->Sign(aApplication.Elements(), aApplication.Length(),
+                               aChallenge.Elements(), aChallenge.Length(),
+                               aKeyHandle.Elements(), aKeyHandle.Length(),
                                &buffer, &bufferlen);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return false;
   }
 
-  signature->ReplaceElementsAt(0, signature->Length(), buffer, bufferlen);
+  MOZ_ASSERT(buffer);
+  aSignature->ReplaceElementsAt(0, aSignature->Length(), buffer, bufferlen);
   free(buffer);
   return NS_SUCCEEDED(rv);
 }
 
 bool
 ContentParent::RecvGetSystemMemory(const uint64_t& aGetterId)
 {
   uint32_t memoryTotal = 0;
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -744,30 +744,30 @@ private:
 
   virtual bool
   RecvPBlobConstructor(PBlobParent* aActor,
                        const BlobConstructorParams& params) override;
 
   virtual bool
   DeallocPCrashReporterParent(PCrashReporterParent* crashreporter) override;
 
-  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& version,
-                                                  bool* isCompatible) override;
+  virtual bool RecvNSSU2FTokenIsCompatibleVersion(const nsString& aVersion,
+                                                  bool* aIsCompatible) override;
 
-  virtual bool RecvNSSU2FTokenIsRegistered(nsTArray<uint8_t>&& keyHandle,
-                                           bool* isValidKeyHandle) override;
+  virtual bool RecvNSSU2FTokenIsRegistered(nsTArray<uint8_t>&& aKeyHandle,
+                                           bool* aIsValidKeyHandle) override;
 
-  virtual bool RecvNSSU2FTokenRegister(nsTArray<uint8_t>&& application,
-                                       nsTArray<uint8_t>&& challenge,
-                                       nsTArray<uint8_t>* registration) override;
+  virtual bool RecvNSSU2FTokenRegister(nsTArray<uint8_t>&& aApplication,
+                                       nsTArray<uint8_t>&& aChallenge,
+                                       nsTArray<uint8_t>* aRegistration) override;
 
-  virtual bool RecvNSSU2FTokenSign(nsTArray<uint8_t>&& application,
-                                   nsTArray<uint8_t>&& challenge,
-                                   nsTArray<uint8_t>&& keyHandle,
-                                   nsTArray<uint8_t>* signature) override;
+  virtual bool RecvNSSU2FTokenSign(nsTArray<uint8_t>&& aApplication,
+                                   nsTArray<uint8_t>&& aChallenge,
+                                   nsTArray<uint8_t>&& aKeyHandle,
+                                   nsTArray<uint8_t>* aSignature) override;
 
   virtual bool RecvIsSecureURI(const uint32_t& aType, const URIParams& aURI,
                                const uint32_t& aFlags, bool* aIsSecureURI) override;
 
   virtual bool RecvAccumulateMixedContentHSTS(const URIParams& aURI,
                                               const bool& aActive) override;
 
   virtual bool DeallocPHalParent(PHalParent*) override;
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -91,128 +91,136 @@ U2F::Init(nsPIDOMWindowInner* aParent, E
   }
 
   if (NS_WARN_IF(mOrigin.IsEmpty())) {
     return;
   }
 
   if (!EnsureNSSInitializedChromeOrContent()) {
     MOZ_LOG(gU2FLog, LogLevel::Debug, ("Failed to get NSS context for U2F"));
+    aRv.Throw(NS_ERROR_FAILURE);
     return;
   }
 
   if (XRE_IsParentProcess()) {
     mNSSToken = do_GetService(NS_NSSU2FTOKEN_CONTRACTID);
     if (NS_WARN_IF(!mNSSToken)) {
+      aRv.Throw(NS_ERROR_FAILURE);
       return;
     }
   }
 
   aRv = mUSBToken.Init();
   if (NS_WARN_IF(aRv.Failed())) {
     return;
   }
 }
 
 nsresult
-U2F::NSSTokenIsCompatible(const nsString& versionString, bool* isCompatible)
+U2F::NSSTokenIsCompatible(const nsString& aVersionString, bool* aIsCompatible)
 {
+  MOZ_ASSERT(aIsCompatible);
+
   if (XRE_IsParentProcess()) {
     MOZ_ASSERT(mNSSToken);
-    return mNSSToken->IsCompatibleVersion(versionString, isCompatible);
+    return mNSSToken->IsCompatibleVersion(aVersionString, aIsCompatible);
   }
 
   ContentChild* cc = ContentChild::GetSingleton();
   MOZ_ASSERT(cc);
-  if (!cc->SendNSSU2FTokenIsCompatibleVersion(versionString, isCompatible)) {
+  if (!cc->SendNSSU2FTokenIsCompatibleVersion(aVersionString, aIsCompatible)) {
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 nsresult
-U2F::NSSTokenIsRegistered(CryptoBuffer& keyHandle, bool* isRegistered)
+U2F::NSSTokenIsRegistered(CryptoBuffer& aKeyHandle, bool* aIsRegistered)
 {
+  MOZ_ASSERT(aIsRegistered);
+
   if (XRE_IsParentProcess()) {
     MOZ_ASSERT(mNSSToken);
-    return mNSSToken->IsRegistered(keyHandle.Elements(), keyHandle.Length(),
-                                   isRegistered);
+    return mNSSToken->IsRegistered(aKeyHandle.Elements(), aKeyHandle.Length(),
+                                   aIsRegistered);
   }
 
   ContentChild* cc = ContentChild::GetSingleton();
   MOZ_ASSERT(cc);
-  if (!cc->SendNSSU2FTokenIsRegistered(keyHandle, isRegistered)) {
+  if (!cc->SendNSSU2FTokenIsRegistered(aKeyHandle, aIsRegistered)) {
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 nsresult
-U2F::NSSTokenRegister(CryptoBuffer& application, CryptoBuffer& challenge,
-                      CryptoBuffer& registrationData)
+U2F::NSSTokenRegister(CryptoBuffer& aApplication, CryptoBuffer& aChallenge,
+                      CryptoBuffer& aRegistrationData)
 {
   if (XRE_IsParentProcess()) {
     MOZ_ASSERT(mNSSToken);
     uint8_t* buffer;
     uint32_t bufferlen;
     nsresult rv;
-    rv = mNSSToken->Register(application.Elements(), application.Length(),
-                             challenge.Elements(), challenge.Length(),
+    rv = mNSSToken->Register(aApplication.Elements(), aApplication.Length(),
+                             aChallenge.Elements(), aChallenge.Length(),
                              &buffer, &bufferlen);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
 
-    registrationData.Assign(buffer, bufferlen);
+    MOZ_ASSERT(buffer);
+    aRegistrationData.Assign(buffer, bufferlen);
     free(buffer);
     return NS_OK;
   }
 
   nsTArray<uint8_t> registrationBuffer;
   ContentChild* cc = ContentChild::GetSingleton();
   MOZ_ASSERT(cc);
-  if (!cc->SendNSSU2FTokenRegister(application, challenge,
+  if (!cc->SendNSSU2FTokenRegister(aApplication, aChallenge,
                                    &registrationBuffer)) {
     return NS_ERROR_FAILURE;
   }
 
-  registrationData.Assign(registrationBuffer);
+  aRegistrationData.Assign(registrationBuffer);
   return NS_OK;
 }
 
 nsresult
-U2F::NSSTokenSign(CryptoBuffer& keyHandle, CryptoBuffer& application,
-                  CryptoBuffer& challenge, CryptoBuffer& signatureData)
+U2F::NSSTokenSign(CryptoBuffer& aKeyHandle, CryptoBuffer& aApplication,
+                  CryptoBuffer& aChallenge, CryptoBuffer& aSignatureData)
 {
   if (XRE_IsParentProcess()) {
     MOZ_ASSERT(mNSSToken);
     uint8_t* buffer;
     uint32_t bufferlen;
-    nsresult rv = mNSSToken->Sign(application.Elements(), application.Length(),
-                                  challenge.Elements(), challenge.Length(),
-                                  keyHandle.Elements(), keyHandle.Length(),
+    nsresult rv = mNSSToken->Sign(aApplication.Elements(), aApplication.Length(),
+                                  aChallenge.Elements(), aChallenge.Length(),
+                                  aKeyHandle.Elements(), aKeyHandle.Length(),
                                   &buffer, &bufferlen);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
 
-    signatureData.Assign(buffer, bufferlen);
+    MOZ_ASSERT(buffer);
+    aSignatureData.Assign(buffer, bufferlen);
     free(buffer);
     return NS_OK;
   }
 
   nsTArray<uint8_t> signatureBuffer;
   ContentChild* cc = ContentChild::GetSingleton();
   MOZ_ASSERT(cc);
-  if (!cc->SendNSSU2FTokenSign(application, challenge, keyHandle,
+  if (!cc->SendNSSU2FTokenSign(aApplication, aChallenge, aKeyHandle,
                                &signatureBuffer)) {
     return NS_ERROR_FAILURE;
   }
 
-  signatureData.Assign(signatureBuffer);
+  aSignatureData.Assign(signatureBuffer);
   return NS_OK;
 }
 
 nsresult
 U2F::AssembleClientData(const nsAString& aTyp,
                         const nsAString& aChallenge,
                         CryptoBuffer& aClientData) const
 {
@@ -415,17 +423,17 @@ U2F::Register(const nsAString& aAppId,
       if (NS_FAILED(rv)) {
         SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
                                                          ErrorCode::OTHER_ERROR);
         return;
       }
 
       if (isCompatible && isRegistered) {
         SendError<U2FRegisterCallback, RegisterResponse>(aCallback,
-                                                  ErrorCode::DEVICE_INELIGIBLE);
+                                                         ErrorCode::DEVICE_INELIGIBLE);
         return;
       }
     }
   }
 
   // Search the requests in order for the first some token can fulfill
   for (size_t i = 0; i < aRegisterRequests.Length(); ++i) {
     RegisterRequest request(aRegisterRequests[i]);