445871 - SSL certificates added as exceptions should never be treated as EV

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P2)

Description

[David Thatcher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

When you create an exception for an invalid certificate (using a certificate for one domain on another), if it is EV SSL, when you click on the green area in the toolbar it does not tell you an exception has been made.

Reproducible: Always

Steps to Reproduce:
1. Go to a website that uses an EV SSL certificate for a different domain
2. Grant the exception
3. Click on the green area
Actual Results:  
The exception I set was not noted

Expected Results:  
It should have noted the exception

EV SSL certificates that are used on a different domain that which they are intended completely invalidate the whole purpose of EV SSL. For this reason, if a manual exception is made for an EV SSL certificate, the favicon area on the toolbar should stay blue despite the fact that an EV SSL cert is being used, and this and the exception should be noted in the popup that shows when you click it.

Comment 1

[Johnathan Nightingale [:johnath]]

I agree with the statements in this bug - that our indicator should not reflect EV status for a mismatched domain, even with exception - and that is our design intent as well, but I was under the impression that that was also how we behaved.  Do you have an example of such a site, for testing purposes?

I tried visiting https://64.4.241.129/ (paypal's site, by IP address) which did cause me to add the exception, and did subsequently show me the EV indicators, but only because the (now-trusted, and blue-buttoned) IP-address site redirected me to the (valid-EV-certificate-holding) https://www.paypal.com.  I do not have a problem with that behaviour - trusted sites (by CA or security exception) are allowed to redirect to other sites, which may or may not have their own EV cert.  But a site which actually presents the mismatched cert with exception should not get EV treatment.

Comment 2

[David Thatcher]

I am sending you the site I am testing on by email since the site is not for public use.

Comment 3

[Johnathan Nightingale [:johnath]]

I can confirm the behaviour David describes, and am hoping to get permission from him to include the information in this bug, but mark it security-sensitive so that his private site is not publicized.

Comment 5

[Daniel Veditz [:dveditz]]

This is surprising to me, it was supposed to be fixed in bug 406999 (and dupe 406034). Definitely a bug.

Comment 6

[Kai Engert [:KaiE:]]

I agree, sounds bad. Will look into it asap.

Comment 7

[Kai Engert [:KaiE:]]

I'm able to reproduce and have started to search for the cause. I'll need a bit more time, it appears to be erratic.

Comment 8

[Daniel Veditz [:dveditz]]

This isn't going to make the code-freeze today -- no time for reasonable QA verification on trunk. Code-freeze for 1.9.0.5 will be Nov 17 but please please please don't wait until the end to try to get this in.

Comment 9

[Samuel Sidler (old account; do not CC)]

Kai, any update here? It's missed two releases so far, this would be the third...

Comment 10

[Daniel Veditz [:dveditz]]

Rather than try to push from the branch side, we'll take it when it lands on 1.9.1 for the quickly approaching Firefox 3.1 release.

Comment 11

[Johnathan Nightingale [:johnath]]

Copying Honza as well - if this is still reproducable, it's a 1.9.1 blocker in need of a patch.

Comment 12

[Honza Bambas (:mayhemer)]

Hmm... I did try the following with FF 3.6 (trunk) and 3.1b3:
- set date to 4/1/2011
- connected to addons.mozilla.org
- the date change made the ssl addons certificate invalid, but all CAs were still valid
- added an exception
- got blue (no EV) UI and a hint telling me that I have added an exception for this site
- I got it even I only add an exception manually and the cert is otherwise ok

So, if this test is sufficient I could say that it has already been fixed.

Comment 13

[Johnathan Nightingale [:johnath]]

Yeah, your steps work for me too, but the steps in comment 4 still cause the problem for me, so it looks like there's still an edge case hiding somewhere.

Comment 14

[Honza Bambas (:mayhemer)]

Ok, but there is no comment 4 in this bug!

Comment 15

[Johnathan Nightingale [:johnath]]

Honza - I'll find you on IRC or email and give you the contents of that comment, it was marked private by request of the reporter.

Comment 16

[Honza Bambas (:mayhemer)]

We are not setting mHaveCertErrorBits on nsSSLStatus object. That should be set for mismatched domain name. That flag prevents EV UI. Investigating further.

Comment 17

[Honza Bambas (:mayhemer)]

Clearing SSL session cache helps. Switching FF offline and online doesn't.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1

EV UI is dropped when there is status->mHaveCertErrorBits set. We set this flag in nsNSSBadCertHandler. But, the ssl session might be reused and nss then consider the connections as ok, we don't get call to nsNSSBadCertHandler while ssl status objects are not reused but newly allocated -> the information about cert error is lost.

The patch stores hosts where certificate is not ok to a hashset. In HandshakeCallback we check presence of the host in that hashset, if it's contained set the status->mHaveCertErrorBits flag.

I wanted to use mapping by session id that would be a bit more reliable but in AuthCertificateCallback fd has not yet a session id.

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Changed to a proper deallocator.

Comment 20

[Honza Bambas (:mayhemer)]

We should probably use mapping by host + port here.

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 371069 [details] [diff] [review]
v1.1

Honza, thanks for working on this bug.

Are you sure that your hashset will get accessed by a single thread, only?

There is a dependency between flag "mHaveCertErrorBits" and the individual state variables
- mIsDomainMismatch
- mIsNotValidAtThisTime
- mIsUntrusted

Variable "mHaveCertErrorBits" has the meaning: "there is an error, the error bits are known and the contents of the individual 3 variables have been set".

I should have made this dependency clear when writing the original code, by using setter/getter functions that keep the flag and the other 3 variables in synch.

In your patch you only set the "status is known, we have error bits", but you leave the individual 3 variables unchanged, they will probably remain at the default "no error".

This mismatch worries me. Maybe this mismatch is unexpected for some code.

I think the use of variable mHaveCertErrorBits is general, not used for EV only. But you use it for a EV only workaround.

Can you either ensure the flags get set correctly (or remembered in the hashset) - or - be pessimistic and set all 3 bools to "true" meaning "error"?

Maybe the latter is sufficient (setting all to true meaning error).


+    // Set the error bit here to prevent EV UI
+    if (rv != SECSuccess)
+      status->mHaveCertErrorBits = PR_TRUE;
+
+    sHostsWithCertErrors.SetCertHasError(fd, rv);


I don't understand why you set errorbits=true only when rv is failure, but you always update the hashset to record a failure? Why? If this makes sense, please add a comment why.

Comment 22

[Kai Engert [:KaiE:]]

I agree that you must use host+port

Comment 23

[Kai Engert [:KaiE:]]

It's great that you have found what's the cause of the bug.

But I have too many concerns that your patch is sufficient or doesn't have side effects.

I would prefer a different patch.
The problem is, as you say, with SSL session reuse, if we have allowed SSL connections to continue, we won't get another callback from NSS into nsNSSBadCertHandler.

I would prefer to update your hashset inside nsNSSBadCertHandler at the time you learn there is a cert failure. Use host+port as the key. Store the values of all 3 error flags (implies haveerror), or clear the hashset from this host+port if there is no longer an error.

In the other callback, the one we always get, check the hashset. If there is an entry, set haveerror=true and set the 3 individual flags. If there is no entry, set haveerror=false (or leave at the default=no error).

Does this make sense?

Comment 24

[Honza Bambas (:mayhemer)]

Attachment: v2

A bit more complicated but better patch. I remember bits in nsNSSBadCertHandler. Retrieving in both HandshakeCallback and AuthCertificateCallback as before. I clear the record in AuthCertificateCallback when verification is OK. Mapping is host:port now.

Comment 25

[Johnathan Nightingale [:johnath]]

This is a Firefox 3.5 blocker (though not a beta 4 blocker). Kai, will you be able to get to this review soon, or should I send Bob R a flower basket of some kind?  :)

