Bug 1244960 - FIDO u2f NSSToken (Part 3): Review updates r?keeler draft
authorJ.C. Jones <jjones@mozilla.com>
Wed, 13 Apr 2016 10:20:37 -0700
changeset 352085 8d6430bafde9338ea7c501436e020f0c3e5d9c96
parent 352084 c1006e362201e771b42fd82dc557d4e6861425c2
child 352086 b76e1700dfabf44f477ed1f070b1ac9b71c92bd0
push id15608
push userjjones@mozilla.com
push dateFri, 15 Apr 2016 16:33:37 +0000
reviewerskeeler
bugs1244960
milestone48.0a1
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]);