Closed
Bug 978597
Opened 10 years ago
Closed 10 years ago
Consider Implementing MAR hash verification with native APIs for OS X 10.6
Categories
(Toolkit :: Application Update, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bbondy, Assigned: spohl)
References
Details
Attachments
(1 file)
33.04 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
+++ 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 :)
Assignee | ||
Comment 1•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a5e487ce0f75
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8410299 -
Flags: review?(smichaud)
Comment 2•10 years ago
|
||
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?
Attachment #8410299 -
Flags: review?(smichaud) → review+
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4fca4b6565d8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•