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.

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.

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

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.

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

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

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.

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.

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

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.

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

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.

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.

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

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.

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

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

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.

Changed to a proper deallocator.

We should probably use mapping by host + port here.

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.

I agree that you must use host+port

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?

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.

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

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

(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 on attachment 376424 [details] [diff] [review] v2.1 [Checkin mozilla-central comment 29] http://hg.mozilla.org/mozilla-central/rev/da613c9fae8c

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

Brad, thanks. Just wanted to do it myself.

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.

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.

No need for the Clean() method, is there? >+ mHostsWithCertErrors = new nsPSMRememberCertErrorsTable(); >+ if (mHostsWithCertErrors) >+ return NS_ERROR_OUT_OF_MEMORY; That check is backwards.

(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.

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

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.

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

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 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 on attachment 377241 [details] [diff] [review] v3.1 [Checkin comment 43] [Checkin comment 44 on 1.9.1] r=kaie

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

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.