Closed Bug 420246 Opened 15 years ago Closed 15 years ago

gBrowser.securityUI doesn't clear invalid cert data after using back button

Categories

(Core Graveyard :: Security: UI, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla-bugzilla-nospam, Assigned: KaiE)

References

()

Details

(Keywords: regression)

Attachments

(6 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre

The "View->Page info->Security" does not show the correct certificate for a web page after clicking 'back' from a page with invalid certificate. Even non-SSL sites are shown as "verified". 

Reproducible: Always

Steps to Reproduce:
1. Visit www.google.com
2. Visit a web site with an invalid certificate, get a "Secure Connection Failed" page
3. Click back button
4. Click "View page info" then Security.
Actual Results:  
Web site: www.google.com
Verified by: (name of (invalid) certificate issuer goes here)

"This web site provides a certificate to verify its identity".

Expected Results:  
"Verified by: Not specified" and no message about certificates being present.

I will attach screenshots showing the results of "View Page Information" before and after vising the site with the incorrect certificate.
This the expected result for "View Page info", which is the result of viewing page info in step 1
This is the (expected) error message when first visiting a web site with an invalid SSL certificate, as in step 2.
I can confirm this. Unsure if it's a problem with the page caching the old result or a bug in PSM.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
Version: unspecified → Trunk
This is a screenshot of the result of step 3+4. This is unexpected; instead I would expect something similar to attachment 306462 [details] (iow. like step 1).

Note there is a "View certificate" button which brings up the (invalid) certificate for the previous page.
I can *partially* confirm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022714 Minefiled/3.0b4pre and following steps:

1. Visit google.com.
2. Visit https://www.videolan.org/
3. Go back from error page to google
4. Do Tools -> Page Info

Actual results:
Owner: This web site does not supply identity information.
Verified by: VideoLAN  <--- bug here only
(In reply to comment #5)
> Actual results:
> Owner: This web site does not supply identity information.
> Verified by: VideoLAN  <--- bug here only

Yeah, that's what I see, too.
OS: Linux → All
Hardware: PC → All
Johnath: is this us or PSM? I figure if it's us, we're not invalidating/updating properly?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3
I've had a look and confirmed that when page info is opened after the STR in comment 5, gBrowser.securityUI is claiming that there's a (videolan) certificate.  For those following along at home, page info gets its security info here:

http://mxr.mozilla.org/mozilla/source/browser/base/content/pageinfo/security.js#66

which in turn is just extracting it from |window.opener.gBrowser.securityUI|, so I think this is over to PSM land (where that value is populated/updated).

I've also confirmed that this doesn't happen if you replace videolan.org with a site using a valid cert - that is, google.com->paypal->back-button does not end up claiming that google has paypal's cert.  Likewise, if you add the exception for videolan.org and then hit back, the cert does not persist.  So the problem seems specific to the combination of a blocked page load and back-button. 
Assignee: nobody → kengert
Component: Page Info → Security: UI
Flags: blocking-firefox3+
Product: Firefox → Core
QA Contact: page.info → ui
Summary: Page Info/Security shows information about SSL certificates from wrong domain after using back button → gBrowser.securityUI doesn't clear invalid cert data after using back button
Target Milestone: Firefox 3 → ---
Was blocking P2 - lost flag in component swap.  Re-nom'ng.
Flags: blocking1.9?
This regression was introduced when we worked on the add-exception dialog.

In the past (1.8) it was never possible to obtain a SSL status object for a rejected page.

But the solution we used for add-exception required us to have an SSL status object available (including cert, to be fetched by the add exception dialog) even when the SSL connection was rejected.

PSM would clear all its state tracking whenever nsIWebProgressListener is notified the combination of STATE_START | STATE_IS_REQUEST, loadFlags has nsIChannel::LOAD_DOCUMENT_URI and we learn this is a request for a top level document.


Shouldn't we get that notification on navigate back ??? I think we should, but we don't.


As a consequence, when you navigate back, there is no change in security state. The security tracking code is unaware that a new document is loaded and does not reset its state. The SSL status object from the rejected connection is still in memory. When someone (firefox UI) asks for it, it will be given out.


So, you could still blame PSM, because the state of the page is insecure and it this state it shouldn't give an SSL status object out. I agree and I'm proposing to make such a change to PSM.


But in addition, I think the UI code should be enhanced, too. It's wrong to assume "presence of SSL status object indicates a secure page". IMHO the UI code should check the state. Only if the state is secure should it attempt to make use of the SSL status object for the purpose of reporting a secure connection.
Keywords: regression
Attached patch Patch v1 (obsolete) — Splinter Review
This patch has
- the proposed change to PSM's tracking
- the proposed change to browser's decision
- an additional change to the browser code to switch to bitwise testing
- a page to PageInfoOverlay, equivalent to the browser change, this code might be used by SeaMonkey

Requesting Bob's review for the changes in security/manager.
Who wants to review the change in browser?
Attachment #306608 - Flags: review?(rrelyea)
Comment on attachment 306608 [details] [diff] [review]
Patch v1

sorry, attached wrong patch
Attachment #306608 - Attachment is obsolete: true
Attachment #306608 - Flags: review?(rrelyea)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #306609 - Flags: review?(rrelyea)
Flags: blocking1.9? → blocking1.9+
Does suite/browser/pageinfo/security.js need changing?
Attachment #306609 - Flags: review?(mano)
Comment on attachment 306609 [details] [diff] [review]
Patch v2

>Index: mozilla/browser/base/content/pageinfo/security.js
>     var isBroken =
>-      (ui.state == Components.interfaces.nsIWebProgressListener.STATE_IS_BROKEN);
>+      (ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_BROKEN);
>+    var isSecure = 
>+      (ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_SECURE);
>     var isEV =
>       (ui.state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL);
>     ui.QueryInterface(nsISSLStatusProvider);
>     var status = ui.SSLStatus;
> 
>-    if (status) {
>+    if (isSecure && status) {

After reading your comments, I had a very similar fix in mind, but was going to use

var isSSL = !(ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_INSECURE)

instead.  Your version of the change will result in Page Info no longer making the certificate available on mixed content, is that right?  If so, that is a change in behaviour from FF2.  I'm not sure I actually have too much of a problem with that - presenting it does send a sort of confusing message ("this site isn't identified, but does present a certificate to verify its identity"), but removing it takes away an ability users used to have.  Arguably, those users who want to view certificates may want to view them even on mixed content.

Was this deliberate?
Comment on attachment 306609 [details] [diff] [review]
Patch v2

r+ with the caveat that you and jonnathan agree on the proper semantic for security.js. (Also figure out if PageInfoOverlay.xul needs to be consistent with security.js as well).

bob
Attachment #306609 - Flags: review?(rrelyea) → review+
Attached patch Patch v3Splinter Review
Johnathan, thanks for your comment.
I think we should keep the old behavior until we have a chance to fix our mixed content story altogether (offering a chance to prevent loading insecure content, ability to detect insecure images, ...)

Your alternate proposal is better than mine.

I'm attaching a new patch.
Johnathan, if you think this is right, then you're ok to carry forward relyea's proposal.
Attachment #306609 - Attachment is obsolete: true
Attachment #307241 - Flags: review?(johnath)
Attachment #306609 - Flags: review?(mano)
Comment on attachment 307241 [details] [diff] [review]
Patch v3

restoring reed's request for mano's review
Attachment #307241 - Flags: review?(mano)
(In reply to comment #14)
> Does suite/browser/pageinfo/security.js need changing?

Yes, thanks Reed! Attaching patch to synchronize the change over to suite.
Attachment #307246 - Flags: review?(neil)
Comment on attachment 307241 [details] [diff] [review]
Patch v3

The change looks good to me, pending mano's "real" review.  Thanks Kai.
Attachment #307241 - Flags: review?(johnath) → review+
(In reply to comment #10)
> PSM would clear all its state tracking whenever nsIWebProgressListener is
> notified the combination of STATE_START | STATE_IS_REQUEST, loadFlags has
> nsIChannel::LOAD_DOCUMENT_URI and we learn this is a request for a top level
> document.
> 
> 
> Shouldn't we get that notification on navigate back ??? I think we should, but
> we don't.

Maybe some interaction between bfcache and error pages?
I have no idea.  biesi might know about error pages.  No one knows about bfcache.
Comment on attachment 307246 [details] [diff] [review]
Separate Patch v4 for SeaMonkey

>+      if (!(ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_INSECURE)) {
...although in nsBrowserStatusHandler.js we assume it's insecure unless it's either broken or secure.
Attachment #307246 - Flags: review?(neil) → review+
Comment on attachment 307241 [details] [diff] [review]
Patch v3

Alternatively trying to get review from Gavin on this blocker bug.

This review request is for the first chunk of this patch, only (change in mozilla/browser)
Attachment #307241 - Flags: review?(gavin.sharp)
(In reply to comment #21)
> > Shouldn't we get that notification on navigate back ??? I think we should, but
> > we don't.
> 
> Maybe some interaction between bfcache and error pages?


I must take my statement back. I repeated my testing, and now I can see the notification for the toplevel document.

Thanks a lot to Gavin for asking me many questions, forcing me to have a deeper look. 

It turns out the function which resets state "never" clears the SSL status object. That's the real reason why it's still around.

I think it would be reasonable to clear mSSLStatus each time we reset the state tracking. Unfortunately, it's hard to predict whether this will cause other regressions.

(At least, I tested, it voids the performance improvement I recently implemented in bug 406999.)

So, I still think the attached patches make sense.
It's correct not to use the mSSLStatus object in the given scenarios.

Apparently we've always kept the mSSLStatus object in memory.
Now that we prevent its use in inappropriate situations, I'd rather keep it around, in order to avoid further potential regressions.
Comment on attachment 307241 [details] [diff] [review]
Patch v3

>Index: mozilla/security/manager/pki/resources/content/PageInfoOverlay.xul

>+          isBroken = (ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_BROKEN);
>+          if (!(ui.state & Components.interfaces.nsIWebProgressListener.STATE_IS_INSECURE)) {
>+            sp = ui.QueryInterface(nsISSLStatusProvider);
>+            if (sp)
>+              status = sp.SSLStatus;
>+          }

Don't really need to make this change in this bug, but you could just get rid of "sp" entirely and just assign ui.QueryInterface(nsISSLStatusProvider).SSLStatus to "status" directly - QI in JavaScript will either return non-null or throw.

I sent Kai some other comments by email.
Attachment #307241 - Flags: review?(mano)
Attachment #307241 - Flags: review?(gavin.sharp)
Attachment #307241 - Flags: review+
This bug complains about page info after having used "navigate back" from an SSL error page.

However, I think the bug can be extended to page info for the error page itself.

When I go to https://www.cacert.org and open page info, I get:
- web site does not supply ID
- Verified by Root CA
- web site provides a cert to verify its identity [view]

IMHO that's inconsistent.


Luckily, the latest patch attached to this patch fixes both problems:
- it fixes the problem I report above (error page itself)
- it fixes the problem originally reported in this bug (after go back)
There was a minor merge conflict after recent changes, this is what I've checked in.
both patches checked in, marking fixed
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.