298320 - Camino trunk still vulnerable to supposedly-patched Dialog Box Spoofing Test

Status: VERIFIED FIXED

(Camino Graveyard :: Security, defect, P1)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(Since the vulnerability is public, I'm not ticking the security box)

This is listed on http://secunia.com/advisories/12712/ as having been patched in
CVS for Camino, but 2005062008 (v0.9a1) is still vulnerable.

The equivalent Deer Park nightly switches focus back to the Secunia tab when it
displays the dialogue that captures keystrokes; Camino continues to show PayPal.
 (The first time I tried it, PayPal was slow to load, so I had a blank, loading
tab focused when the dialogue was capturing my keystrokes, but still....)

Comment 1

[Simon Fraser [no longer active]]

How about we show the url or title of the parent document, and its favicon in
the JS alert window? (as well as switching tabs).

Comment 2

[Simon Fraser [no longer active]]

Attachment: Simple testcase

Comment 3

[Simon Fraser [no longer active]]

Attachment: Patch to switch to the tab showing the sheet

This patch makes Camino switch to the tab for which the sheet is being shown.
It does this by returning the CHBrowserView, rather than an NSWindow to a
GetSiteWindow request, allowing the prompt service code to fire methods on the
CHBrowserView that propagate up to the window controller. The BWC then switches
to the appropriate tab.

Comment 4

[Josh Aas]

Comment on attachment 187325 [details] [diff] [review]
Patch to switch to the tab showing the sheet

+  while ((curTabItem = [tabsEnum nextObject]))
+  {

Parens on the same line please

+- (void)didDismissPromptForBrowser:(BrowserWrapper*)inBrowser
+{
+}

I can see why such a callback follows logically, but can't we add it if/when we
need it? I don't really care I guess, just throwing that out there. I doubt the
compiler will optimize it out, and we call every time we dismiss the prompt,
unnecessarily.

+// XXX unused now

I don't really like that comment because it's not worth pointing that out, and
potentially confusing. We have to have that method there (I think), but it
shouldn't matter to potential users that it's currently unused. If someone
wants to know for some reason they can use lxr.

+- (void)onFoundShortcutIcon:(NSString*)inIconURI
+{
+}

What is that all about?

Mostly nits, otherwise looks good. Going to test it, then decide on r.

Comment 5

[Josh Aas]

Patch no longer applies. Needs to be reworked.

Comment 6

[Simon Fraser [no longer active]]

(In reply to comment #4)
> (From update of attachment 187325 [details] [diff] [review] [edit])
> +  while ((curTabItem = [tabsEnum nextObject]))
> +  {
> 
> Parens on the same line please

Much of the other code in thiso file has open braces on new line, so I was
following that style (which I find more readable).

> +- (void)didDismissPromptForBrowser:(BrowserWrapper*)inBrowser
> +{
> +}
> 
> I can see why such a callback follows logically, but can't we add it if/when we
> need it? I don't really care I guess, just throwing that out there. I doubt the
> compiler will optimize it out, and we call every time we dismiss the prompt,
> unnecessarily.

It's there for symmetry (a good design principle), and may well get used in
future. For example, if we decided we wanted to switch back to the previously
frontmost tab after showing the sheet, we'd do it here. I think it's often best
to do such things up-front, because then they get done correctly. Later
additions don't always follow the spirit of the earlier changes.

> +// XXX unused now
> 
> I don't really like that comment because it's not worth pointing that out, and
> potentially confusing. We have to have that method there (I think), but it
> shouldn't matter to potential users that it's currently unused. If someone
> wants to know for some reason they can use lxr.

I should just remove this method.

> +- (void)onFoundShortcutIcon:(NSString*)inIconURI
> +{
> +}
> 
> What is that all about?

Fixing warnings about a class having an incomplete implementation of a protocol.

Comment 7

[Simon Fraser [no longer active]]

Attachment: Patch updated to trunk

Comment 8

[Simon Fraser [no longer active]]

Checked in.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

One slightly annoying side-effect of this fix is that if I open links in bg tabs
and one of them wants to set a cookie, I'm switched from the tab I was reading
to the tab throwing the cookie sheet (I have "Ask before accepting each cookie"
on). I used to be able to just hit return or escape and keep reading the page I
was reading.

I don't suppose there's a way to tell a cookie sheet from a JavaScript sheet or
an HTTPAuth login sheet, is there?

Comment 10

[John Bailey]

(In reply to comment #9)
> One slightly annoying side-effect of this fix is that if I open links in bg tabs
> and one of them wants to set a cookie, I'm switched from the tab I was reading
> to the tab throwing the cookie sheet (I have "Ask before accepting each cookie"
> on). I used to be able to just hit return or escape and keep reading the page I
> was reading.
> 
> I don't suppose there's a way to tell a cookie sheet from a JavaScript sheet or
> an HTTPAuth login sheet, is there?

Yes, this annoys me too!  I'd think it's OK for JavaScript sheets, but not for
cookie prompts.  It can make tabbed browsing annoying.  It doesn't bring other
'windows' (opposed to tabs) to the front, does it?

What would be optimal would be for the sheet for the cookie to slide down ONLY
when YOU bring it's tab to the front.  Or have a pref to either do it that way,
or the old way were you get the sheet in whatever tab you are already in (this
is what i would want, just the old way), so you can have your tab continue to
load in the background.  Just don't bring the tab forward for a cookie.  And if
it has to be this way, then any way to give the last tab focus after making your
cookie "deny/accept" desicion. (this would be a lot of tab jumping though,
pretty clunky if you ask me...)

If nothing happens with this here, I am opening a bug.

Comment 11

[Simon Fraser [no longer active]]

We can't tell that it's a cookie dialog; it just comes through nsIPrompt.

Comment 12

[John Bailey]

(In reply to comment #11)
> We can't tell that it's a cookie dialog; it just comes through nsIPrompt.

Would it make sense to only display ANY sheet for a tab only when YOU click on
it, instead of bringing the tab forward?  When an app controls what you are
viewing without your control, it can frustrate users.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #12)
> (In reply to comment #11)
> > We can't tell that it's a cookie dialog; it just comes through nsIPrompt.
> 
> Would it make sense to only display ANY sheet for a tab only when YOU click on
> it, instead of bringing the tab forward?  When an app controls what you are
> viewing without your control, it can frustrate users.

Safari does that, I see, but with Gecko I think we run into bug 123908 and the
bugs it depends on, which I imagine require a major re-architecting of various
parts of Core (and why the fixes for these problems have been "switch to the
active tab" and "display the URL of the site presenting the JS prompt")... :-(

The other "problem" with only displaying a sheet when you click on it is that
when you get to that tab, the page load has been held up by the cookie dialogue,
so you're back to waiting for the content again instead of having it actually
load in the background....

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I've confirmed bug 302596, so let's take any future conversation about the nasty
cookie side effects there....

Comment 15

[Samuel Sidler (old account; do not CC)]

Attachment: Patch for 1.7 Branch

This is the patch for the 1.7 branch. It has not been tested, but it will apply cleanly. :)

One thing to note: The changes in BrowserWindowController.mm are for tabbed browsing, which changed significantly post-1.7 branch. So I'm not sure if the changes here are valid since, well, I don't code. :)

Requesting review (meaning testing) from Smokey and super review from Simon with the note I listed above.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 213278 [details] [diff] [review]
Patch for 1.7 Branch

Applies, builds, and works.  

I'm not sure what I should test as far as unintended side effects/possible breakage are concerned, but the expected results (cookie sheets and JS prompts switch to the tab that spawned them, and the latter identifies the hostname) do occur.

Comment 17

[Samuel Sidler (old account; do not CC)]

Comment on attachment 213278 [details] [diff] [review]
Patch for 1.7 Branch

timeless checked this into the MOZILLA_1_7_BRANCH at 2006-02-28 23:10.