The default bug view has changed. See this FAQ.

NSS stores too many certs from an S/MIME message

RESOLVED FIXED in 3.7.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

14 years ago

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.
(Assignee)

Comment 2

14 years ago
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).

(Assignee)

Comment 3

14 years ago
Created attachment 114455 [details] [diff] [review]
Only save Certs of the requested certusage from email. Only save them if the verify.

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

The patch also only saves these certs if they verify valid 'now'. This
prevents:
  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.

bob
(Assignee)

Comment 4

14 years ago
taking bug... moving to 3.7.2 target.
Assignee: wtc → relyea
Priority: -- → P2
Target Milestone: --- → 3.7.2

Updated

14 years ago
Attachment #114455 - Flags: review?(ian.mcgreer)

Comment 5

14 years ago
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-
(Assignee)

Comment 6

14 years ago
re keepcert == TRUE..

Good point.

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

bob
(Assignee)

Comment 7

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

Comment 8

14 years ago
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+

Comment 9

14 years ago
Created attachment 114753 [details] [diff] [review]
Do not call PR_Now() in a loop

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 10

14 years ago
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.

Updated

14 years ago
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
NSPR2.  
(Assignee)

Comment 12

14 years ago
Fix has been checked into the 3.7 branch and the tip.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Attachment #114499 - Flags: superreview?(ian.mcgreer)

Comment 13

14 years ago
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.