54014 - S/MIME importing certs into tempdb

Status: RESOLVED DUPLICATE of bug 298538

(NSS :: Libraries, defect, P3)

Description

[Ian McGreer]

cmsutil.c:465 has call to CERT_ImportCerts with boolean keepCerts and NULL array
for retCerts.  If keepCerts == PR_FALSE, new temp certs are created, but then
deleted since retCerts == NULL.  Thus this call only works if keepCerts ==
PR_TRUE.  In the PR_FALSE case, retCerts should dump the values into
sigd->certs.

Comment 1

[Ian McGreer]

moving to 3.3

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 4

[Wan-Teh Chang]

Ian, does this bug still apply to the 3.4+ world?

Comment 5

[Ian McGreer]

Yes, I believe this is a still a bug.  See the workaround here:

http://lxr.mozilla.org/security/source/security/nss/cmd/smimetools/cmsutil.c#303

Comment 6

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Ian, 

I'm trying to understand the "workaround" code you put into cmsutil.c
rev 1.7 for this bug.  I wonder if we can back out that workaround now.

I found the source code (as it was back in September 2000) doesn't match 
the original description above, but I think I've managed to sort it out.

Ian, Please confirm (or correct) each of the following details:

1. The call to CERT_ImportCerts described in the original description above
   is not in nss/cmd/smimetools/cmsutil.c:465, but rather is in 
   NSS_CMSSignedData_ImportCerts in nss/lib/smime/cmssigdata.c at line 465 
   (in rev 1.9, which was current when this bug was filed).
   This may be seen at this URL:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/smime/cmssigdata.c&rev=1.9

2. NSS_CMSSignedData_ImportCerts was called from 
   nss/cmd/smimetools/cmsutil.c in function decode at line 290 
   in rev 1.6 (which was the current rev in September 2000).
   That code may be seen at this URL:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/smimetools/cmsutil.c&rev=&cvsroot=/cvsroot&rev=1.6

3. The workaround described in comment 5 above was in rev 1.7 to cmsutil.c
   which may be seen at 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/smimetools/cmsutil.c&rev=&cvsroot=/cvsroot&rev=1.7

   The diffs may be seen here:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/cmd/smimetools&command=DIFF_FRAMESET&file=cmsutil.c&rev2=1.7&rev1=1.6

4. The "workaround" was to call NSS_CMSSignedData_ImportCerts with 
   keepCerts=PR_TRUE, causing the certs to be permanently added to the 
   cert DB. This was apparently so that the certs would remain in memory,
   so that the subsequent (new) call to NSS_CMSSignedData_VerifyCertsOnly 
   would actually find those certs.  

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/cmd/smimetools&command=DIFF_FRAMESET&file=cmsutil.c&rev2=1.7&rev1=1.6

5. The problem described above (certs not actually being imported/kept when 
   keepcerts is PR_FALSE) appears to have been fixed by rev 1.16 of 
   nss/lib/smime/cmssigdata.c . The diffs may be seen at

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=cmssigdata.c&branch=&root=/cvsroot&subdir=mozilla/security/nss/lib/smime&command=DIFF_FRAMESET&rev1=1.15&rev2=1.16

Questions:

a) Do you agree that the problem originally described is now fixed in 
   NSS_CMSSignedData_ImportCerts ?

b) Do you agree that we can now change the keepCerts argument back to PR_FALSE
   in the call to NSS_CMSSignedData_ImportCerts in cmd/smimetools/cmsutil.c
   just below the comment 
       /* XXX workaround for bug #54014 */ 

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Hmm.  Please forget question b above, and consider this question instead:

c) Can we just eliminate the call to NSS_CMSSignedData_ImportCerts that
   immediately preceeds the call to NSS_CMSSignedData_VerifyCertsOnly,
   since there is another call to NSS_CMSSignedData_ImportCerts within the
   preceeding 20 lines?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

One more question:

d) do we have a QA regression test case for these "certs only" "cert transport"
   messages, perhaps in all.sh ??

Comment 11

[Julien Pierre]

*** This bug has been marked as a duplicate of 298538 ***