193367 - NSS stores too many certs from an S/MIME message

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Robert Relyea]

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]

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]

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]

Attachment: 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

Comment 4

[Robert Relyea]

taking bug... moving to 3.7.2 target.

Comment 5

[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?

Comment 6

[Robert Relyea]

re keepcert == TRUE..

Good point.

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

bob

Comment 7

[Robert Relyea]

Attachment: Updated patch with Ian's comments included.

Comment 8

[Wan-Teh Chang]

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]

Attachment: 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]

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)]

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.  

Comment 12

[Robert Relyea]

Fix has been checked into the 3.7 branch and the tip.

Comment 13

[Kai Engert [:KaiE:]]

I think this change is too restrictive. I filed bug 209166 to suggest relaxing
the logic.