140836 - https surfing: Clicking on a link early leads to "mixed/broken" lock icon

Status: VERIFIED FIXED

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

Description

[Kai Engert (:KaiE:)]

It is possible to see a "broken/mixed security" lock icon and warning, when
surfing https pages, that are perfectly secure.

However, this does only occur, when one clicks a link, or presses "stop" or
"escape" while the page is still loading.

The reason for the wrong display comes from the fact, that for summarizing the
security info, we are currently analyzing the transfer status of the page's parts.

When the user interrupts loading too early, it is possible that loading of some
parts has begun, for whom no security information is yet available.

On stop, all those connections are stopped, too. The logic tries to extract
security information, but is unable to do so. Therefore it counts the objects
which were tried to load as being insecure. And having at least one insecure
object is enough for the summarized page status going to "mixed".

While this is a bug in behaviour, it is not a bug in code. Rather the code is
currently not trying to detect this kind of failure.

Expected behaviour: Interrupted loading of objects on a page should not lead to
mixed/broken security icon.

Suggested implementation:

For each loaded subpart of a displayed page, the code should take into
consideration two more properties:
a) the loading "stop reason"
b) the fact, whether content bytes have been transfered or not.

We have to prove, but actually I believe, that a) is not necessary, looking at
b) should be enough.

I suspect, security information will be unavailable for those connections, where
the SSL handshaking did not yet finish. Those connections did not yet transfer
any content data, and therefore could not lead to insecure data being displayed.
Therefore those connections should get ignored for determining page security status.

Therefore it should be enough to implement a "per-object flag" whether bytes
were received already or not yet at all. It has to be researched, whether web
progress notification (the mechanism used by PSM to track security state)
already delivers the information or not.

Comment 1

[Stephane Saux]

Kai.

Comment 2

[Kai Engert (:KaiE:)]

I think I have a solution for this problem, my tests so far show that I have a
working fix.

Here are two test cases:
- https://www.kuix.de/misc/test16/something.html
- https://www.kuix.de/misc/test16/nothing.html

Both testcases are frame-documents. The left frame will not immediately, but
there will be a 10 second delay before the page is completely shown.


The first testcase "something" uses more text content in the left frame, there
will be some content shown in the page before the 10 second timeout is reached.

This testcase has already been working correctly. When you hit stop, we
correctly find security information for the frame, and the lock icon will
indicate a secure page.


The second testcase "nothing" will display nothing in the frame if you hit
cancel early.

Right now, and this is what this bug complains about, when you hit cancel before
the 10 second timeout, the page will go to a mixed/insecure state.

The reasons for the failure is indeed, as I explained in the previous comments,
that no data has been transfered over the channel yet, and we are unable to
retrieve security information.

But because there really has no data been transfered, there will nothing be
shown on the screen that could potentially been insecure. So we can ignore the
state of this frame, even though we are unable to obtain security information
for it.


I'll be attaching a patch that corrects the problem. For each part of the loaded
document, we remember whether data has been transfered or not, and ignore the
requests we don't care for.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Suggested Fix

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Same Patch, but ignoring whitespace changes for easier reviewing

Comment 5

[Kai Engert (:KaiE:)]

Javi, can you please review?

Comment 6

[Stephane Saux]

let's get this to bake on the trunk, have banks who reported being worried about
this bug give it a spin.  We should make sure that we review a build that
contains both this fix and the fix to bug 148610.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 85951 [details] [diff] [review]
Suggested Fix

I was told nsHashtable is not recommended for usage. I'll rework the patch to
use a better hashtable class.

Comment 8

[Kai Engert (:KaiE:)]

More testing showed that my patch does not work correctly.

I assumed that I would see the STATE_TRANSFERRING state for any request part for
the loaded document.

But I don't see that request for subdocuments like CSS or JS documents loaded
over separate URLs. So I guess I have to use something else.

Comment 9

[Kai Engert (:KaiE:)]

Rick, do you have an idea? How could I query a nsIRequest at the time I receive
the STOP progress, whether any content bytes have been transfered or not?

Comment 10

[Kai Engert (:KaiE:)]

I tried several other approaches, also using OnProgress and OnStatus events.

For some reason, external stylesheets behave special, and we do not receive
progress information using either approach.

While I first thought, this would be a general behaviour of non-visual
subdocuments, I learned that referenced external JavaScript documents behave
correctly, we do receive the expected event for them.

I have filed bug 148981.


It would be nice to have a fix for 148981, in that case, the existing approach
in this bug would already work correctly.


I was thinking how we should behave for such documents, where we do not receive
progress information.

If we don't, we must decide whether we want to assume data has been transfered
or not over the channel.

If we assume there has not, but data has been actually transfered, and in fact
used an unsecure protocol, we would incorrectly display the page as being secure.

Or, if we always assume, data has been transfered, but we are unable to extract
the security info, and there has no data been transfered, then this will lead to
a mixed state icon, which effectively would mean, this bug is not fixed correctly.

Because of that, I think we should not make assumptions. If all else fails, we
don't find a solution for 148981, and we really want to fix this bug as good as
possible, then we should use the latter approach, and accept there are still
some cases that could lead to a broken icon.


One could argue: "Hey, it's only a style sheet, not content shown on the screen!
So let's ignore it".

