138479 - View Frame Info Security

Status: VERIFIED FIXED

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

Description

[Andreas Bokeloh]

When I select View Frame Info via the right mouse button
for an encrypted frame, the information on the security
tab is incorrect. It appears to correspond to the page
instead of the selected frame.
Tested Mozilla version is 1.0RC1 from RedHat RPM under
Linux 2.4.18

Comment 1

[Daniel Brooks [:db48x]]

-> PSM

Comment 2

[Boris Zbarsky [:bzbarsky]]

To PSM for real.  We use the "securityUI" for security information, which is
utterly wrong for a frame....

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Charles Rosendahl]

Nominating to be fixed ASAP.

Comment 5

[Stephane Saux]

Confirmed.

Moreover, if the frameset has mixed content (i.e., one of the frames came from
http, then the info says that it's secure (because the frameset is) even though
the frame itelf isn't. This should be fixed.  However:

In the case of mixed content, the lock icon is "mixed", telling the user about
that there is some unencrypted content, which should give the user plenty of
warning that he shouldn't trust any of this with a ten-foot pole.

If the content is all encrypted, the bug is less serious (but should be fixed).

Comment 6

[Kai Engert [:KaiE:]]

Our current implementation is indeed unable to provide independent security
information for each frame. We only have the security info for the whole page.

Implementing the independent solution is more work, but will be required for bug
140837.

Anyway, I think we should start by doing the simple and obvious thing: When the
page has a mixed security state, the page information text should also say that.

Comment 7

[Kai Engert [:KaiE:]]

After thinking more about it, I see two problems:

1.) We don't have separate security info for the frame.

Currently, when the user opens the page info, we always show the security info tab.

I think we should only display the security tab if the user has opened the page
info for the outermost page.

I'll try to produce a patch that does the conditional display within this bug.


2.) The page info for pages with mixed security is wrong.

I think that is a separate issue. I already filed bug 143455 about that problem.

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Suggested Fix

First, I found an off-by-one bug. That's a different problem, and not related
to this bug.

While the initial design obviously tried to obtain the hostname of the selected
frame, the off-by-one causes that to fail, I checked that.

The first part of the patch tries to find out, whether the first given argument
is null (that means, we are being called for the top level document), or
whether it is non-null (that means, we are being called for a frame).

We only provide our security infomation if we are being called for the top
level document, for frames we return null.

When opening page info for a frame, this patch hides the security tab
completely.

Comment 9

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 10

[Javier Delgadillo]

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

r=javi

Comment 11

[Kai Engert [:KaiE:]]

Jag, can you please superreview?

Comment 12

[jag (Peter Annema)]

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

sr=jag

You may want to comment out that w = ... line for the pedantics ;-)

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Corrected Patch

> You may want to comment out that w = ... line for the pedantics ;-)

Good argument. I'll be checking in a slightly rearranged patch, having that
assignment before the return call, because it will not hurt, and we reduce the
likelyhood that the logic of this assignment will be forgotten or removed, if
we comment it out.

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 85545 [details] [diff] [review]
Corrected Patch

carrying over reviews

Comment 15

[Kai Engert [:KaiE:]]

Checked in, fixed

Comment 16

[John Unruh]

Verified on 5/30 trunk Win2000 using this in-house test URL - 
https://pki.mcom.com/frames.html

Comment 17

[Kai Engert [:KaiE:]]

Marking verified as per John's comment.

Comment 18

[Kai Engert [:KaiE:]]

Raising priority of this security bug to ADT 1.

Comment 19

[scottputterman]

adding adt1.0.1+.  Please get drivers approval and check into the branch as soon
as possible.

Comment 20

[chris hofmann]

Comment on attachment 85545 [details] [diff] [review]
Corrected Patch

a=chofmann for 1.0.l
add the fixed1.0.1 keyword after checking in on  the branch, and lets get a bug
opened for figuring out how this might be done the right way to show security
info for each frame.

Comment 21

[Kai Engert [:KaiE:]]

Checked in to branch.

Comment 22

[Kai Engert [:KaiE:]]

I filed bug 149207 to request implementing correct security info for each frame.

Comment 23

[John Unruh]

Verified on branch.