Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 193367 - NSS stores too many certs from an S/MIME message
: NSS stores too many certs from an S/MIME message
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.7
: All All
: P2 normal (vote)
: 3.7.2
Assigned To: Robert Relyea
: Bishakha Banerjee
Depends on:
  Show dependency treegraph
Reported: 2003-02-14 10:31 PST by Robert Relyea
Modified: 2003-06-12 06:35 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Only save Certs of the requested certusage from email. Only save them if the verify. (2.99 KB, patch)
2003-02-14 10:42 PST, Robert Relyea
bugz: review-
Details | Diff | Splinter Review
Updated patch with Ian's comments included. (3.15 KB, patch)
2003-02-14 16:02 PST, Robert Relyea
wtc: review+
Details | Diff | Splinter Review
Do not call PR_Now() in a loop (1.65 KB, patch)
2003-02-17 21:16 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review

Description Robert Relyea 2003-02-14 10:31:05 PST
NSS blindly stores all the certs it receives from any S/MIME message. It should
only store any validated Encryption certs and their chains.
Comment 1 Robert Relyea 2003-02-14 10:38:45 PST

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.
Comment 2 Robert Relyea 2003-02-14 10:39:14 PST
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).

Comment 3 Robert Relyea 2003-02-14 10:42:41 PST
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
(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.

Comment 4 Robert Relyea 2003-02-14 10:44:10 PST
taking bug... moving to 3.7.2 target.
Comment 5 Ian McGreer 2003-02-14 12:29:56 PST
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?
Comment 6 Robert Relyea 2003-02-14 13:20:25 PST
re keepcert == TRUE..

Good point.

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

Comment 7 Robert Relyea 2003-02-14 16:02:08 PST
Created attachment 114499 [details] [diff] [review]
Updated patch with Ian's comments included.
Comment 8 Wan-Teh Chang 2003-02-14 16:21:11 PST
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.
Comment 9 Wan-Teh Chang 2003-02-17 21:16:36 PST
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 Wan-Teh Chang 2003-02-17 21:24:19 PST
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.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2003-02-18 14:04:33 PST
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
Comment 12 Robert Relyea 2003-03-04 16:21:41 PST
Fix has been checked into the 3.7 branch and the tip.
Comment 13 Kai Engert (:kaie) 2003-06-12 06:35:09 PDT
I think this change is too restrictive. I filed bug 209166 to suggest relaxing
the logic.

Note You need to log in before you can comment on or make changes to this bug.