257308 - Visual indicators of site security appear for the wrong site (lock icon)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ben Sizer]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040819 Firefox/0.8 StumbleUpon/1.9 (MOOX M2)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040819 Firefox/0.8 (MOOX M2)

If I'm on an http site, then I click a link on that site to a totally unrelated
https server which provides a downloadable file - not a file that can be viewed
in Firefox, and not where the link target is a new window - then the address bar
for the currently viewed HTTP site goes yellow, and the security box on the
status bar shows the http site with a padlock. In other words, the HTTP site is
reported as taking on the security characteristics of the HTTPS server that is
supplying the file rather than the currently-viewed page.

Further discussion here: http://forums.mozillazine.org/viewtopic.php?t=117908

Reproducible: Always
Steps to Reproduce:
1.Find or create a link on a HTML page hosted on a HTTP server that points to a
downloadable file on an unrelated HTTPS server.
2.Click that link.
3.Accept the file or cancel (the visual indications change before you make the
choice anyway.)

Actual Results:  
Address bar background changes yellow.
Status bar security field displays the HTTP server name with a padlock, and
changes the tool-tip to mention the HTTPS's certificate issuing authority.

Expected Results:  
Made none of the above changes in the UI.

Perhaps the relevant section of the download manager window should go yellow
instead, but I'm sure that's a separate issue...

The linked-to discussion leads me to believe that the yellow-bar feature may
only be visible in more recent builds.

Comment 1

[kstahl]

Confirming. Using the link in that thread to download a file from a secure
server gives me the padlock on the page I'm currently on.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

Comment 2

[logan]

This appears to be quite old. I've been able to duplicate this with Firefox
branch, 0.9.3, 0.9.1, 0.8 and Mozilla 1.7.2. It's possible this is a dupe of
another bug with the security flag.

Marking NEW, requesting blocking.

Thanks for filing man. :)

Comment 3

[logan]

HW/OS -> All, Severity -> critical

Well, it's pretty serious anyway heh..

Comment 4

[Mike Connor [:mconnor]]

If we've been shipping with this behaviour for a while, I doubt we'll block on this.

Comment 5

[chris hofmann]

darin will take a look to see what we can do here.

Comment 6

[chris hofmann]

if this can make PR it would be good.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is a dup.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

happens in mozilla 2004090105/winxp too, -> browser

Comment 9

[Daniel Veditz [:dveditz]]

On the surface this is sort of the opposite of bug 258048

Comment 10

[Daniel Veditz [:dveditz]]

*** Bug 238566 has been marked as a duplicate of this bug. ***

Comment 11

[Daniel Veditz [:dveditz]]

Are you sure this is a dupe? We've fixed similar bugs, but I can't find one
currently like this.

bug 240053 was fixed in Moz1.7 / ff0.9 (redirects)
bug 253121 was fixed in Moz1.7.2 / ff0.9.3 (onunload document.write)

bug 258048 describes an opposite situation, security indicators for the current
site stick around too long.

Comment 12

[Darin Fisher]

Attachment: sample binary attachment

Comment 13

[Darin Fisher]

Attachment: sample test case (view on non-secure server)

Load this attachment over plaintext HTTP.  Then, click the link named "click
me".  Notice that you get a download dialog and the lock icon is present on the
browser window, but the original page contents are still showing.

Comment 14

[Darin Fisher]