I would argue against that: With a stylesheet it is possible to hide information
from a page. For example, a secure web page could display a warning. The hacked
non-securely transfered stylesheet could have rules to hide that warning.

Therefore I think we should not ignore the stylesheet.

Comment 11

[Kai Engert (:KaiE:)]

My previous comment might rise the impression, there is a partial solution, even
if bug 148981 is not fixed. Thinking about it more, I would like to restate,
there is no partial solution. The approach of this suggested patch only work, if
one can rely one STATE_TRANSFERRING events to arrive. If we can't rely on that,
we must assume the worst case, check for security information, and this bug will
not be fixed in any way.


To restate what our options are:

Either:
- Fix for 148981 and a statement from the owner of the document loader team,
that we can indeed rely on STATE_TRANSFERING objects to arive, for any request
where content bytes have been transfered

Or:
- Do not rely on that event. Instead, find a way to obtain the information, by
querying an object passed in to the progress listener, whether content bytes
have been transfered or not.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Updated Patch / Only recommended with 148981 fixed

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Updated Patch / ignoring whitespace for easier review / Only recommended with 148981 fixed

Comment 14

[Kai Engert (:KaiE:)]

The previous patch would make the redness go away, but it would be a regression
without having 148981 fixed. Because we would display a page as secure, even if
stylesheets (and potential other subdocuments) have been transfered without
security.

Comment 15

[Kai Engert (:KaiE:)]

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

Comment 16

[Kai Engert (:KaiE:)]

I'm dumping some thoughts about my patch (only of interest for those trying to
follow the implementation).

While a document gets loaded, I need to keep a list of all the requests that
were seen. The list gets cleared each time a new top level document loading starts.

For each request, I remember a bool, whether data has been transfered or not.
Actually, I'm simplifying, by only remembering those URLs, where data has
actually been transfered. The existence in the list defines that data has been
transfered for that request.

Loading a document can consist of a lot of requests.

Because I don't want to be responsible for a performance regression, or cause
other side effects by delaying the freeing of request objects, I made a
simplification in the previous version of my patch. I'm not storing references
to the objects, and I'm not storing URLs, I'm just storing the numerical pointer
values to the request objects. 

But when I did that, I blindly assumed that those pointer values would not get
reused while loading a document. But I think I should not assume that. If a
request finishes and gets deleted early, the address might get re-used.

I thought I would have to use a more complicated solution, but actually there is
a simple one: I don't need to remember an object longer than it's lifetime. I
know that I will receive a stop event before an object gets deleted. The
solution is: Remove the entry from the hash table, as soon as the state has been
looked up. That way, we only remember pointer variables during the lifetime of
request objects, and we are fine by making that simplification.

The next issue with my previous patch is that I limited the detection to
sub-documents only. However, as seen in bug 125199 - see also the testcase at
https://www.kuix.de/misc/test21/ - it makes sense to include the top level
document case.

I will attach a new patch that implements the above changes.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Patch v3 / needs 148981

Comment 18

[Kai Engert (:KaiE:)]

Attachment: Patch v3 / ignoring whitespace / needs 148981

Comment 19

[Kai Engert (:KaiE:)]

javi: Can you please review?

jkeiser: You asked me to use the PLD hash table. Can you please have a look
whether I'm using it correctly?

Thanks!

Comment 20

[Javier Delgadillo]

Comment on attachment 88228 [details] [diff] [review]
Patch v3 / needs 148981

r=javi

Comment 21

[John Keiser (jkeiser)]

Looks good to me, except the ADD and REMOVE don't require a lookup first.  ADD
will  do a lookup automatically and REMOVE will just do nothing if the entry
isn't there.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v4 / needs 148981

Slightly updated patch, only removing the unnecessary lookup-before-add.

I think I can't remove the lookup-before-remove, because I need the information
whether the key was contained in the table or not.

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Patch v4 / ignoring whitespace / needs 148981

Same patch, ignoring whitespace.

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 88393 [details] [diff] [review]
Patch v4 / needs 148981

carrying forward r=javi.

Comment 25

[Kai Engert (:KaiE:)]

I just retested, the patch from this bug, combined with the patch from bug
148981, also corrects the issues with german TOPSITE https://freemail.web.de, as
reported in duplicate bug 125199.

Comment 26

[rpotts (gone)]

Comment on attachment 88394 [details] [diff] [review]
Patch v4 / ignoring whitespace / needs 148981

sr=[e-mail]

Comment 27

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 28

[Jaime Rodriguez, Jr.]

junruh - can you pls verify this fix on the trunk, then resolve this bug as
verified? thanks!

Comment 29

[John Unruh]

Verified on 6/21 trunk. The lock icon behavior has never been more accurate.

Comment 30

[Kai Engert (:KaiE:)]

Comment on attachment 88393 [details] [diff] [review]
Patch v4 / needs 148981

To avoid confusion during the approval phase, carrying over sr= from the
white-space-ignoring patch to this one.

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 88394 [details] [diff] [review]
Patch v4 / ignoring whitespace / needs 148981

Marking this patch as obsolete, since it was only a helper for easier
reviewing.

Comment 32

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 33

[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 34

[Kai Engert (:KaiE:)]

Checked in to 1_0 branch.

Comment 35

[John Unruh]

Verified on 6/25 branch.

Comment 36

[Kai Engert (:KaiE:)]

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

Comment 37

[Kai Engert (:KaiE:)]

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