258048 - Security indicators updated when page finishes load, not when it starts rendering

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[:Mook]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040903 Firefox/1.0 PR (NOT FINAL)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040903 Firefox/1.0 PR (NOT FINAL)

When loading a secure page, the security indicates (lock in status bar, yellow
address bar for Firefox) is only changed when the page is completely loaded.  It
may be possible to pretend a site is secure (and, in the case of Firefox, the
domain in the status bar is not updated).

Reproducible: Always
Steps to Reproduce:
1. Go to a secure site
2. Go to a non-secure site that loads slowly


Actual Results:  
All indicators of secure site are kept until the non-secure site finishes
loading, after any page content has been rendered

Expected Results:  
The secure site inidicators are removed when any page rendering begins (and,
possibly, restored after it finishes if the new site is also secure)

It may be possible to spoof a secure site by:
1. Opening a window to the secure site's login page via script
2. Wait for the site to load (via timeouts?  Can't seem to do it via event
handlers, which is good)
3. Open a non-secure site in the same window (via script).  Spoofs the first
document.
4. Make sure the second page never finishes loading (possibly via an hidden
image with the download being throttled by the server).

To the user, all indications are that the second page is secure - and, excluding
the address, also indicates that the second document is on the domain of the
first document.

Proposed fix: Clear the security indicators when page starts loading.

(Sorry if this is a dupe - can't seem to find a dupe)

Tested on
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040903
Firefox/1.0 PR (NOT FINAL)
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.1) Gecko/20040707
(Seamonkey 1.7.1)

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[Daniel Veditz [:dveditz]]

Sort of the opposite of bug 257308, which has been made public. Opening this bug
so we can figure out what really's going on.

Comment 3

[chris hofmann]

try for 1.8b and the point releases

Comment 4

[Darin Fisher]

Attachment: v1 patch

This patch builds on jst's patch for bug 277564 by simply updating the lock
icon state for any channel in OnLocationChange.  The idea being that
OnLocationChange corresponds to a change in the URL bar, so it is therefore the
right time to affect the lock icon state.

One concern with this patch is that it introduces two security dialogs
potentially.  In the mixed case, we will first tell the user that the content
they are seeing is secure, and then when the page finishes loading, we will
tell them that, no, it is actually mixed.  This is a concern to me, but I think
it is a reasonable trade-off.  Certainly, if the toplevel document is insecure,
then there is no way for the lock icon to transition away from the unlocked
state.

Comment 5

[Darin Fisher]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Seems reasonable...

Comment 7

[Johnny Stenback (:jst)]

I didn't look at this diff yet, but maybe we should make the code never change
to a *secure* state before we actually know that it is secure, but we should for
sure change to an *insecure* state as soon as we're not sure that it is secure.
Would that be tricky in our code?

Comment 8

[Darin Fisher]

> I didn't look at this diff yet, but maybe we should make the code never change
> to a *secure* state before we actually know that it is secure, but we should for
> sure change to an *insecure* state as soon as we're not sure that it is secure.
> Would that be tricky in our code?

That's an interesting idea.  But, what would you do if navigating from one
https:// link to another?  Would you make the URL bar change from yellow to
white during the page load?  Consider the case where we are switching sites, but
those sites might be related from the users point of view.  What if the second
site is a spoofer?  I think that's enough reason to update the lock icon to the
secured state in OnLocationChange.  What do you think?

We should probably also verify that the mixed icon warning appears as soon as we
get some mixed content on the page and not once the page finishes loading.

We have to assume that the page never finishes loading when considering how to
tackle this problem.

Comment 9

[Johnny Stenback (:jst)]

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

r=jst, I think this is indeed an improvment over what we've got today.

Comment 10

[Daniel Veditz [:dveditz]]

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

a=dveditz for 1.7 and aviary1.0.1 branches

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

a=caillon for 1.8b.  Seems like it is a good (not to mention important) fix,
and we can do any further work later.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fix committed on HEAD.

Comment 13

[Daniel Veditz [:dveditz]]

adding to bounty nomination list at reporter's request.

Comment 14

[Darin Fisher]

fixed1.7.6, fixed-aviary1.0.1

shaver: thanks for landing this on the trunk for me! :)

Comment 15

[Jay Patel [:jay]]

Verified Fixed.  Going from a secure site to a non-secure site now immediately
removes the yellow background and lock icon when the second site begins to render.

Aviary 1.0.1 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20050217 Firefox/1.0

M18b1/Trunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217

M176 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050217

Comment 16

[Daniel Veditz [:dveditz]]

As I worried over in dupe bug 278003 comment 3 the multiple entering/leaving
dialogs have come back :-(

See bug 145730
regressed by "show lock early" bug 148610
fixed again in bug 154084

Also we should retest bug 154240 and make sure appropriate warnings are given
for redirections.

Should I file a new bug, reopen this one, or reopen bug 154084?

Comment 17

[Daniel Veditz [:dveditz]]

This probably should have been in the PSM product.