I can reproduce this bug using Mozilla 1.0 and 1.4, so this has been around for
a long time.  I also don't see any evidence in the code that there was ever a
design intended to handle this case :-(

Comment 15

[Darin Fisher]

Attachment: v1 patch

OK, this seems like a safe patch.  The solution is to basically make retargeted
loads behave more like redirected loads as far as the webprogresslistener
notifications are concerned.

With this patch, we pass NS_BINDING_RETARGETED as the status code when calling
RemoveRequest from inside nsExternalAppHandler::RetargetLoadNotifications.  It
makes sense to pass an error code here because the load is actually not
successful with respect to loading inside the load group.  It is really pretty
wrong to say that the request has completed normally, which is what the current
code does.

The other change this patch makes is to cause the DocLoader to not synthesize a
STATE_TRANSFERRING event when the loadgroup calls its OnStopRequest method
(triggered from RemoveRequest).  This is what we currently do for redirected
requests, and it makes sense to do the same for retargeted requests.

One more thing to note: NS_BINDING_RETARGETED is not used anywhere else.  I
recall back in the day that it was used.  Used by code in uriloader land if I'm
not mistaken.  The fact that it is not used is good from the point of view of
this patch, since it means that we aren't co-opting an existing meaning for
this error code.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158258 [details] [diff] [review]
v1 patch

sr=bzbarsky if you also fix the similar code at
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#4546
(which is retargeting from one docshell to another, but the effect is the
same).

Comment 17

[Darin Fisher]

Attachment: v1.1 patch - with similar nsDocShell changes

thanks bz!

Comment 18

[Darin Fisher]

fixed-on-trunk

Comment 19

[Daniel Veditz [:dveditz]]

Needed on 1.7 also

Comment 20

[Mike Kaply [:mkaply]]

Comment on attachment 158283 [details] [diff] [review]
v1.1 patch - with similar nsDocShell changes

a=mkaply for both

Comment 21

[Bill Gianopoulos [:WG9s]]

Needed on Aviary also.

Comment 22

[Darin Fisher]

fixed1.7.x, fixed-aviary1.0

Comment 23

[Ben Sizer]

This isn't totally fixed, assuming it's supposed to be on my build (Firefox
branch, 20040912). I can't give out the test URL I see this on for copyright and
practicality reasons, but clicking a link hosted on http://www.livejournal.com
leading to an mp3 file hosted at https://home.comcast.net still makes
livejournal's location bar go yellow and padlocked, and the status bar shows
livejournal.com as being a secure site.

It seems possibly dependent on mime type as the behaviour seems fixed for zip
files on the secure server but still broken for mp3s.

Comment 24

[logan]

Attachment: 30kb mp3

Comment 25

[logan]

Hrm, ok I'm not sure that worked right heh.. I was trying to make a testcase
with test.mp3, but the file is displaying in the browser.

Thought maybe if the content-type was being sent as text/plain, Firefox would
start out wanting to view the file, but once it sees binary data it'd change to
download instead.

I setup a test at work and didn't have any problems with content-type bits.

http://www.gozer.org/test/mozilla/257308/

Comment 26

[Ben Sizer]

Ok, that one worked fine for me too. Maybe it's something Comcast-specific.

Comment 27

[Robert Parenton]

The testcase should work now.  A mimetype of application/octet-stream should
force Firefox to always ask to download.

Comment 28

[Darin Fisher]

Hmm... I just downloaded the latest aviary branch build from ftp.mozilla.org
(Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20040910 Firefox/0.10), and I
couldn't reproduce the bug using the testcases at
http://www.gozer.org/test/mozilla/257308/.

Comment 29

[logan]

(In reply to comment #26)
> Ok, that one worked fine for me too. Maybe it's something Comcast-specific.

Since you can't put up the urls, could you get the headers for the file that's
causing this?

Comment 30

[Ben Sizer]

I don't know how to get the headers for an mp3 download. Page Info and the Web
Developer extension's 'view response headers' function don't work because the
page stays blank while it downloads in the download manager. Any suggestions?

Comment 31

[logan]

There's lotsa ways to do this..

wget -sqO- http://www.example.com/some.mp3

-(~) telnet www.example.com 80
Trying 127.0.0.1...
Connected to www.example.com.
Escape character is '^]'.
HEAD /some.mp3 HTTP/1.0
Host: www.example.com

HTTP/1.1 200 OK
...

telnet won't work on an ssl-enabled server, so...

openssl s_client -connect www.example.com:443
<send http request>

Or, you could even make a small libcurl or perl client. :P

Comment 32

[Ben Sizer]

Interesting... :

HTTP/1.1 200 OK
Server: Netscape-Enterprise/4.1
Date: Tue, 14 Sep 2004 01:29:16 GMT
Content-type: text/plain
Last-modified: Mon, 13 Sep 2004 20:15:51 GMT
Content-length: 2740429
Accept-ranges: bytes
Connection: close

It's sending text/plain, yet it's definitely an mp3 file, and Firefox correctly
identifies it as such (giving me the save/open dialog) when I download it.

Comment 33

[Ben Sizer]

Ok! I have a new testcase for you to try:
http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html
Click the mp3 link there, which leads to an mp3 file hosted on a Comcast server,
accessed with https. The actual/expected results are as I originally reported at
the start of this bug. I have just tested with a 20040916 branch build of
Firefox. Can anyone confirm this?

Comment 34

[logan]

Man, I feel like a dumbass - Content-Type: test/plain

I've fixed my testcase and am able to see it now, must be due to the text/plain
[oh it's binary] stuff (bug 220807).

Reopened

Comment 35

[logan]

Ok, I can't get my bugzilla test.mp3 attachment to work right, I think it's
content-disposition: inline header, so we'll just have to rely on the external
testcases.

Comment 36

[Darin Fisher]

> http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html
> Can anyone confirm this?

Yes, I can reproduce the problem too using the 9/14 aviary branch build w/ your
testcase.  Investigating...

Comment 37

[Darin Fisher]

It looks like we are getting an OnProgress event prior to OnStartRequest when
loading http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html.  I think
that would explain the lock icon bug because the nsSecureBrowserUI uses the
OnProgress event to determine that secure content is being shown to the user. 
Hmm...

It looks like nsHttpChannel does not ensure that OnProgress occur only after
OnStartRequest.  There's a side issue since we use OnProgress to report upload
progress as well as download progress.  Clearly, nsSecureBrowserUI should only
apply its OnProgress logic to downloads, so we should verify that.

Comment 38

[Darin Fisher]

ok, after further analysis i understand the problem.  the channel cannot solve
this problem.  it is in fact delaying OnProgress until after OnStartRequest, but
that is not enough.  in this case, the content-type from the server is
text/plain.    in this case we invoke a content sniffer to determine if the
content is really text because it is often the case that servers such as this
one are misconfigured.  the content sniffer requires data to be read from the
channel, which triggers the OnProgress notification.

to solve this problem i think we need to suppress sending
OnStateChange(STATE_TRANSFERRING) until after a load has been targetted.  this
only applies to toplevel loads with the LOAD_DOCUMENT_URI load flag.  one way to
do that would be to invent a new load flag that gets set by the
nsDocumentOpenInfo once it has targetted a load.  nsDocLoader::OnProgress would
then check for that load flag and suppress the OnStateChange call if the load
flag is not set (for the case in which LOAD_DOCUMENT_URI is set).

i discussed this plan with biesi over IRC and he agrees.  this seems like a more
robust fix that should help minimize the chance of this bug ever coming up again.

patch coming up...

Comment 39

[Darin Fisher]

Attachment: v2 patch

Here's a patch for the solution I mentioned.  I still need to document the load
flags used by the URILoader, DocLoader, and DocShell better.  That
documentation probably doesn't belong in nsIChannel.idl ;-)

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159229 [details] [diff] [review]
v2 patch

>Index: uriloader/base/nsDocLoader.cpp
>-  nsLoadFlags loadFlags = 0;
>+  PRUint32 loadFlags = 0;

Why?  The loadFlags attribute of nsIRequest is of type "nsLoadFlags"...

>+    newLoadFlags = nsIChannel::LOAD_RETARGETED_DOCUMENT_URI;

This should be an |=, no?

sr=bzbarsky with those fixed (and perhaps the other GetLoadFlags callers also
using nsLoadFlags?)

Comment 41

[Darin Fisher]

Attachment: v2.1 patch - revised per bz's comments

nsLoadFlags vs. PRUint32 ... no good reason really.  much of necko uses
PRUint32 (despite nsIRequest.idl), and so I went with that here too.  But, I
don't care much either way, so I changed all of uriloader/base over to
nsLoadFlags.

Thanks for catching the |= mistake.  That's corrected in this patch too.

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

looks good

Comment 43

[Darin Fisher]

fixed-on-trunk

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

a=dveditz for 1.7x contingent on aviary approval. Would like some trunk baking
first.

Comment 45

[Asa Dotzler [:asa]]

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

a=asa for aviary checkin.

Comment 46

[Darin Fisher]

fixed-aviary1.0 and fixed1.7.x for v2.1 patch

Comment 47

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Complete patch, backported to 1.4

Comment 48

[Darin Fisher]

Comment on attachment 172289 [details] [diff] [review]
Complete patch, backported to 1.4

>Index: uriloader/base/nsURILoader.cpp

>     }
> 
>+    aChannel->SetLoadFlags(loadFlags | newLoadFlags);
>+
>+
>     const char* contentTypeToUse;

nit: no need for extra newline


>     if (helperAppService)
>     {
>+      // Set these flags to indicate that the channel has been targeted and that
>+      // we are not using the original consumer.
>+      nsLoadFlags loadFlags = 0;
>+      request->GetLoadFlags(&loadFlags);
>+      request->SetLoadFlags(loadFlags | nsIChannel::LOAD_RETARGETED_DOCUMENT_URI
>+                                    | nsIChannel::LOAD_TARGETED);
>+
>       rv = helperAppService->DoContent(contentType.get(),
>                                        request,
>                                        m_originalContext,
>                                        getter_AddRefs(contentStreamListener));
>       if (NS_SUCCEEDED(rv) && contentStreamListener) {
>         m_targetStreamListener = contentStreamListener;
>+        request->SetLoadFlags(loadFlags);
>         return NS_OK;
>       }

I think it is wrong to call SetLoadFlags in the success case here.
The trunk does so in the failure case.

Comment 49

[Darin Fisher]

also, the last patch didn't include the definition of nsIChannel::LOAD_TARGETED.

Comment 50

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Backport for 1.4 take 2

addressing darin's comments.  I also think we need to null out
m_targetStreamListener (if DoContent() fails) looking at the current trunk.  It
seems like the correct thing to do here, what do you think darin?

Comment 51

[Darin Fisher]

Comment on attachment 174321 [details] [diff] [review]
Backport for 1.4 take 2

sure, this looks fine to me.  r=darin