308887 - CRMF request generation problem when using latest firefox

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Chandrasekar Kannan]

using our CA we attempted to a CRMF request generation to obtain a certificate. 
this process works ok with firefox 1.0 which uses nss 3.9.3. 

this doesn't work when using today's build of firefox ( deer park ) 20050916
which   uses nss 3.10.2

here's the bad CRMF request as reported by our Red Hat CA :

[16/Sep/2005:17:18:23][Thread-591]: DirBasedAuthentication: formed DN
'UID=testuser1,OU=people,O=redhat'
[16/Sep/2005:17:18:23][Thread-591]: returnConn: mNumConns now 1
[16/Sep/2005:17:18:23][Thread-591]: Start parseCRMF
MIIBozCCAZ8wggEFAgQYnqAzMIHHgAECpQ4wDDEKMAgGA1UEAxMBeKaBnzANBgkq^M
hkiG9w0BAQEFAAOBjQAwgYkCgYEAv7po72uvzlecMg16UA5c5lnhFm9l/BbfPQaL^M
MBSn6sK2pQL6XAVue+mxY1ICZqGS96Ry31icuphJSvrl1BGoDva3I1qzqTfVNBH2^M
dgfs8oZSDbAPE3oatCRfF2UVpWAb3MukzMbjE0I0tzZVB6BThaiuRpUtTZAsQUKM^M
crAMf08CAwEAAakQMA4GA1UdDwEB/wQEAwIF4DAzMBUGCSsGAQUFBwUBAQwIcmVn^M
VG9rZW4wGgYJKwYBBQUHBQECDA1hdXRoZW50aWNhdG9yoYGUMA0GCSqGSIb3DQEB^M
BQUAA4GBAJijYwUy0iOrJ/Yjud9qCLTHNk4VKxc2JrFiLV+chP3NkiiU9ajdj4I1^M
Jnf3TpCsafKTiFhF1oXwH6yug5h8NwZt7ktDCqoATSbVOoMFF0D/va6skZVuDl9u^M
IMsnLxZ8zkuN1t3F7qRCaiW8pf+vNEckqqMAF3o8p8XjJueaw+vJ
[16/Sep/2005:17:18:23][Thread-591]: EnrollProfile: parseCRMF
org.mozilla.jss.asn1.InvalidBERException: SEQUENCE(item #0) >> SEQUENCE(item #1)
>> SEQUENCE(item #3) >> SEQUENCE is 1 bytes longer than expected


as suggested by wan-teh, I replaced the NSS libraries in today's build with NSS
3.9.3 and it works ok.

Comment 1

[Chandrasekar Kannan]

this is a regression. setting priority to P1

Comment 2

[Wan-Teh Chang]

Bug 245429 was fixed in NSS 3.10 and has to do with
encoding CRMF.  I'll see if the regression was introduced
by that bug fix.

Comment 3

[Chandrasekar Kannan]

Steps to reproduce : ( using a redhat internal CA ): 

(1) use firefox and goto this url
https://rhcs.dsdev.sjc.redhat.com:3104/ca/profileSelect?profileId=caUserCert

(2) you can simply fill out the UID field and click Submit. 

If the request is good, you will see that the request gets submitted to the
CA. Else, you will get an error message "invalid request".

Comment 4

[Wan-Teh Chang]

I confirmed that Nelson's checkin to secasn1e.c, rev. 1.18,
for bug 245429 is the change that introduced the regression.
That patch is attachment 150326 [details] [diff] [review].  The CVS log message says:

revision 1.18
date: 2004/07/13 06:02:54;  author: nelsonb%netscape.com;  state: Exp;  lines: +
34 -52
Allow subtemplates to have the SEC_ASN1_DYNAMIC flag without asserting.
Bug 245429. Patch 4 of 5.  r=relyea.

Within the same period of time (between FIREFOX_1_0_RELEASE
=NSS_3_9_3_RTM and NSS_3_10_RTM),  there were no changes to
the ASN.1 decoders (secasn1d.c and quickder.c).

Comment 5

[Wan-Teh Chang]

Attachment: Real changes in secasn1e.c, rev. 1.18

The checkin of secasn1e.c, rev. 1.18 contains several
reformatting, whitespace, and comment changes.	This
patch contains the real changes in that checkin.

Comment 6

[Wan-Teh Chang]

Attachment: The change in secasn1e.c, rev. 1.18 that introduced the regression

My experiment showed that this is the change in the
checkin of secasn1e.c, rev. 1.18, that introduced the
regression.

Nelson's descriptions of that whole checkin (attachment 150326 [details] [diff] [review])
can be found in bug 245429 comment 6 and bug 245429 comment 24.

Comment 7

[Wan-Teh Chang]

Attachment: A patch that seems to work

I don't understand Nelson's change.  In bug 245429 comment 24,
he said sec_asn1e_contents_length and 
sec_asn1e_init_state_based_on_template parellel each other in
structure and function.  In sec_asn1e_init_state_based_on_template,
the corresponding code is:

       /*
	* This is an implicit, non-universal (meaning, application-private
	* or context-specific) field.  This results in a "magic" tag but
	* encoding based on the underlying type.  We pushed a new state
	* that is based on the subtemplate (the underlying type), but
	* now we will sort of alias it to give it some of our properties
	* (tag, optional status, etc.).
	*
	* NB: ALL the following flags in the subtemplate are disallowed
	*     and/or ignored: ECPLICIT, OPTIONAL, INNER< INLINE< POINTER.
	*/

       under_kind = state->theTemplate->kind;
       if ((under_kind & SEC_ASN1_MAY_STREAM) && !disallowStreaming) {
	   may_stream = PR_TRUE;
       }
       under_kind &= ~(SEC_ASN1_MAY_STREAM | SEC_ASN1_DYNAMIC);
   } else {
       under_kind = encode_kind;
   }

So I wrote this patch to undo some of Nelson's change so it matches
the above code better.	It doesn't cause any assertion failures when
I run Chandra's test and the CRMF message is valid.

I hope Nelson can look at this bug.  Any comment will be helpful.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I wonder about a couple of things.

1. How could 3.9.3 CRMF code be perceived as working OK in the browser when 
   the same code experienced several assertion failres when run in the QA
   test program?
2. Does PSM duplicate some of the CRMF code in NSS?

When I started working on bug 245429, the CRMF tests in NSS were having 
assertion failures.  The CRMF test program in nss/cmd failed.  With my 
patches in place, it passed the tests.  

So, I'm wondering how that code, which failed assertions in NSS 3.9, could 
be perceived as working in the browser.  I'm thinking perhaps the browser 
is using its own templates and/or structures, instead of the ones in NSS?  

As for the invalid encoding (one extra byte), I think that is the subject 
of bug 244922.  That patch has been blocked waiting for review for some 
months.  (Let's discuss that offline, if desired.) 
I still hope to get that fix into NSS 3.11.

Wan-Teh, perhaps you and Chandrasekar can test and see if the patch for 
bug 244922 fixes this problem.  If so, please mark this bug as being 
blocked by that bug.  

Comment 9

[Wan-Teh Chang]

Nelson, the patch in bug 244922 (which is patch part 5 of
bug 245429) fixed this bug.  Thanks.

Does this mean patch part 4 and patch part 5 of bug 245429
need to be checked in together?  I am wondering if the
proper action for the NSS_3_10_BRANCH is to back out (portions
of) patch part 4 and the proper action for the tip (NSS 3.11)
is the check in patch part 5?  The idea is that patch parts 4
and 5 should be both in or both out, and it is too risky to
check in part 5 on the NSS_3_10_BRANCH.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

The history of the asn.1 encoder shows an oscillating behavior.  There are 
several components of NSS and PSM that use it.  One of them has a bug, and 
puts in a patch that fixes that component.  That patch is then found to 
break a second component.  So the developer of that second component 
introduces another change that breaks the first component again.  This has 
been going on for 4 years now, since rev 1.3 of secasn1e.c.  See 
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/security/nss/lib/util/secasn1e.c

In the summer of 2004, I developed a set of patches that seemed to fix all
components for which I had test cases, but I did not have test cases for them
all.  See bug 245429 for that list of patches.  This work was all motivated 
by the desire to fix CRMF without (re)introducing regressions in other code
that uses the ASN.1 encoder.  The CRMF work was stopped before it was done
because of my employer's changing priorities.  

I think that the last (5th) patch for bug 245429 should also fix the problem 
reported in this bug.  The question is: will it reintroduce any problems in 
any of the other code that uses the encoder, for which we perhaps do not have 
an adequate test case?

I no longer have all the knowledge of the ASN.1 encoder users in my head that
I had 15 months ago.  I know that the change that Wan-Teh's patch proposes
to undo was done to fix a specific problem experienced by one of the encoder's
users, but I do not now recall which one it was.  I suspect that undoing that
change will reintroduce the bug is was trying to fix - more oscillation. 

Several months ago, I asked one of our developers to work on developing test
cases for all the nss and PSM code that uses the encoder, in the hope of 
finally stopping the oscillation.  I hoped it would verify that the last 
patch of the 5 attached to bug 245429 would fix the original problem in a 
way that did not break any other users of it.  Unfortunately, that project
(to test all the NSS/PSM users of the ASN.1 encoder) has taken months, much
longer than I would have guessed.  :(

In the meantime, we need to choose how to move forward.  There are at least
3 known bugs that patch 5 is known to fix, so I suggest we move forward with
it.  I only wish we had more confidence that there is no encoder user that
will experience any regressions due to it.

Comment 11

[Wan-Teh Chang]

When the patch in bug 244922 is applied to Thunderbird,
it generates an invalid S/MIME signature.  Here are the
tests that we performed on that patch:

(1) crmf cert request generation  and approval by CA - PASS
(2) import certificate generated in step 1 into browser - PASS
(3) backup certificate imported in step 2 as a .p12 file - PASS
(4) delete cert from browser and re-import .p12 file - PASS
(5) send s/mime signed and encrypted email - FAIL { digitial
    signature is not valid }
(6) try pkcs10 instead of crmf - NOT DONE.
(7) NSS all.sh - PASS

It's unlikely to get two reviews for the patch in bug 244922
in time for NSS 3.10.2.  Now we also need to debug the invalid
S/MIME signature problem to even get the patch considered for
NSS 3.11.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, if TBird generates invalid signatures, but all.sh passes, this
means that our libSMIME test program, cmsutil, is not effectively testing
libSMIME in the way that TBird is doing.  That problem alone seems like it 
should be P1.  That is, we should ensure that cmsutil (or some test program
that we run in all.sh) tests libSMIME in exactly the same way (using the
same templates) as TBird.  The whole point of cmsutil is to test libSMIME
sufficiently that when it passes, then we know that libSMIME will also work
for TBird.

Alternatively, we might find that TBird has its own templates and some of 
them are wrong (we've found numerous erroneous templates elsewhere, but 
haven't looked at the templates in PSM).  

For the last 4 years or more, at no time have ALL the users of our ASN.1
encoder been working correctly.  All attempts to fix one of them have
broken another.  It seems likely that one or more of them are using the 
ASN.1 encoder incorrectly, making incorrect assumptions about how it is 
is intended to work.  If multiple users are making different assumptions
about how the ASN.1 encoder is supposed to be used, and they cannot all
be true, then at least one of them must be changed to match the others'
assumptions.  The oscillation cannot stop until then.

In the meantime, if we don't have the time or desire to really fix this
once and for all, then I think we must decide which of the many problems
is the least evil of the bunch and live with that.  I submit that bug 
244922 is NOT that least evil bug.  It has 2 other bugs that are dups of
it, and it blocks 3 other bugs.  If that patch fixes 5 bugs but appears
to introduce one more, I would like us to understand that one more, to 
see if it is actually a bug in the user of the encoder, based on an 
incorrect assumption.  I propose we discuss this this aftenroon.

Comment 13

[Wan-Teh Chang]

Bob, could you look at Thunderbird's invalid S/MIME signature
when the patch in bug 244922 is applied?  I am not familiar with
the ASN.1 encoder code.

Comment 14

[Robert Relyea]

Attachment: Patch to improve CMS converage

This patch does 3 things:
1) Improves CMS coverage by adding a test case for detached signatures.
2) Fixes the CMS converage to actually use the higher level hash functions.
3) preserves the intermediate results for the higher level hash functions for
later diagnostic evaluation

Comment 15

[Robert Relyea]

Comment on attachment 197446 [details] [diff] [review]
Patch to improve CMS converage

Oops there's a problem with this patch...

bob

Comment 16

[Robert Relyea]

Attachment: Working version of the tests patch

OK, this patch should work. The previous patch was missing a -c alice.txt in
the verify line. the -c was needed to fetch the original test so the signature
could be verified (since the resulting signature is a detached signature).

I've verified this works with the system secasn1e.c, but not the patched
version.

bob

Comment 17

[Robert Relyea]

Attachment: Unified context diff of previous patch

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197472 [details] [diff] [review]
Unified context diff of previous patch


>+smime_sign()
>+{
>+  if [ "${HASH}" = "SHA1"];  then 
>+      HASH_CMD=""
>+      SIG=sig
>+   else
>+      HASH_CMD=-H ${HASH}
>+      SIG=sig.${HASH}
>+  fi

The above code treats SHA1 as a special case.  I believe that's unnecesary.
Please treat it just like all the other hash cases.

>+  echo "$SCRIPTNAME: Signing Detached Message {$HASH} ------------------"
>+  echo "cmsutil -S -T -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} \
-p nss -o alice.${SIG}"
		^ "d" is missing there.
>+  cmsutil -S -T -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.d${SIG}
>+  html_msg $? 0 "Create Detached Signature Alice (${HASH})" "."
>+
>+  echo "cmsutil -D -i alice.d${SIG} -c alice.txt -d ${P_R_BOBDIR} "
>+  cmsutil -D -i alice.d${SIG} -c alice.txt -d ${P_R_BOBDIR} 
>+  html_msg $? 0 "Verifying Alice's Signature (${HASH})" "."
				    ^ insert Detached
>+
>+  echo "$SCRIPTNAME: Signing Attached Message (${HASH}) ------------------"
>+  echo "cmsutil -S -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.${SIG}"
>+  cmsutil -S -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.${SIG}
>+  html_msg $? 0 "Create Signature Alice (${HASH})" "."
			 ^ insert Attached
>+
>+  echo "cmsutil -D -i alice.${SIG} -d ${P_R_BOBDIR} -o alice.data1.${HASH}"
>+  cmsutil -D -i alice.${SIG} -d ${P_R_BOBDIR} -o alice.data1.${HASH}
>+  html_msg $? 0 "Decode Alice's Signature (${HASH})" "."

Please change that to "Verify Alice's Attached Signature" ...

>+  echo "diff alice.txt alice.data1.${HASH}"
>+  diff alice.txt alice.data1.${HASH}
>+  html_msg $? 0 "Compare Decoded Signature and Original (${HASH})" "."

please change Signature to Text.  It's not comparing signatures. 
It's comparing the original text to the signed/decoded text.

Comment 19

[Robert Relyea]

Attachment: Incorporate nelson's comments

Comment 20

[Robert Relyea]

Attachment: add crmf tests.

OK, I've integrated crmf tests. This patch does the following:

1) create a new crmf test script and add it to all.sh (tests/crmf/crmf.sh
tests/all.sh).
2) add a paramater to pass the password on the command line to crmftest.c
3) fix some of the code needed to handle database initialization in crmftest.c
4) found and fixed the reference leak in the crmf library.

