Last Comment Bug 333767 - nsNSSCertificateDB::AddCertFromBase64 mishandles trust settings
: nsNSSCertificateDB::AddCertFromBase64 mishandles trust settings
Status: RESOLVED FIXED
[kerh-coa]
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks: 391575
  Show dependency treegraph
 
Reported: 2006-04-12 13:46 PDT by Robert Basch
Modified: 2007-08-09 13:31 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to remove calls to SetValidCA and AddCATrust (760 bytes, patch)
2006-04-12 13:54 PDT, Robert Basch
kaie: review+
darin.moz: review+
kaie: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review

Description Robert Basch 2006-04-12 13:46:04 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 (CK-MIT) Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060124 (CK-MIT) Firefox/1.5.0.1

We are using nsNSSCertificateDB::AddCertFromBase64() to load a CA certificate
(from a Firefox extension, via the nsIX509CertDB2 interface).  We pass a trust
string argument of "C,c,c", i.e. to set trust for SSL, but not for email or
object-signing.  But regardless of the given trust string passed, this function
seems always to set trust for all three fields, i.e. as if "C,C,C" were given.

From an examination of the source code, it seems that the function's calling
of the SetValidCA and AddCATrust trust methods is incorrect.  The SetValidCA
call clears all of the trust bits, which have already been decoded from the
given string.  And the AddCATrust call will set TRUSTED_CA (and TRUSTED_CLIENT_CA) for all three fields -- note that AddCATrust's arguments
are booleans, and AddCertFromBase64 passes the entire flag fields, which
will be non-0 resulting from the call to SetValidCA.

The simplest fix appears to be to simply remove the calls to SetValidCA and
AddCATrust, i.e. to set the trust bits exactly as given by the caller.  The
caveat here would be if the intent is to ensure that VALID_CA is always set,
regardless of what the caller passes; if that is the goal, a more complicated
fix seems to be needed, as there does not seem to be a clean way to turn on
just that bit.

Reproducible: Always

Steps to Reproduce:
1. Call the addCertFromBase64 method of the nsIX509CertDB2 interface, supplying
   a valid CA certificate in Base64 format, and a trust string of "C,c,c".
2. In Firefox, use the Certificate Manager to examine the certificate.
3.

Actual Results:  
The CA certificate is stored with trust set for SSL, email, and object-signing.

Expected Results:  
For a given trust string of "C,c,c", the certificate should be set to be trusted
for SSL only, not email or object-signing.
Comment 1 Robert Basch 2006-04-12 13:54:32 PDT
Created attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

As noted in the original Description, this fix is only correct if we are to set
the trust bits to correspond exactly to the caller's given string.  If this code
wants to enforce that VALID_CA is always set, regardless of the trust string
argument, then a more complicated fix is needed.
Comment 2 Daniel Veditz [:dveditz] 2006-04-12 16:30:29 PDT
I defer to Kai on this one
Comment 3 Kai Engert (:kaie) 2006-04-13 05:51:20 PDT
nsNSSCertificateDB::AddCertFromBase64 was added with bug 221329, and according to 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp&rev=1.21
it has not been changed since that time.

I think it's reasonable to assume, the intent has been to set trust according to the given trust string.

The call to SetValidCA() is ok, because it sets the flag "this is a valid CA, regardless whether it is trusted or not".

The following call to AddCATrust is not necessary, and is actually used incorrectly, and that is the bug.

AddCATrust takes bools:
  void AddCATrust(PRBool ssl, PRBool email, PRBool objSign);

The boolean value should only be set to true, if the trust is to be enabled.

However, the code incorrectly passes in the trust bitmask, which can be true for many reasons. The fact that we just enabled the "valid CA" flag causes it to be true, and the incorrect calls adds the trust.


So, I have to agree, your patch can be considered to be correct.

As we have already used NSS to decode the trust string to trust bits, why fiddle with the trust bits more? It does not seem necessary.

And why call SetValidCA, if the interface definition seems to be open for all kinds of certs, not just CA's?


I'm going to approve your patch, but I'm cc'ding David and Nelson, so they could veto, if my understanding is wrong.
Comment 4 Robert Basch 2006-04-13 06:23:19 PDT
(In reply to comment #3)

> The call to SetValidCA() is ok, because it sets the flag "this is a valid CA,
> regardless whether it is trusted or not".

Note that it also clears all of the other bits, thus losing the results from
decoding the given trust string.  So both the calls to it and to AddCATrust()
need to be removed for this to function properly.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2006-04-13 11:33:51 PDT
I'm missing a lot of context here.  I'd hope this function cannot be used by
ordinary javascript in just any old web page to add truted certs to a user's 
cert db.  But apart from that sort of concern (which this patch doesn't address)
this patch looks right to me.
Comment 6 Kai Engert (:kaie) 2006-04-13 11:43:48 PDT
This function can not be accessed by a web page, but only code running within the application. However, add-on software like "extensions", that the user decides to install on his own risk, are able to call to this function, as well as all other internal interfaces.
Comment 7 Mike Kaply [:mkaply] (Out June 27-July 5) 2006-04-14 08:45:55 PDT
I have investigate this very thoroughly for the CCK and I definitely agree with Robert here.

These calls should simply be removed.

The only consumers of this API are the MCD and CCK.

In both these cases, a trust string is passed in, and this should be used as the trust string. 

We should not be modifying the passed in trust string.
Comment 8 Kai Engert (:kaie) 2006-04-14 09:33:31 PDT
Comment on attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

ok, r=kengert
Comment 9 Mike Kaply [:mkaply] (Out June 27-July 5) 2006-04-14 09:40:12 PDT
We would need someone from NSS to check this in.

I'd also like this on the branch (which I can do once I get approval)
Comment 10 Kai Engert (:kaie) 2006-04-14 09:52:48 PDT
We need a second review from either a superreviewer or a PSM peer.

This is not NSS code, but PSM. I can do the checkin, once we have the second review. I will give branch approval when we have the review.
Comment 11 Darin Fisher 2006-04-17 14:50:47 PDT
Comment on attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

r=darin (after reading the comments here and inspecting the code, i agree that this seems like the right change)

rrelyea, please jump in if you think there's something we're missing.
Comment 12 Kai Engert (:kaie) 2006-04-20 09:22:57 PDT
checked in
Comment 13 Mike Kaply [:mkaply] (Out June 27-July 5) 2006-04-20 09:30:41 PDT
Comment on attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

I'd like this on 1.8.0.3 - this only affects CCK and MCD.
Comment 14 Daniel Veditz [:dveditz] 2006-04-21 14:28:35 PDT
Comment on attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

approved for 1.8.0 branch, a=dveditz for drivers
Comment 15 Kai Engert (:kaie) 2006-04-24 09:34:30 PDT
I don't have a 1_8_0 tree at the moment, Dan or Mike, would you like to check it in? Let me know if I should do it. Thanks.
Comment 16 Kai Engert (:kaie) 2006-04-24 09:43:32 PDT
Never mind, I'll need a tree anyway, to check in bug 321598.
I can check it in later today.

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