333767 - nsNSSCertificateDB::AddCertFromBase64 mishandles trust settings

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Robert Basch]

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]

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

I defer to Kai on this one

Comment 3

[Kai Engert (:KaiE:)]

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]

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

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

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

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

Comment on attachment 218210 [details] [diff] [review]
Patch to remove calls to SetValidCA and AddCATrust

ok, r=kengert

Comment 9

[Mike Kaply [:mkaply]]

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

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]

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

checked in

Comment 13

[Mike Kaply [:mkaply]]

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

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

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

Never mind, I'll need a tree anyway, to check in bug 321598.
I can check it in later today.