243245 - addbuiltins program fails on Windows

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 148166 [details] [diff] [review]
patch v1

Please review.

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Bob, please SR after reading Wan-Teh's question in comment 3 above.

Comment 5

[Robert Relyea]

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

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 7

[Robert Relyea]

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 148166 [details] [diff] [review]
patch v1

clearing review requests for obsolete patch.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 12

[Robert Relyea]

Comment on attachment 149302 [details] [diff] [review]
patch v2, requires no new symbols in nss.def

r=relyea

looks good

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

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.  :(

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Marking P2 for 3.10.