accessing a relative anchor in a secure page removes the locked icon and yellow background UI

VERIFIED FIXED

Status

Core Graveyard
Security: UI
--
critical
VERIFIED FIXED
14 years ago
2 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: Darin Fisher)

Tracking

(Blocks: 1 bug, 4 keywords)

Other Branch
fixed-aviary1.0.2, fixed1.7.6, regression, verified1.7.6
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

14 years ago
posting from Asa's blog:

1. If I enter the URL: https://agia.fsf.org/order/
I get the padlock in the bottom and the address bar is yellow.

2. Now if I click on one of the #refs, eg the link "Software",
I lose the padlock and the yellow address bar.

3. Pasting the full url https://agia.fsf.org/order/#software
gives the secure feedback, but clicking another #ref
loses it again.

This is with 1.0.1 en-us Linux

------------------------------------

I was able to reproduce this somewhat using a welsh (cy-GB, have been doing l10n
testing, I hope this doesn't matter) 20050223-1.0.1 build. (I can recheck later
with en-US.)

Anyhow, I get the same misbehavior at step 2 --but strangely, step 3 for me
doesn't restore the locked padlock or yellow background color in the address bar.

I tested this using a ffox 1.0 build (mac again), and this was not a problem.
(Assignee)

Comment 1

14 years ago
yikes... this sounds pretty bad.
Severity: normal → critical
Status: NEW → ASSIGNED

Comment 2

