Closed Bug 258048 Opened 15 years ago Closed 15 years ago
Security indicators updated when page finishes load, not when it starts rendering
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. ***
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.
try for 1.8b and the point releases
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.
*** 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?
> 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
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
Closed: 15 years ago
Resolution: --- → FIXED
adding to bounty nomination list at reporter's request.
fixed1.7.6, fixed-aviary1.0.1 shaver: thanks for landing this on the trunk for me! :)
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.
You need to log in before you can comment on or make changes to this bug.