In the n.p.m.crypto newsgroup a user recently reported that he was able to succesfully add a cert to the builtins list on Linux, but not on Windows. The program's usage message shows that the cert is read in via stdin. No code is present to put stdin in "binary" mode on windows, so windows alters certs as they pass through stdin. A workaround is to use the undocumnted -i option to read the cert file, rather than reading it via stdin. I intend to add the missing code to put stdin into binary mode. Other issues I plan to address at the same time include: a) add the -i option to the usage message b) build addbuiltins with shared libs, if possible, c) build addbuiltins as part of the normal builds.
Comment on attachment 148166 [details] [diff] [review] patch v1 Please review.
Comment on attachment 148166 [details] [diff] [review] patch v1 This patch is fine. There is only one issue that I need Bob to decide. This patch requires exporting CERT_DecodeDERCertificate from the nss shared library. This function is among the ones that we didn't want to export but had to export for the smime shared library to work. These functions are exported with a double underscore (__) prefix. See the comment before __CERT_DecodeDERCertificate in lib/nss/nss.def.
Bob, please SR after reading Wan-Teh's question in comment 3 above.
Status: NEW → ASSIGNED
CERT_DERDecodeCert is 'archaic'. There are supposed to be a replacement version of this function. In some cases I think we found that we needed the original to keep from rewriting the SMIME libraries. It has the side effect of putting the cert in the 'tmpdb'. For the addbuiltin case we should replace the line doing the decode with the SEC_ASN1DecodeItem() call. See SECU_PrintCertificate() in cmd/lib for how this should be done. The remaining DERDecodeCert call in nss/cmd would then be in sign. It's used in the classic 'importing DER cert' case. We should use a real import DER call for the last case. bob
I completely disagree with Bob's comment 5. #1. CERT_DecodeDERCertificate is not "archaic". It is the heart of CERT_NewTempCertificate. It is used to decode every cert that is decoded. It is the ONLY function in NSS that takes a DER cert and creates a populated CERTCertificate structure from it. There is no other function that does exactly what it does. The primary difference between CERT_DecodeDERCert and CERT_NewTempCertificate is that the latter adds the decoded cert to the temp cert DB. There are many places (e.g. test programs) where adding the cert to the temp DB is undesirable. In those cases, CERT_DecodeDERCertificate is EXACTLY the program that should be called. So, I invite you to reconsider your objection in light of those facts.
CERT_NewTempCert is also "archaic". We've spent the last 5 years expunging both from our exported code. Neither function *purposefully* is exported in nss3.dll. For those places were it was to hard to match semantics, we included the underbar versions of these functions. We don't want to encourage general usage. CERT_DecodeDERCert is particually dangerous because it returns a partially initialized cert Structure. This is because the old cert structure does not separate decoded certs data from their pointers to objects within our system. Bad things are likely to happen if you pass a cert returned from CERT_DecodeDERCert to other certificate functions. There are two ways we can candle the call in addbuiltins: 1) (prefered) use SEC_ASN1DecodeItem() a.la. SECU_PrintCertificate. This makes it more clear that we don't have a 'real' CERTCertificate structure. I believe this should be the prefered way to get a decoded version of the cert without storing it. 2) use the already exported _CERT_DecodeDERCert with appropriate comments on the restrictions of the resulting cert structure.
Bob, CERT_DecodeDERCertificate does 12 steps, of which the initial ASN.1 decoding of the raw cert is only one of the steps. There is no newer (e.g. stan) function that does what this function does. You appear to be suggesting that the right solution is not to call this function that does all 12 steps, but rather to DUPLICATE this function, by separately performing the 12 steps that it performs. That necessitates exporting all the functions used for those 11 other steps as well. However, you reminded me (as I see Wan-Teh also did) that __CERT_DecodeDERCertificate is exported. So, I will use that.
Created attachment 149302 [details] [diff] [review] patch v2, requires no new symbols in nss.def This patch is identical to patch v1, except that it adds no symbols to nss.def and isntead includes "nssrenam.h" in allbuiltin.c
Attachment #148166 - Attachment is obsolete: true
Comment on attachment 148166 [details] [diff] [review] patch v1 clearing review requests for obsolete patch.
Comment on attachment 149302 [details] [diff] [review] patch v2, requires no new symbols in nss.def Bob, please review. Recapping this patch: a) adds the missing code to put stdin into binary mode. b) add the -i option to the usage message c) builds addbuiltins with shared libs (includes "nssrenam.h" for some symbols) d) build addbuiltins as part of the normal builds.
Attachment #149302 - Flags: review?(rrelyea0264)
Comment on attachment 149302 [details] [diff] [review] patch v2, requires no new symbols in nss.def r=relyea looks good
Attachment #149302 - Flags: review?(rrelyea0264) → review+
Thanks for the review. /cvsroot/mozilla/security/nss/cmd/manifest.mn,v <-- manifest.mn new revision: 1.17; previous revision: 1.16 /cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v <-- addbuiltin.c new revision: 1.9; previous revision: 1.8 /cvsroot/mozilla/security/nss/cmd/addbuiltin/manifest.mn,v <-- manifest.mn new revision: 1.5; previous revision: 1.4 The cvs log entry failed to include the bug number, sorry. :(
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Marking P2 for 3.10.
Priority: -- → P2
Target Milestone: --- → 3.10
You need to log in before you can comment on or make changes to this bug.