Closed Bug 193367 Opened 22 years ago Closed 22 years ago

NSS stores too many certs from an S/MIME message


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: rrelyea, Assigned: rrelyea)



(2 files, 1 obsolete file)

NSS blindly stores all the certs it receives from any S/MIME message. It should
only store any validated Encryption certs and their chains.

Apparently an NSS issue. Here's David's observations:

David Drinan wrote:

> After decoding the signed blob but before checking this signature, the certs 
are imported by making the following call:
>   if (NSS_CMSSignedData_ImportCerts(sigd, CERT_GetDefaultCertDB(), 
certUsageEmailRecipient, PR_TRUE) != SECSuccess) {
>     goto loser;
>   }
> Note that  we specify that only encryption certs are to be be stored i.e. 
cert usage is certUsageEmailRecipient. The above function calls 
CERT_ImportCerts. From my reading of this function, it appears to be ignoring 
the cert usage parameter and adding all certs to the perm db i.e. for each cert 
in the signed blob that is not a CA cert, it calls CERT_AddTempCertToPerm.
Ah it's a misunderstanding about the CERT_ImportCert function. The usage is in
that function is the expected usage of the imported certs, not the way to filter
the imported certs.

The NSS_CMSSignedData_ImportCerts needs to filter the certs before importing
them. Currently it looks like it imports all the certs (including CA's).

The attached patch only saves the certs with the requested usage and their
(in the typically case, only the encryption certs and their chains).

The patch also only saves these certs if they verify valid 'now'. This
  1) the poison cert syndrome (someone could construct a chain which a cert
which looks like a verisign intermediate cert and inject it into your database
by including it in an S/MIME message. They couldn't get you to trust new certs,

but they could prevent you from validating otherwise valid certs).
  2) loading expired certs form old email messages into your database.
  3) loading unwanted and extraneous certs that were included in the mail
message (like the signing certificate).

points 1 and 2 required the CertVerify call points 3 can be accomplished just
the filter and extractChain call.

Ian, please review.

taking bug... moving to 3.7.2 target.
Assignee: wtc → relyea
Priority: -- → P2
Target Milestone: --- → 3.7.2
Attachment #114455 - Flags: review?(ian.mcgreer)
Comment on attachment 114455 [details] [diff] [review]
Only save Certs of the requested certusage from email. Only save them if the verify.

The loop over the filtered certs should only be executed if keepcerts ==
PR_TRUE.  Otherwise, the certs are already in the temp db, so there's no need
to import again.  Maybe we should remove invalid certs from the temp db while
were at it?
Attachment #114455 - Flags: review?(ian.mcgreer) → review-
re keepcert == TRUE..

Good point.

As far as deleting certs, I don't think we should automatically delete any certs without 
user intervention.

Attachment #114455 - Attachment is obsolete: true
Comment on attachment 114499 [details] [diff] [review]
Updated patch with Ian's comments included.

r=wtc.	Ian, could you review the new patch?

Bob, you can go ahead and check this into the tip and 3.7 branch.
Attachment #114499 - Flags: superreview?(ian.mcgreer)
Attachment #114499 - Flags: review+
This patch should be applied on top of the previous
patch (attachment 114499 [details] [diff] [review]).

PR_Now() requires a system call on all platforms.
We don't need to call PR_Now() repeatedly in a loop.
We just need to call it once before entering the loop.

Question: is it okay to use the PRTime datatype in
NSS?  I saw that NSS uses int64 as the datatype for
time in most places.  I don't know if that's just
historic or we want to avoid the use of NSPR datatypes.
Comment on attachment 114499 [details] [diff] [review]
Updated patch with Ian's comments included.

This patch has another performance problem.  It allocates
and frees rawArray in a loop.  It is better to allocate
rawArray before the loop, enlarge it if it's not big enough
for certChain, and free it after the loop.  We can even
use a stack array if it is big enough.	What is the common
length of a cert chain?  I think cert chains are usually
quite short.

Another issue with this patch is that it ignores the
failures of CERT_CertChainFromCert and PORT_Alloc.  I
think the function should definitely fail if
CERT_CertChainFromCert or PORT_Alloc fails.
Attachment #114753 - Flags: review?(nelsonb)
Attachment #114753 - Flags: review?(nelsonb) → review+
Regarding your question in comment 9, it is OK to use PRTime in NSS. In fact, 
it's preferable, IMO.  The use of the int64 type comes from code that predates
Fix has been checked into the 3.7 branch and the tip.
Closed: 22 years ago
Resolution: --- → FIXED
Attachment #114499 - Flags: superreview?(ian.mcgreer)
I think this change is too restrictive. I filed bug 209166 to suggest relaxing
the logic.
You need to log in before you can comment on or make changes to this bug.