The test detects the asn1 encoder problem in 3.10 and passes with no problem on
the tip.

Comment 21

[Robert Relyea]

OK, I've now determined that a small change to nelson's patch 5 in bug 244922
will solve both the crmf problem and the detached signature encoding problem.
I'll attach my new version to that bug.

This bug has patches to our build system with are required to detect future
regressions in both of these areas.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197591 [details] [diff] [review]
Incorporate nelson's comments

Bob, I think you applied one of my suggested changes to the wrong script line.
The objectives of my previous comment were 
a) to have the echo'ed command match the command that is actually executed, 
and 
b) to use alice.d$(SIG) for detached sigs and alice.$(SIG) for attached ones.

>+smime_sign()
>+{
>+  HASH_CMD=-H ${HASH}
>+  SIG=sig.${HASH}
>+
>+  echo "$SCRIPTNAME: Signing Detached Message {$HASH} ------------------"
>+  echo "cmsutil -S -T -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.${SIG}"

That line above is the one that needs to change to say "alice.d$(SIG), so that 
it agrees with the actual command line below

>+  cmsutil -S -T -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.d${SIG}
>+  html_msg $? 0 "Create Detached Signature Alice (${HASH})" "."
>+
>+  echo "cmsutil -D -i alice.d${SIG} -c alice.txt -d ${P_R_BOBDIR} "
>+  cmsutil -D -i alice.d${SIG} -c alice.txt -d ${P_R_BOBDIR} 
>+  html_msg $? 0 "Verifying Alice's Detached Signature (${HASH})" "."
>+
>+  echo "$SCRIPTNAME: Signing Attached Message (${HASH}) ------------------"
>+  echo "cmsutil -S -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.d${SIG}"

The line just above should use alice.$(SIG), not alice.d$(SIG), to agree
with the command below, and because this is an attached sig.

>+  cmsutil -S -N Alice ${HASH_CMD} -i alice.txt -d ${P_R_ALICEDIR} -p nss -o alice.${SIG}
>+  html_msg $? 0 "Create Attached Signature Alice (${HASH})" "."

Comment 23

[Wan-Teh Chang]

Bob,

First, I'd like to thank you for investigating this bug
and also adding several new test cases to all.sh.

In comment 20, you wrote:
> The test detects the asn1 encoder problem in 3.10 and
> passes with no problem on the tip.

Did you mean 3.9 rather than 3.10?  Nelson's secasn1e.c
patch part 4 was checked in before 3.10 was released, so
3.10 and the tip should behave the same.

Comment 24

[Robert Relyea]

re comment 23

Sorry, the should have been tip with nelson's 'patch 5' applied.

bob

Comment 25

[Robert Relyea]

Attachment: ASN.1 encoder patch (for bug 244922?)

Sigh I should finally have it right now..

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197648 [details] [diff] [review]
ASN.1 encoder patch (for bug 244922?)

Bob, I believe you intended to attach here a patch to smime.sh, but instead you
attached a patch for secasn1e.c.  I believe the patch you attached here was
intended for another bug.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197648 [details] [diff] [review]
ASN.1 encoder patch (for bug 244922?)

This ASN.1 encoder patch was attached to this bug with a comment that indicated
that it was intended to be a replacement smime.sh patch.  
I think maybe *this* patch was intended to be attached to bug 244922, since it
seems to replace my original patch that is attached to that bug.  But there is
already a replacement patch attached to that bug, and it's not the same as this
one.  So, right now I'm very confused about these patches.  I don't know which
ASN.1 encoder patch  (this one, or the one attached to bug 244922 is the one
Bob really intends to replace my old "patch 5".  

Bob, please attach the replacement smime.sh patch to this bug and clearly mark
it as such.  

Also, please indicate whether this patch, or the patch attached to bug	244922
is the one really intended to fix bug 244922.  For now, I'm marking this patch
r-, but maybe that will change once we get all this patch confusion
straightened out.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197632 [details] [diff] [review]
add crmf tests.

Bob, in this patch, you ask:

>+    /* why isn't this just a DupCert? */
>     return CERT_NewTempCertificate(CERT_GetDefaultCertDB(), 
> 	          &inKeyRecRep->newSigCert->signatureWrap.data,
> 		   NULL, PR_FALSE, PR_TRUE);

The answer is that CRMF and CMMF do their own cert decoding.  
They don't call CERT_NewTempCertificate in the first place.  
inKeyRecRep->newSigCert doesn't have its own arena, and isn't a
dup'able cert.

When they decode the CRMF/CMMF messages that contain certs, the
CRMF/CMMF functions *SHOULD* decode each cert as type SEC_ASN1_ANY, 
so that the undecoded raw DER cert is extracted from the CRMF/CMMF 
message, and then is separately decoded by passing it to (something 
like) CERT_NewTempCertificate.	That way, when they decode the cert 
with CERT_NewTempCertificate, they will get a full blown 
CERTCertificate that can be dup'ed etc.  But that is not what they do.

Instead, the CRMF and CMMF templates for messages that contain certs
decode those certs themselves, while decoding the messages that contain
them.  They create CERTCertificate structures, but those structures are
not fully initialized as they would be if they had been created by 
CERT_NewTempCertificate.  

For example:  The template CMMFKeyRecRepContentTemplate, which 
corresponds to the inKeyRecRep structure cited in the code above, 
contains subtemplates such as SEC_SignedCertificateTemplate.  The 
CERTCertificate pointer inKeyRecRep->newSigCert is produced by 
using the SEC_SignedCertificateTemplate template to decode the cert
inline as part of the CMMF message.  

It would be better if the subtemplate used to decode (and encode) 
certs in CMMFKeyRecRepContentTemplate was SEC_ASN1_ANY.  Then the raw 
DER cert would be dumped into a buffer, where the function that 
decodes the CMMF message could then subsequently pass the raw DER
cert to CERT_NewTempCertificate for a full and proper decoding.

The certs decoded that way do not have their own arenapools, as certs
decoded by CERT_NewTempCertificate do.	Their contents are part of 
the arenapool used for the CMMF message.  I suspect the ref counts of
the certs decoded that way are not initialied properly, either.  
So, one must be careful is using the CERTCertificate structs that are 
part of decoded CMMF messages as if they were ordinary temp certs.
IIRC, that struct has a member named isDecoded, whose purpose is to
record the fact that the cert in that struct is part of the CMMF
arena, and isn't an ordinary CERTCertificate.  *Hack* *Cough*

Note that this problem is not limited to the decoder.  When encoding
the CMMF messages, that cert is encoded from its components, too. 
Rather than taking the pre-issued DER cert and including it 
verbatim in the generated message, the cert is re-encoded from 
the components in the CERTCertificate named newSigCert.  

Clearly all that CRMF/CMMF code that decodes/encodes certs as part
of the CRMF/CMMF messages is broken.  It should be changed to 
decode the certs as "ANY"s, so that the certs' component fields 
are not decoded while decoding the CMMF messages.  I was going to 
rewrite that CRMF/CMMF code, so that it en/decoded the certs as
ANYs, after completing the work on the ASN.1 encoder that I was
doing in Summer 2004.  But that work was cut short, as you know.

Comment 29

[Robert Relyea]

Attachment: 15th time's a charm: smime.sh again

Comment 30

[Robert Relyea]

re comment 27

The the patch here is identical to the on in 244922, not another version. I
grabbed the wrong 'patch' file when I meant to attach the smime.sh patch.

re comment 28

Yes, after reviewing the code further I discovered that the cert could either be
a hand decoded cert or a real CERTCertificate (and thus the original !isDecoded
in the free routine, though it was 'too agressive' since several of the
components do exactly as you suggest [grab an 'any' and then build certs from
them]).

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197712 [details] [diff] [review]
15th time's a charm: smime.sh again

Yup, 15th time's the charm.  :)

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #30)

> re comment 28
> 
> Yes, after reviewing the code further I discovered that the cert could 
> either be a hand decoded cert or a real CERTCertificate (and thus the 
> original !isDecoded in the free routine, though it was 'too agressive' 
> since several of the components do exactly as you suggest 

I think that getting the CRMF/CMMF code to the point where we can finally
be confident that CERTCertificates inside it are being properly handled
(freed/duped as appropriate) will be very dificult unless and until one
of us finally fixes it to stop doing its own encoding/decoding of certs
and start using ANY for the certs.  

Is that a project you'd be willing to undertake? 

There are several bugs filed about these problems in CRMF/CMMF, 
including bug 245420, bug 245941, and bug 305019.  
Of those, the first two are probably most immediately relevant.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197632 [details] [diff] [review]
add crmf tests.

r-.  Several small things.

>Index: tests/all.sh

>+#   crmf.sh   - CRMF/CMMF testing

>-TESTS="cert ssl sdr cipher smime perf tools fips dbtests"
>+TESTS="cert ssl sdr cipher smime crmf perf tools fips dbtests"

There is not presently a directory named nss/tests/crmf in the 
tree, nor is there a crmf.sh in that directory.  We shouldn't
change all.sh to invoke that script until it exists.  
Also this change conflicts with the change for bug 309412,
which I am going to commit within 24 hours.
Maybe you should make a separate patch that adds crmf.sh, and
adds it to all.sh.


>@@ -216,7 +200,7 @@
>     printf ("Warning: ALL PIN'S WILL BE ECHOED TO SCREEN!!!\n");
>     printf ("Now enter the PIN for the user: ");
>     scanf  ("%s", userPin);
>-    return PK11_InitPin (slot, NULL, userPin);
>+    return PK11_InitPin (slot, "", userPin);
> }

Do we still need the function SetSlotPassword, given that we're
using SECU_GetModulePassword ?


>Index: lib/crmf/cmmfrec.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/crmf/cmmfrec.c,v
>retrieving revision 1.4
>diff -u -r1.4 cmmfrec.c
>--- lib/crmf/cmmfrec.c	3 Jun 2004 03:41:07 -0000	1.4
>+++ lib/crmf/cmmfrec.c	27 Sep 2005 23:29:23 -0000
>@@ -70,22 +70,22 @@
> {
>     PORT_Assert(inKeyRecRep != NULL);
>     if (inKeyRecRep != NULL && inKeyRecRep->poolp != NULL) {
>-        if (!inKeyRecRep->isDecoded) {
>-	    int i;
>+	int i;
> 
>+	if (!inKeyRecRep->isDecoded && inKeyRecRep->newSigCert != NULL) {
> 	    CERT_DestroyCertificate(inKeyRecRep->newSigCert);
>-	    if (inKeyRecRep->caCerts != NULL) {
>-	        for (i=0; inKeyRecRep->caCerts[i] != NULL; i++) {
>-		    CERT_DestroyCertificate(inKeyRecRep->caCerts[i]);
>-		}
>+	}
>+	if (inKeyRecRep->caCerts != NULL) {
>+	    for (i=0; inKeyRecRep->caCerts[i] != NULL; i++) {
>+		CERT_DestroyCertificate(inKeyRecRep->caCerts[i]);
> 	    }

If "isDecoded" is true, doesn't that mean that the "caCerts" are also
bogus certs, and that we shouldn't destroy them either?
Doesn't "isDecoded" mean that all the certs in the struct are bogus?

I think that's how the code interpreted that flag before this patch,
and I think that was a correct interpretation.	
If "isDecoded" means "some are valid and some are bogus", then we
need to fix that or we'll never get it right.


>@@ -117,6 +117,9 @@
>     if (inKeyRecRep == NULL || inNewSignCert == NULL) {
>         return SECFailure;
>     }
>+    if (inKeyRecRep->isDecoded && inKeyRecRep->newSigCert) {
>+	CERT_DestroyCertificate(inKeyRecRep->newSigCert);
>+    }

Shouldn't that new test begin with 
   if (!inKeyRecRep->isDecoded ...
??  Isn't it true that we don't want to destroy it when isDecoded is true?

>     inKeyRecRep->newSigCert = CERT_DupCertificate(inNewSignCert);

After doing this, do we need to clear the "isDecoded" flag, to show that
this cert is a real CERTCertificate, to ensure that it will get properly
destroyed?  (But what if some certs are decoded and others not?  Oy!)

>     return (inKeyRecRep->newSigCert == NULL) ? SECFailure : SECSuccess;    
> }
>@@ -231,6 +234,7 @@
> 	inKeyRecRep->newSigCert == NULL) {
>         return NULL;
>     }
>+    /* why isn't this just a DupCert? */

Maybe this comment should explain why, rather than ask.

>     return CERT_NewTempCertificate(CERT_GetDefaultCertDB(), 
> 			          &inKeyRecRep->newSigCert->signatureWrap.data,
> 				   NULL, PR_FALSE, PR_TRUE);

Comment 34

[Robert Relyea]

> Maybe you should make a separate patch that adds crmf.sh, and
> adds it to all.sh.

I initially wanted to do just that, but the crmf tests were failing because of a
reference leak. The cmmfrec.c patch was to fix that.


> >@@ -216,7 +200,7 @@
> >     printf ("Warning: ALL PIN'S WILL BE ECHOED TO SCREEN!!!\n");
> >     printf ("Now enter the PIN for the user: ");
> >     scanf  ("%s", userPin);
> >-    return PK11_InitPin (slot, NULL, userPin);
> >+    return PK11_InitPin (slot, "", userPin);
> > }
> 
> Do we still need the function SetSlotPassword, given that we're
> using SECU_GetModulePassword ?

Yes, well sort of. We still should initialize the database if it wasn't
initialized (SECU_GetModulePassword doesn't do that), however we can use
SECU_Change_ChangePW(). We should also use libsec to prompt the password without
echoing it here.

> If "isDecoded" is true, doesn't that mean that the "caCerts" are also
> bogus certs, and that we shouldn't destroy them either?
> Doesn't "isDecoded" mean that all the certs in the struct are bogus?
> 
> I think that's how the code interpreted that flag before this patch,
> and I think that was a correct interpretation.	
> If "isDecoded" means "some are valid and some are bogus", then we
> need to fix that or we'll never get it right.

No, the isDecode only applies to the newSignCert. This was the source of the
reference leak. All the others are real certs (caCerts are handled in completely
separate ways, and keyPair history are always full certs (well if choice
indicates that they are a cert).


> >@@ -117,6 +117,9 @@
> >     if (inKeyRecRep == NULL || inNewSignCert == NULL) {
> >         return SECFailure;
> >     }
> >+    if (inKeyRecRep->isDecoded && inKeyRecRep->newSigCert) {
> >+	CERT_DestroyCertificate(inKeyRecRep->newSigCert);
> >+    }

> Shouldn't that new test begin with 
>    if (!inKeyRecRep->isDecoded ...
> ??  Isn't it true that we don't want to destroy it when isDecoded is true?

Yes, this is a bug in the patch. and we should reset 'isDecoded'. I suspect that
this call is never made on a inKeyRecRep, but it should work correctly if it
ever is.

>     inKeyRecRep->newSigCert = CERT_DupCertificate(inNewSignCert);

After doing this, do we need to clear the "isDecoded" flag, to show that
this cert is a real CERTCertificate, to ensure that it will get properly
destroyed?  (But what if some certs are decoded and others not?  Oy!)


> >+    /* why isn't this just a DupCert? */

> Maybe this comment should explain why, rather than ask.

yes, better than just removing it;).

Comment 35

[Robert Relyea]

> Also this change conflicts with the change for bug 309412,
> which I am going to commit within 24 hours.

In both patches crmf should go after smime, though it does not depend on smime
(it already checks that the cert tests ran, which it does depend on).

bob

Comment 36

[Robert Relyea]

Attachment: OK, this patch should have everything, including nelson's review comment.s

Comment 37

[Robert Relyea]

Comment on attachment 197888 [details] [diff] [review]
OK, this patch should have everything, including nelson's review comment.s

Nope, wrong patch.

Comment 38

[Robert Relyea]

Attachment: crmf patch with crmf.sh and updates from nelson's comments

Comment 39

[Robert Relyea]

Attachment: crmf patch with crmf.sh and updates from nelson's comments

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197890 [details] [diff] [review]
crmf patch with crmf.sh and updates from nelson's comments

r=nelson, with the following observations:

>+	case 'P':
>+	    password = PORT_Strdup(optstate->value);
>+	    if (password == NULL) {
>+	        printf ("-P  failed\n");
>+	        return 606;
>+	    }
>+	    PArg = PR_TRUE;
>+	    break;

This will crash if optstate->value is NULL.  
Even though we pass "P:" to the parsing function, that function
does not require that the argument be present, so optstate->value
can be NULL.  All the cases in this function that attempt to 
strdup the argument have this same problem.  I think there's a 
separate bug filed that reports this problem for many NSS utility 
commands.  This program should get fixed with that bug is fixed.

Comment 41

[Robert Relyea]

Bug 310499 created for crmftest argument issue.

Comment 42

[Wan-Teh Chang]

This bug has been fixed by the patch in bug 244922.

Firefox 1.5 Beta 2 (which is still using NSS 3.10)
has this bug.  If I drop in NSS 3.10.2 Beta 2 DLLs,
it works (I followed Chandra's instructions in comment 3).

We are trying to get NSS 3.10.2 into Firefox 1.5 RC1
(bug 311402).

Comment 43

[Wan-Teh Chang]

Chandra, you can verify this bug by dropping in
NSS 3.10.2 Beta 2 DLLs/shared libraries from
/share/builds/verification/nss/NSS_3_10_2_BETA2.
For Windows, cd into WIN954.0_OPT.OBJ/lib.
For RHEL 4, cd into Linux2.6_x86_glibc_PTH_OPT.OBJ/lib.

Get these files:
nss3.dll/libnss3.so
nssckbi.dll/libnssckbi.so
softokn3.dll/libsoftokn3.so
softokn3.chk/libsoftokn3.chk
smime3.dll/libsmime3.so
ssl3.dll/libssl3.so

Comment 44

[Chandrasekar Kannan]

verified on : oct 17,2005 

builds used: 

(1) firefox from :
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/
(2) thunderbird from : 
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-mozilla1.8/

platforms: win2000 and RHEL 4

verified the following:

(1) crmf cert request generation  and approval by a CA(redhat internal) - PASS
(2) pkcs10 cert request generation and approval by a CA(redhat internal) - PASS
(3) import certificate generated in step 1 into browser - PASS
(4) backup certificate imported in step 3 as a .p12 file - PASS
(5) delete cert from browser and re-import the .p12 file - PASS
(6) send/receive "signed and encrypted email" and verify
digital signature on received email - PASS

Comment 45

[Wan-Teh Chang]

Thank you very much, Chandra!