240668 - JAR signature certs should not be stored in certificate store

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 2004-04-10-08 Mozilla.org Linux nightly

STEPS TO REPRODUCE:
1)  Load https://secure.arcamax.com/moztest/amirootca.cacert
2)  Accept that CA cert and allow it to sign software
3)  Load https://secure.arcamax.com/moztest/moztest2.jar (may not stick around
    for long).
4)  Deny the privilege request (this part is not actually relevant to the bug,
    but it's safer this way).
5)  Go to Edit > Preferences > Privacy and Security > Certificates
6)  Click "Manage Certificates"
7)  Switch to "Web Sites" tab in the dialog
8)  Look for certificates under "arcamax.com"

EXPECTED RESULTS: nothing there
ACTUAL RESULTS: the certificate used to sign the jar is present

The problem seems to be that nsNSSComponent::VerifySignature passes PR_TRUE for
the "keepcerts" argument of SEC_PKCS7VerifyDetachedSignature.  The only users of
this function are nsJAR, the CertReader in xpinstall, and
nsJVMManager::IsAppletTrusted.  So we should either be passing PR_FALSE or push
the decision up to the callers of the nsISignatureVerifier interface (and change
those three callers to pass PR_FALSE).

Comment 1

[Boris Zbarsky [:bzbarsky]]

So... I'm willing to do the work of fixing this bug if someone who can claim to
be an owner for the NSS interfaces actually tells me what the
nsISignatureVerifier interface is supposed to represent (and hence which of the
two proposed solutions would be preferable).

Comment 2

[Darin Fisher]

I've only ever made use of nsISignatureVerifier to compute an MD5 hash. 
Unfortunately, the verifySignature method is not documented :(

It'd be nice if mstoltz or ddrinan were still around since they have CVS blame
for that interface.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Are the "expected" and "actual" results switched in the description above?
This bug seems to complain tht the certs are not listed among the stored
certs, yet the proposed fix seems to have the opposite effect, of ensuring
that they will not be stored.  So, it's not clear to me what the real 
problem is, and what fix is really desired.

In general, applications that verify signatures on various random signed
things should not be routinely adding the signature-verification certs 
which accompany those signatures to the local cert store.  Email is one 
of the few exceptions, and in that case, only encryption certs, not 
signature certs should be added.  

Can someone here please add a comment with an LXR URL for the code that
is calling the PKCS7 API with keepcerts == true?

Also, the old PKCS7 API was superseded long ago with the new CMS API,
which (IINM) is now used for email.  If the cited instance is one of 
the last remaining calls to the old deprecated PKCS7 API, perhaps now
is the time to complete the conversion to the new API.  

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Are the "expected" and "actual" results switched in the description above?

No.

> This bug seems to complain tht the certs are not listed

No, the bug complains that they _are_ listed.

The problem is that random certificates that are used to sign code are
automatically added to the certificate store.  When I pointed this out to you
back in April, you said that was wrong and that I should file a bug.  This is
that bug.

> Can someone here please add a comment with an LXR URL 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.118#1438

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I'll look into switching to the new API; whether we do that or not
doesn't affect the question of whether VerifySignature() should let the caller
decide whethe to store the signing cert, or whether it should unconditionally
not store it.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

IMO, none of the 3 uses of this function documented above (nsJAR, the 
CertReader in xpinstall, and nsJVMManager::IsAppletTrusted) should be 
"keeping" the certs they decode from the signatures.  So, you could just 
change the keepcerts argument to false.  

If you anticipate that this function will someday be used for applications 
like email, which frequently convey both signing certs and encrytpion certs, 
with the expectation of receiving an encrypted reply, then you might choose 
to burden the callers with deciding whether or not to keep the certs.  

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Fix for this bug.

I'll file a separate bug on switching to the CMS api; it's not immediately
clear to me how to map the SEC_PKCS7DecryptionAllowedCallback arg into it....

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 174387 [details] [diff] [review]
Patch

sr=shaver, but why the rewrapping?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... it was being too wide for my editor, but that seems to be set at 79
chars.  I'll undo that before landing.

Comment 10

[Wan-Teh Chang]

Comment on attachment 174387 [details] [diff] [review]
Patch

I agree with what Nelson said in the second
paragraph of comment 3.

Wrapping lines longer than 80 characters is
fine.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Bug 283099 filed on the switch to CMS.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk for 1.8b2.