Comment 26

[Kai Engert [:KaiE:]]

You did not address my question "single thread only" from comment 21. I think access to sHostsWithCertErrors must be serialized using some mutex
(unless you convince me there is no need)

We will soon have multiple SSL threads.
If you prefer, you may file a separate bug for adding this protection and make it block 390036.

Comment 27

[Kai Engert [:KaiE:]]

Comment on attachment 372684 [details] [diff] [review]
v2

Patch looks good. I'd like to ask for trivial changes only:


Typo: Please rename RemeberCertHasError to RememberCertHasError (add "m").


Please rename function UpdateCertErrorBits to LookupCertErrorBits.

This makes it clear that you are "reading from remembered data" (while the term update sounds like you are storing something). I was initially confused when I read your patch.


Please add a comment to function LookupCertErrorBits saying:
// Get remembered error bits from our cache, because of SSL session caching
// the NSS library potentially hasn't notified us for this socket.


In function GetHostPortKey please call Truncate before you add the result.


r=kaie if you address the above requests and file the bug mentioned in comment 26 and the bug below:


Note on bloat:
You never remove any entries for visited sites.
But the number of sites with cert errors visited
Please file an enhancement bug, that suggests to expire map entries that haven't been used for a while.

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v2.1 [Checkin mozilla-central comment 29]

