nsNSSCertificateDB::AddCertFromBase64 mishandles trust settings

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Robert Basch, Assigned: kaie)

Tracking

(Blocks: 1 bug, {fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Linux
fixed1.8.0.4, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-coa])

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
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.
I defer to Kai on this one
Assignee: dveditz → kengert
Status: UNCONFIRMED → NEW
Component: Security → Security: PSM
Ever confirmed: true
QA Contact: toolkit
(Assignee)

Comment 3

12 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: [kerh-coa]
(Reporter)

Comment 4

12 years ago
(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.
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.
(Assignee)

Comment 6

12 years ago
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

12 years ago
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.
(Assignee)

Comment 8

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

ok, r=kengert
Attachment #218210 - Flags: review+

Comment 9

12 years ago
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)
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #218210 - Flags: review?(rrelyea)
Attachment #218210 - Flags: approval-branch-1.8.1?(kengert)

Comment 11

12 years ago
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.
Attachment #218210 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

12 years ago
Attachment #218210 - Flags: approval-branch-1.8.1?(kengert) → approval-branch-1.8.1+
(Assignee)

Comment 12

12 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 13

12 years ago
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.
Attachment #218210 - Flags: approval1.8.0.3?
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
Attachment #218210 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 15

12 years ago
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.
(Assignee)

Comment 16

12 years ago
Never mind, I'll need a tree anyway, to check in bug 321598.
I can check it in later today.
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.0.3
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1
Blocks: 391575
You need to log in before you can comment on or make changes to this bug.