154084 - entering/leaving secure site alert pops up incessantly

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: anything with the patch for bug 148610 in it

STEPS TO REPRODUCE:
This part is a little tricky:

1)  Turn on notifications for entering/leaving secure sites
2)  Load https://www.rsiaa.org/rsiauth/cgi-bin/home.pl
3)  Log in
4)  Click on the image in the top left corner (link to the same page)

EXPECTED RESULTS:  Page just reloads (possibly from browser cache)

ACTUAL RESULTS:  Popup appears saying you are leaving secure site.  Then popup
                 appears saying you are entering secure site.  Then page
                 reloads.

The same happens when going between any two password-protected pages on this site.

This is a regression caused by the patch for bug 148610 (tested by finding the
regression date, finding the checkins on that day, reverting
nsSecureBrowserUIImpl.{h,cpp} to the state they were in on the evening of the
16th, seeing the bug disappear, applying that patch, seeing the bug reappear).

I suspect that the bug has something to do with the need to send the
Authorization: header, since loading pages on this site that are secure but not
password-protected does not show this problem.

Unfortunately, I cannot allow access to this site, but I am willing to procure
whatever logs and so forth are necessary to investigate this problem....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, this is a problem on both branch and trunk.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: HTTP log of steps from comment 0

This log shows that in between the two GET requests for the document there were
in fact no other requests or responses (except for the images and stylesheet of
the doc).  This is just to demonstrate that this is not a misconfigured
redirect or anything like that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Needless to say, this creates the false impression that the site is sending data
somewhere insecurely when that is in fact not the case....

Comment 4

[Kai Engert (:KaiE:)]

Boris, I need your assistance to produce more logs.
Can you please run the test again, and run once with
  NSPR_LOG_MODULES="nsSecureBrowserUI:5"

I have been trying to reproduce this problem, but I can't.
I have provided a test case at:
  https://www.kuix.de/misc/test25/a/

Those links involve navigating between pages protected by the same password, and
pages protected by different passwords. I don't ever see an additional security
warning but the first.

I will now have a closer look at the attached http log.

Comment 5

[Kai Engert (:KaiE:)]

I need more input.

Whatever test page I create, I can not reproduce it. I even updated my test page
to include all the images and the stylesheet that are loaded on the reported page.

I was testing on Linux, as the reporter was. Besides testing my own build, with
and without the checkins from the previous days, I also tested the branch build
from 6/20 from the ftp server. None showed the reported problem.

Comment 6

[Kai Engert (:KaiE:)]

I only can speculate at the moment. Are the links on that site using JavaScript?
Could it be the same problem that was meanwhile fixed with 145730? Or could it
be the same problem that was meanwhile fixed with 148981/140836?

Comment 7

[Kai Engert (:KaiE:)]

I'm now able to reproduce the bug with the www.thawte.com website.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch

This fixes the problem for me.

Comment 9

[Kai Engert (:KaiE:)]

The cause of the problem is a behaviour of the document loader that I did not
expect, and that was not seen previously during testing.

The lock icon tracking code extracts the security state at the time it is seeing
"request stop notifications" from the document loader.

The implementation of the lock icon tracking code assumes, the stop for the top
level document request will be received, before the first stop of a sub-document
will be received.

Tests with www.thawte.com show, that this is assumption is not correct. When
clicking on a particular link on that site, that is a link to self, the
following happens:
- the request for loading the toplevel document starts
- the request for loading the stylesheet starts
- the request for loading the stylesheet stops
Rick, is this allowed/expected behaviour?

At the time the loading of the stylesheet stops, the code wants to update to the
new state.

However, the implementation assumes, the new top level state has already been
obtained. This is not true in the above scenario. Instead, the variable, which
indicates the top level state, will have been reset to the "insecure" state.
This reset is done each time loading of a new top level document starts. At the
time the stop for the stylesheet is seen, the insecure state will be used.

The solution is simple. Since the newest work on the lock icon code, it is no
longer necessary to reset the variable each time a new top level document loads.
It is sufficient, to only set this variable, when a new top level state is seen.
The variable can stay at the previously known top level state. Whenever we see a
stop for either the top level or a sub level document, the states will be
combined to make the lock icon decision, and it will get compared to the current
lock icon state.

The attached fix does no longer reset the top level state to insecure each time
a toplevel document starts. This is ok, because there is code in effect, that
will set the top level lock icon state each time loading a top level document stops.

Comment 10

[Kai Engert (:KaiE:)]

Updated test case.

Make sure you have a personal log in to www.thawte.com

Go to:
  https://www.thawte.com/cgi/personal/contents.exe

Enter your log in data and wait for the page to load.

In the second row from the top, click on "Personal Certs" which is the link to
the same page as being displayed.

Comment 11

[Javier Delgadillo]

Comment on attachment 89105 [details] [diff] [review]
Patch

r=javi

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yep.  That fixes the problem on the original site I saw it on to.  Thanks!

Comment 13

[Kai Engert (:KaiE:)]

Alec or Rick, could you please sr?

Comment 14

[Paul Wyskoczka]

Upgrading to [adt1 rtm]

Comment 15

[Alec Flett]

Comment on attachment 89105 [details] [diff] [review]
Patch

sr=alecf

Comment 16

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 17

[Jaime Rodriguez, Jr.]

This has been checked into the trunk. Resolving as fixed.

junruh - can you verify this fix on the trunk tomorrow? thanks!

Comment 18

[John Unruh]

Verified on 6/26 Win2000 trunk. Tested after logging into E-Trade, and also at 
Thawte - https://www.thawte.com/cgi/personal/contents.exe

Comment 19

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch.  Please get
drivers approval before checking in. When you check this into the branch, please
change the mozilla1.0.1+ keyword to fixed1.0.1

Comment 20

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 21

[Kai Engert (:KaiE:)]

Checked in to 1_0 branch.

Comment 22

[John Unruh]

Verified on 6/27 branch.

Comment 23

[Daniel Veditz [:dveditz]]

This recently regressed on the 1.7 and aviary1.0.1 branches. Filed a new bug
283201 just to keep things cleaner.