(In reply to comment #26)
> You did not address my question "single thread only" from comment 21. I think
> access to sHostsWithCertErrors must be serialized using some mutex
> (unless you convince me there is no need)
> 

I'm using MT hash table (multithread). As there is no "GetOrCreate" method doing if (!table.get()) {new obj; table.put(obj)} it's IMO sufficient. I forgot to mention that in attachment comment.

(In reply to comment #27)
> Note on bloat:
> You never remove any entries for visited sites.
> But the number of sites with cert errors visited
> Please file an enhancement bug, that suggests to expire map entries that
> haven't been used for a while.

Will do it.

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 376424 [details] [diff] [review]
v2.1 [Checkin mozilla-central comment 29]

http://hg.mozilla.org/mozilla-central/rev/da613c9fae8c

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

I backed this out due to leaks on the unit test boxes after it landed
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242208481.1242213526.26757.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242208481.1242211617.23864.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242208481.1242215875.29624.gz

Comment 31

[Honza Bambas (:mayhemer)]

Brad, thanks. Just wanted to do it myself.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Some quick notes:

1)  nsPSMRememberCertErrorsTable::GetHostPortKey has a mismatched allocator
    (nsCRT::free vs NS_strdup).  It should use NS_free, or better yet use an
    nsXPIDLCString and stop leaking if GetPort() fails.
2)  mErrorHosts needs to be cleared (and ideally deleted) on xpcom-shutdown if
    you don't want to leak.  And please repeat after me: do NOT use static
    instances of a class with a nontrivial destructor. Just don't.  We have a
    rule about that.

Comment 33

[Honza Bambas (:mayhemer)]

Attachment: v3

Replacing the wrong patch. I made the table a static member pointer of nsNSSIOLayer class that keeps some similar stuff and has Init and Cleanup methods. Also using xpidl string to get rid of the mismatched deallocator.

Comment 34

[Boris Zbarsky [:bzbarsky]]

No need for the Clean() method, is there?

>+  mHostsWithCertErrors = new nsPSMRememberCertErrorsTable();
>+  if (mHostsWithCertErrors)
>+    return NS_ERROR_OUT_OF_MEMORY;

That check is backwards.

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to comment #34)
> No need for the Clean() method, is there?
> 

You suggested to do this manually, but true is that the hash table is wiped out and all freed when it's deleted. I'll remove it.

> >+  mHostsWithCertErrors = new nsPSMRememberCertErrorsTable();
> >+  if (mHostsWithCertErrors)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> 
> That check is backwards.

Yes, typo, it's well visible that the result value is not checked. I'll fix that too.

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

Comment 37

[Boris Zbarsky [:bzbarsky]]

This should probably get review from kai; I'm happy to sr after that...

Comment 38

[Honza Bambas (:mayhemer)]

It already got r+. The system of the bug fix is the same as in v2(.1) of the patch that was r+'ed from Kai, comment 27 and comment 28.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Yes, but you're adding static members to various nss classes; I'm not qualified to review whether they're the right classes...

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 377241 [details] [diff] [review]
v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

Really does need module owner review.  I'd also love an interdiff; I can try to review without, but it'd take me a good deal longer to do it...

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 377241 [details] [diff] [review]
v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

>* * *

Nix that part of the checkin comment.

>+++ b/security/manager/ssl/src/nsNSSCallbacks.cpp

>+nsPSMRememberCertErrorsTable::GetHostPortKey(nsNSSSocketInfo* infoObject,
>+  result.Assign(hostName.get());

You don't need the .get()

>@@ -1980,20 +2075,24 @@ nsresult nsSSLIOLayerHelpers::Init()
>+  mHostsWithCertErrors = new nsPSMRememberCertErrorsTable();
>+  if (!mHostsWithCertErrors)
>+    return NS_ERROR_OUT_OF_MEMORY;

That check should be:

  if (!mHostsWithCertErrors || !mHostsWithCertErrors->mErrorHosts.IsInitialized())

or some equivalent thereof, no?

sr=bzbarsky with those nits, but Kai really does need to ok this.

Comment 42

[Kai Engert [:KaiE:]]

Comment on attachment 377241 [details] [diff] [review]
v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

r=kaie

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 377241 [details] [diff] [review]
v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

http://hg.mozilla.org/mozilla-central/rev/752810cce461

Comment 44

[Honza Bambas (:mayhemer)]

Comment on attachment 377241 [details] [diff] [review]
v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/97c0d63fdb5d

Comment 45

[u279076]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre

Using the steps in comment #4, I can no longer reproduce this behaviour.  Marking VERIFIED.