317380 - The status bar domain name display is not updated when the "back" button is clicked (bfcache, XSLT)

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Tyler Close]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

The onSecurityChange() notification is not sent when "Back" button is clicked.

Reproducible: Always

Steps to Reproduce:
1. Visit an https page
2. Click on a contained hyperlink to an https page at another site
3. Click the "back" button

Actual Results:  
You are now looking at the first https page; however, the domain name display at the right hand side of the status bar still shows the domain name from the linked to https page.

Expected Results:  
The domain name display should show the domain name of the current web page.

This happens because the onSecurityChange() event of the nsIWebProgressListener interface does not get fired when you click the "back" button.

Comment 1

[Anders Conbere]

WFM Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051110 Firefox/1.5

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Issues like these are very often caused by misbehaving extensions. Could you try reproducing this in Safe mode?

http://kb.mozillazine.org/Safe_Mode

Comment 3

[Tyler Close]

The bug seems to have something to do with the use of XSLT on the linking page. For example, the bug can always be reproduced when visiting this page:

<https://yurl.net/id/backtest>

The bug also occurs when using the Firefox Safe mode.

Comment 4

[Anders Conbere]

Gavin I'm going to confirm this for now, you can back that out later if need be.

I deffinantly see the issue described in that test case.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This is a bfcache issue... doesn't happen with browser.sessionhistory.max_total_viewers = 0.

Comment 6

[Jesse Ruderman]

WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051120 Firefox/1.6a1

Comment 7

[Jesse Ruderman]

Oops, I had disabled bfcache.  I can reproduce now.

Comment 8

[Daniel Veditz [:dveditz]]

If the back navigation takes you to a non-ssl page there's no spoofing. This lets you spoof one SSL page as another SSL page in the status bar (the address bar, of course, is correct).

It's not hard for a spoofer to get a domain-controlled SSL cert.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to comment #8)
> If the back navigation takes you to a non-ssl page there's no spoofing.

Sorry, Tyler was right in comment 3: the backtest page using xslt reproduces the problem when hosted locally non-ssl, other random starting pages don't have the problem regardless of SSL or not.

Comment 10

[Tyler Close]

After clicking the "back" button, try clicking the lock icon in the status bar to display the security page info. The first paragraph in the "Security" tab says:

"The web site yurl.net supports authentication for the page you are viewing. The identity of this web site has been verified by Verisign, Inc., a certificate authority you trust for this purpose."

yurl.net doesn't have a certificate from Verisign, Inc. The dialog appears to be using a cached value for the domain name and the certificate authority name taken from the vsci.hpl.hp.com certificate (the linked to site). That's a second bug. The dialog should be taking the domain name value from the CN field of the displayed certificate. Doing this at least ensures that the dialog is presenting self consistent information, rather than fiction.

Comment 11

[Jesse Ruderman]

Here are two ways a site like https://yurl.net/id/backtest could exploit this to spoof https://bugzilla.mozilla.org/:

(1) With an alert

javascript: function invoke() { location.href = "https://bugzilla.mozilla.org"; alert("Please wait"); history.back(); } setTimeout(invoke, 200); void 0

(2) With a temporary, possibly hidden popup

javascript:function foo() { opener.location = "https://bugzilla.mozilla.org"; setTimeout(function() { opener.history.back(); close(); }, 1500); } var b = document.createElement("button"); b.appendChild(document.createTextNode("Click me!")); b.onclick = function() { open('data:text/html,<script>' + foo + 'foo()</script>Please wait','','width=100,height=100,location=no,directories=no,status=no,menubar=no'); }; document.body.appendChild(b); void 0

To test, just load https://yurl.net/id/backtest, paste one of those bookmarklets, and keep an eye on the status bar.

Using alert() has the advantage of being fast and not requiring previous user interaction with the evil page.  Using a popup has the advantage that the user might not see it at all, especially if you use window.focus() to keep the main window in front.

Comment 12

[Tyler Close]

I filed this bug before the 1.5 release went out. I am a little surprised the two bugs weren't fixed before putting out the 1.5 release. It looks like we've isolated the problems. Is there a plan for releasing a fix for this bug?

I was also wondering how many hidden "Security-Sensitive" bugs are currently outstanding against the Firefox 1.5 release.

Comment 13

[Jesse Ruderman]