14 years ago
This is public (http://weblogs.mozillazine.org/asa/archives/007626.html) and not
a security hole.  Why is it marked as security-sensitive?
(Assignee)

Comment 3

14 years ago
Are people seeing this on the trunk, or is it only affecting Firefox 1.0.1?
(Reporter)

Comment 4

14 years ago
I cannot reproduce this using 2005021409-trunk ffox bits (linux fc3). I'll grab
a more recent build...
(Reporter)

Comment 5

14 years ago
hrm, I do see this using 2005022511-trunk ffox bits.

Jay had email some https / location bar / security / padlock bugs with checkins
which might be suspect:

https://bugzilla.mozilla.org/show_bug.cgi?id=258048
https://bugzilla.mozilla.org/show_bug.cgi?id=268483
https://bugzilla.mozilla.org/show_bug.cgi?id=277564
https://bugzilla.mozilla.org/show_bug.cgi?id=283201

Comment 6

14 years ago
Testing Windows, this regressed on the 1.0.1 branch between
2005-02-14-06-aviary1.0.1 and 2005-02-15-06-aviary1.0.1. Sarah, please check the
trunk build from the 15th.
(Reporter)

Comment 7

14 years ago
using ffox trunk builds on linux fc3, this regressed between:

* 2005021510-trunk (works)
* 2005021609-trunk (broken)

another observation: after clicking a relative link (then noticing the locked UI
go away), reloading (ctrl+R) the page restores the locked UI.
(Assignee)

Comment 8

14 years ago
Thanks for narrowing the regression window.  It looks like the patch for bug
258048 is the likely cause of this bug.  Investigating...
(Assignee)

Comment 9

14 years ago
OK, the problem here is quite simple.  We are getting a null request in the
OnLocationChange event generated for navigations to https://blah.com/#ref

I could attempt to fix this bug by skipping the lock icon updating logic when we
have a null request (or perhaps when we have a null channel).  What do people
think of that solution?

It may also be wrong that we are sending an OnLocationChange in this case (since
there is no corresponding request), but then again... I think that might be
necessary to ensure that the URL bar is updated properly.  I wonder what other
cases may result in a null request.  Is it always valid to ignore those and
assume that they have the same security level?
(Assignee)

Comment 10

14 years ago
Created attachment 175629 [details] [diff] [review]
v1 patch
Attachment #175629 - Flags: superreview?(dveditz)
Attachment #175629 - Flags: review?(jst)
Comment on attachment 175629 [details] [diff] [review]
v1 patch

This seems reasonable to me. r=jst
Attachment #175629 - Flags: review?(jst) → review+
Clearing the security flag for reasons Jesse noted in comment 2
Group: security
Comment on attachment 175629 [details] [diff] [review]
v1 patch

sr- This regresses recently fixed bug 258048 and bug 277564

As far as I can tell aRequest is *always* null when passed to OnLocationChange.
Attachment #175629 - Attachment is obsolete: true
Attachment #175629 - Flags: superreview?(dveditz) → superreview-
-> PSM
Blocks: 130949
Component: General → Client Library
Flags: review+
Product: Firefox → PSM
Version: Trunk → unspecified
(Assignee)

Comment 15

14 years ago
dveditz: no!  if you recall, i fixed the regression where aRequest was always
null.  that regression was caused by a bad merge to nsDocShell.  i tested this
very carefully, and aRequest is only null in special cases such as this one
where there is not a network request.  otherwise, due to johnny's patch, there
is a non-null network request.  please re-consider the patch.

Updated

14 years ago
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
(Assignee)

Comment 16

14 years ago
Comment on attachment 175629 [details] [diff] [review]
v1 patch

With this patch applied I could not reproduce bug 258048.  Did you find that
you could?

restoring r=jst
Attachment #175629 - Flags: superreview?(dveditz)
Attachment #175629 - Flags: superreview-
Attachment #175629 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #175629 - Attachment is obsolete: false
Isn't aRequest null on tab switches?  Does the lock icon update properly on tab
switch with this change?
Comment on attachment 175629 [details] [diff] [review]
v1 patch

ah, *docshell*. I made sure to cvs up in security before testing your patch,
but forgot the late-breaking docshell change. :-(

Sorry about that, it of course works as advertised (*phew*, I was wondering how
in the world the other patches worked on a null request).

sr=dveditz
Attachment #175629 - Flags: superreview?(dveditz) → superreview+
I think this should block 1.7.6 just to get it right, but since it shows a
secure site as non-secure rather than the other way around it's not a security
hole that requires a firefox respin.
Flags: blocking1.7.6+
Comment on attachment 175629 [details] [diff] [review]
v1 patch

requesting 1.7.6 approval, and 1.0.1 branch for the heck of it just in case
there's a 1.0.2
Attachment #175629 - Flags: approval1.7.6?
Attachment #175629 - Flags: approval-aviary1.0.1?
(Assignee)

Comment 21

14 years ago
(In reply to comment #17)
> Isn't aRequest null on tab switches?  Does the lock icon update properly on tab
> switch with this change?

No, nsSecureBrowserUI::OnLocationChange is not called when tabs are switched. 
If that is using nsIWebProgressListener, then it must depend on something
special being done by tabbrowser.xml for nsIWebProgressListener's attached
directly to it.  I suspect that the tabbrowser stores the state of each browser,
and notifies its listeners when things change.  The nsSecureBrowserUI is
attached to each browser.
(Assignee)

Comment 22

14 years ago
I think it would be wise to consider respinning for this fix since it can give
users false confidence in the browser.  It may for example make some ecommerce
site appear insecure, which might discourage customers, etc.  That seems like
something worth fixing.  Wasn't there a respin planned for the installer problem?

Comment 23

14 years ago
(In reply to comment #22)
> Wasn't there a respin planned for the installer problem?

The respin was for 1.0.1 and included no app-level changes, only installer
changes.  Happened last night, files have been QA'd, signed, and are ready to be
pushed onto the site.
(Assignee)

Comment 24

14 years ago
ok, fair enough.  i don't know how best to handle this.
(Assignee)

Comment 25

14 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
The tabbrowser code is at
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#572

Looks like the security UI is just not a listener on the <tabbrowser>, if this
code doesn't break us....

Updated

14 years ago
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
(Assignee)

Comment 27

14 years ago
Comment on attachment 175629 [details] [diff] [review]
v1 patch

1.0.1 has shipped; requesting approval for aviary 1.0.2
Attachment #175629 - Flags: approval-aviary1.0.2?
(Assignee)

Updated

14 years ago
Flags: blocking-aviary1.0.2?

Comment 28

14 years ago
Comment on attachment 175629 [details] [diff] [review]
v1 patch

approved.
Attachment #175629 - Flags: approval1.7.6?
Attachment #175629 - Flags: approval1.7.6+
Attachment #175629 - Flags: approval-aviary1.0.2?
Attachment #175629 - Flags: approval-aviary1.0.2+
Attachment #175629 - Flags: approval-aviary1.0.1?
Can someone land this fix on the appropriate branches?
(Assignee)

Comment 30

14 years ago
yes, i will do so shortly.
(Assignee)

Comment 31

14 years ago
fixed-aviary1.0.1, fixed1.7.6
Keywords: fixed-aviary1.0.1, fixed1.7.6

Comment 32

14 years ago
*** Bug 285241 has been marked as a duplicate of this bug. ***

Comment 33

14 years ago
(In reply to comment #31)
> fixed-aviary1.0.1, fixed1.7.6

Hmm fixed-aviary1.0.2 I guess.

Updated

14 years ago
Keywords: fixed-aviary1.0.1 → fixed-aviary1.0.2

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
(Reporter)

Comment 34

14 years ago
tested with 2005031011-1.7 mozilla bits on linux fc3: looks good, the locked
padlock in the statusbar doesn't disappear when I click a relative anchor in the
secure page.
Keywords: fixed1.7.6 → verified1.7.6
Flags: blocking-aviary1.0.3? → blocking-aviary1.0.2+
(Reporter)

Comment 35

14 years ago
mistakenly removed fixed1.7.6 --pardon the bugspam. set your filter/quicksearch
to "ZippidityDooDahHey" to catch these for easy removal/etc/
Keywords: fixed1.7.6
(Reporter)

Comment 36

14 years ago
also verified fixed in

- firefox 1.0.2 branch 20050314 build on Mac OS X 10.3.8

- firefox trunk 20050314 builds on Mac and Win XPsp2.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.