Closed Bug 240668 Opened 20 years ago Closed 20 years ago

JAR signature certs should not be stored in certificate store


(Core Graveyard :: Security: UI, defect)

Other Branch
Not set


(Not tracked)



(Reporter: bzbarsky, Assigned: bzbarsky)



(1 file)

BUILD: 2004-04-10-08 Linux nightly

1)  Load
2)  Accept that CA cert and allow it to sign software
3)  Load (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 ""

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).
Assignee: kaie → nobody
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).
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.
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.  
> Are the "expected" and "actual" results switched in the description above?


> 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

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.
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.  
Attached patch PatchSplinter Review
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....
Attachment #174387 - Flags: superreview?(shaver)
Attachment #174387 - Flags: review?(jgmyers)
Comment on attachment 174387 [details] [diff] [review]

sr=shaver, but why the rewrapping?
Attachment #174387 - Flags: superreview?(shaver) → superreview+
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 on attachment 174387 [details] [diff] [review]

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

Wrapping lines longer than 80 characters is
Attachment #174387 - Flags: review?(jgmyers) → review+
Bug 283099 filed on the switch to CMS.
Assignee: nobody → bzbarsky
Fixed on trunk for 1.8b2.
Closed: 20 years ago
Resolution: --- → FIXED
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.