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

VERIFIED FIXED in mozilla1.8beta1

Status

()

Core
Security
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Mook, Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug, {fixed-aviary1.0.1, fixed1.7.6})

Trunk
mozilla1.8beta1
fixed-aviary1.0.1, fixed1.7.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.1 +
blocking1.8b +
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] need landing)

Attachments

(1 attachment)

3.90 KB, patch
jst
: review+
dveditz
: approval1.7.6+
Christopher Aillon (sabbatical, not receiving bugmail)
: approval1.8b+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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)
*** Bug 238566 has been marked as a duplicate of this bug. ***
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 130949
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.
Group: security
Assignee: dveditz → darin
Group: security
Whiteboard: [sg:spoof?]
(Assignee)

Updated

13 years ago
Target Milestone: --- → mozilla1.8beta
(Assignee)

Updated

13 years ago
Blocks: 278003
Whiteboard: [sg:spoof?] → [sg:fix]
Blocks: 278184
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Flags: blocking1.8b?
(Assignee)

Updated

12 years ago
Flags: blocking1.7.6?
Flags: blocking-aviary1.0.1?

Comment 3

12 years ago
try for 1.8b and the point releases
Flags: blocking-aviary1.1?
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+

Updated

12 years ago
Flags: blocking1.8b?
Flags: blocking1.8b+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
(Assignee)

Comment 4

12 years ago
Created attachment 173823 [details] [diff] [review]
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.
Attachment #173823 - Flags: superreview?(bzbarsky)
Attachment #173823 - Flags: review?(jst)
(Assignee)

Comment 5

12 years ago
*** Bug 278003 has been marked as a duplicate of this bug. ***
Comment on attachment 173823 [details] [diff] [review]
v1 patch

Seems reasonable...
Attachment #173823 - Flags: superreview?(bzbarsky) → superreview+
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?
Whiteboard: [sg:fix] → [sg:fix] need review jst
(Assignee)

Comment 8

12 years ago
> 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 on attachment 173823 [details] [diff] [review]
v1 patch

r=jst, I think this is indeed an improvment over what we've got today.
Attachment #173823 - Flags: review?(jst) → review+
Comment on attachment 173823 [details] [diff] [review]
v1 patch

a=dveditz for 1.7 and aviary1.0.1 branches
Attachment #173823 - Flags: approval1.8b?
Attachment #173823 - Flags: approval1.7.6+
Attachment #173823 - Flags: approval-aviary1.0.1+
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.
Attachment #173823 - Flags: approval1.8b? → approval1.8b+
Whiteboard: [sg:fix] need review jst → [sg:fix] need landing
Fix committed on HEAD.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
adding to bounty nomination list at reporter's request.
Blocks: 256195
(Assignee)

Comment 14

12 years ago
fixed1.7.6, fixed-aviary1.0.1

shaver: thanks for landing this on the trunk for me! :)
Keywords: fixed-aviary1.0.1, fixed1.7.6

Comment 15

12 years ago
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
Status: RESOLVED → VERIFIED
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?
This probably should have been in the PSM product.
Group: security
(Assignee)

Updated

12 years ago
Blocks: 258376
You need to log in before you can comment on or make changes to this bug.