978597 - Consider Implementing MAR hash verification with native APIs for OS X 10.6

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

+++ This bug was initially created as a clone of Bug #978596 +++

This bug is to consider implementing MAR hash verification using native APIs on OS X 10.6 and up using CDSA/CSSM.

https://developer.apple.com/library/mac/documentation/security/conceptual/cryptoservices/CDSA/CDSA.html

See bug 978596 for context. 
Please make this bug a much lower priority than bug 978596.
This will only be necessary if we can implement it before we deprecate 10.6 :)

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=a5e487ce0f75

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8410299 [details] [diff] [review]
Patch

This looks fine to me.

I also tested both the CDSA/CSSM and Security Transforms code, and didn't see any problems (as per bug 978596 comment #11).  However I didn't want to go to the trouble of trying to build on OS X 10.6 without clang, so I ran both tests on OS X 10.7.  To test the CDSA/CSSM code I made OnLionOrLater() lie by returning false when running on 10.7.

I do have a couple of questions, though:

+void* cssmMalloc (CSSM_SIZE aSize, void* aAllocRef) {
+  (void)aAllocRef;
+  return malloc(aSize);
+}

Why the first line?  Was it to avoid a compiler warning?

-void
-CryptoMac_FreeCertificate(CryptoX_Certificate* aCertificate)
-{
-  if (!OnLionOrLater()) {
-    return CERT_DestroyCertificate((CERTCertificate*)*aCertificate);
-  }
-
-  if (!aCertificate || !*aCertificate) {
-    return;
-  }
-  CFRelease((SecKeyRef)*aCertificate);
-}

Why did you remove the whole method, rather than just the first part?  Was it unused?  Was it wrong?

Comment 3

[Stephen A Pohl [:spohl]]

Thanks, Steven! I really appreciate the fast review! Answers inline below and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fca4b6565d8


(In reply to Steven Michaud from comment #2)
> +void* cssmMalloc (CSSM_SIZE aSize, void* aAllocRef) {
> +  (void)aAllocRef;
> +  return malloc(aSize);
> +}
> 
> Why the first line?  Was it to avoid a compiler warning?

You're right. I try to avoid any "unused variable" warnings. Over the years, I found this way of 'using' the variable to be the most reliable and portable one.

> -void
> -CryptoMac_FreeCertificate(CryptoX_Certificate* aCertificate)
> -{
> -  if (!OnLionOrLater()) {
> -    return CERT_DestroyCertificate((CERTCertificate*)*aCertificate);
> -  }
> -
> -  if (!aCertificate || !*aCertificate) {
> -    return;
> -  }
> -  CFRelease((SecKeyRef)*aCertificate);
> -}
> 
> Why did you remove the whole method, rather than just the first part?  Was
> it unused?  Was it wrong?

It became unnecessary. Basically, when we didn't use NSS on 10.7+, aCertificate was always NULL because CryptoMac_LoadPublicKey would never populate it when using Security Transforms. So we would always return before reaching the CFRelease at the bottom. With CDSA/CSSM we don't use certificates either. I noticed this while I was reviewing my memory management in this patch, so I simply removed it and replaced it with the #define CryptoX_FreeCertificate(aCertificate) in cryptox.h, which is simply a no-op.

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fca4b6565d8