While it's true that you filed this bug before Firefox 1.5 was released, Firefox 1.5 RC3 had already been released and was on track to become Firefox 1.5.  If we had to respin for a new RC and restart our (unfortunately week-long) release process every time a security hole was discovered, Firefox 1.5 could have been delayed for quite a while.  Firefox 1.5 itself contains important security fixes and an overhaul of the software update system, giving us one reason to not want to delay the release of Firefox 1.5 so it could pick up a fix for this bug.  Sorry for not explaining this earlier.

I have nominated this bug to block Gecko 1.8.0.1 (Firefox 1.5.0.1), which is expected to be released in late January.

FWIW, there are 150 security-sensitive bugs in the "new", "reopened", or "assigned" states, and 37 in the "unconfirmed" state.  Those lists include a lot of bugs that aren't security holes (such as metabugs, reminders to audit certain features, and reports without useful information), but they also include more security holes at least as severe as this one than I'd like.

Comment 14

[Jesse Ruderman]

CCing some people who have fixed lock icon bugs recently and might be able to help figure out what's going on here.

Comment 15

[[e-mail]]

I can only reproduce the problem when navigating back or forward from another https site to https://yurl.net/id/backtest while bfcache is enabled.

Comment 16

[Darin Fisher]

-> bryner

I also only see this bug when I have bfcache enabled.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

At:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.758&mark=5513#5510
document->GetChannel() is null. So at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/base/nsDocLoader.cpp&rev=3.289&mark=1257,1264#1228
aRequest is null, so:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp&rev=1.54&mark=1174#1150
is true, which means UpdateSecurityState (which calls onSecurityChange) isn't called.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

I thought this might have been introduced by bug 301516, but it dates back to the landing of fastback (between 2005-05-04 07:00 and 2005-05-04 16:00) so none of the subsequent fixes seem to have affected it.

Comment 19

[Brian Ryner (not reading)]

Yeah, this only happens when XSLT is used.  When the result document is created, StartDocumentLoad is never called on it.  This should be fine, since we're not actually going to load anything.  But I do think we need to get the channel that's created in URIUtils::ResetWithSource to be the new document's mChannel, or we won't have any channel to use for the progress notifications when restoring the result doc.

Boris, do you think it makes sense to set mChannel automatically when Reset() is called, or should we have a separate way to do this?

Comment 20

[Brian Ryner (not reading)]

Attachment: patch

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207372 [details] [diff] [review]
patch

Seems reasonable if you make the same change to txMozillaTextOutput::createResultDocument

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 207372 [details] [diff] [review]
patch

Oops, darin is still on vacation.  I'm going to check this in with r+sr=bzbarksy, assuming that's ok.

Comment 23

[Brian Ryner (not reading)]

checked in

Comment 24

[Brian Ryner (not reading)]

Comment on attachment 207372 [details] [diff] [review]
patch

requesting branch approvals.  this patch should be fairly safe and does not break any APIs.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Might be worth posting the full patch (with the other file that needs to be included) so we make sure to fix that on branch...

Comment 26

[Brian Ryner (not reading)]

Attachment: full patch that was checked in

Comment 27

[Mike Schroepfer]

Is the onSecurityChange() notification accessible through Javascript?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Chrome JS, probably yes.  Content JS, no.

Comment 29

[Daniel Veditz [:dveditz]]

we want more baking, -> 1.8.0.2
You can check reviewed patches into 1.8.1 (ff2) without extra approvals for now

Comment 30

[Brian Ryner (not reading)]

checked in on MOZILLA_1_8_BRANCH

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to comment #29)
> You can check reviewed patches into 1.8.1 (ff2) without extra approvals for now

I have been corrected: 1.8.1 needs approvals, see updated tree rules on tinderbox. Drivers approved this fix for 1.8.1 though.

Comment 32

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 322683, apparently.  :(

Comment 33

[Bob Clary [:bc] (inactive)]

v yurl testcase 1.8.1, 1.9a1 windows/linux

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207531 [details] [diff] [review]
full patch that was checked in

This needs to land, if it's going to block... ;)

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 207531 [details] [diff] [review]
full patch that was checked in

approved for 1.8.0 branch, a=dveditz for drivers

Comment 36

[Boris Zbarsky [:bzbarsky]]

Checked this in on 1.8.0.2 branch.

Comment 37

[Marcia Knous [:marcia]]

verified on the 1.8.0.2 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060306 Firefox/1.5.0.2. I tried to reproduce using the steps in the initial report and I don't see the problem. adding keyword.