Last Comment Bug 329959 - Dialog Origin Spoofing not fixed on Mac for SeaMonkey
: Dialog Origin Spoofing not fixed on Mac for SeaMonkey
Status: RESOLVED FIXED
: fixed-seamonkey1.0.1, fixed-seamonkey1.1a, fixed1.8.0.2, fixed1.8.1
Product: SeaMonkey
Classification: Client Software
Component: Security (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Stefan [:stefanh]
:
Mentors:
http://secunia.com/multiple_browsers_...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-09 13:42 PST by Stefan [:stefanh]
Modified: 2006-07-07 02:53 PDT (History)
7 users (show)
kairo: blocking‑seamonkey1.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
How Firefox treats the test (27.51 KB, image/png)
2006-03-09 13:44 PST, Stefan [:stefanh]
no flags Details
How SeaMonkey treats the test (32.40 KB, image/png)
2006-03-09 13:50 PST, Stefan [:stefanh]
no flags Details
Display title inside dialog (on Mac) (3.61 KB, patch)
2006-03-12 16:40 PST, Stefan [:stefanh]
no flags Details | Diff | Review
Screenshot, showing behaviour described in comment #5 (14.87 KB, image/gif)
2006-03-13 08:55 PST, Stefan [:stefanh]
no flags Details
Corrected version (3.67 KB, patch)
2006-03-13 11:31 PST, Stefan [:stefanh]
mnyromyr: review-
neil: superreview-
Details | Diff | Review
New version (2.77 KB, patch)
2006-03-14 16:20 PST, Stefan [:stefanh]
no flags Details | Diff | Review
New version with Neil's suggestion (3.72 KB, patch)
2006-03-18 06:13 PST, Stefan [:stefanh]
neil: superreview-
Details | Diff | Review
Final version (hopefully) (2.36 KB, patch)
2006-03-19 15:31 PST, Stefan [:stefanh]
mnyromyr: review+
neil: superreview+
dveditz: approval1.8.0.2+
kairo: approval‑seamonkey1.0.1+
kairo: approval‑seamonkey1.1a+
Details | Diff | Review

Description Stefan [:stefanh] 2006-03-09 13:42:21 PST
Seamonkey, recent trunk & the 1.0 release on Mac:

Bug 301069 fixed the dialog origin spoofing on mac for toolkit. It seems that this was never fixed for seamonkey, though.
Comment 1 Stefan [:stefanh] 2006-03-09 13:44:42 PST
Created attachment 214603 [details]
How Firefox treats the test

This is how the test displays on Firefox. IMHO, we should do the  same.
Comment 2 Stefan [:stefanh] 2006-03-09 13:50:18 PST
Created attachment 214604 [details]
How SeaMonkey treats the test

This is how the test displays on SeaMonkey 1.0 (sv-SE). Same behaviour on trunk.
Comment 3 Stefan [:stefanh] 2006-03-12 15:46:26 PST
Since I'm working on a patch for this...
Comment 4 Stefan [:stefanh] 2006-03-12 16:40:58 PST
Created attachment 214861 [details] [diff] [review]
Display title inside dialog (on Mac)

The bad thing is that the title will also display in our "own" dialogs and not just in the browser. But I can't think of any other way of doing this.

I'll request review once I've checked that everyting looks ok (might need some more style tweaks).
Comment 5 Stefan [:stefanh] 2006-03-13 08:53:11 PST
If someone is trying this patch, note the error in themes/modern/global/formatting.css (patch works anyway):

+#info\.title {
+  font-weight: bold;
+{ <-------------------- :-/
+

As a side-note, I've found an odd behaviour when doing the Secunia test (trunk) - first I thought it was related to my patch - but it occurs in a trunk build from 20006021510 as well:

1) Do the test with Classic theme
2) Once the test is finished and you're back at the Secunia web site, open a new  empty tab.
3) Note that the title of the window is "..." and not "SeaMonkey". This doesn't seem to happen if I open a new tab without first doing the Secunia test...

It's quite odd, but so far I've only been able to reproduce this with the classic theme.
Comment 6 Stefan [:stefanh] 2006-03-13 08:55:59 PST
Created attachment 214914 [details]
Screenshot, showing behaviour described in comment #5
Comment 7 Stefan [:stefanh] 2006-03-13 11:31:49 PST
Created attachment 214926 [details] [diff] [review]
Corrected version

Same as previous patch - just fixed the error in modern's formatting.css and some indention in the js file. Considering that this should go into 1.8.0 and that I think we might want to re-style the mac classic dialogs anyway (larger/different font, an app icon instead of the old ones, some margin/line-height adjustments etc etc), I'll leave the styles as they are (except for the title, of course).
Comment 8 Karsten Düsterloh 2006-03-13 14:22:45 PST
Comment on attachment 214926 [details] [diff] [review]
Corrected version

>Index: themes/classic/global/mac/formatting.css
>===================================================================
>+  #info\.title

I really don't like that "dotted" id. Even though it's legal, it's bad style imo. On the contrary, I'd rather see you changing the other dotted id attributes as well to non-dotted style.

>Index: xpfe/global/resources/content/commonDialog.js
>===================================================================
> function commonDialogOnLoad()
> {
>-  // set the document title
>-  document.title = gCommonDialogParam.GetString(12);
>+  // Set the document title. On Mac, we display the title inside the dialog.
>+  if (/Mac/.test(navigator.platform)) {
>+    var dialogTitle = document.getElementById("info.title");
>+    dialogTitle.hidden = false;
>+    setElementText("info.title", gCommonDialogParam.GetString(12), true);
>+  }
>+  else {
>+    document.title = gCommonDialogParam.GetString(12);
>+  }

As you already said yourself, this will mean that *all* our common dialogs show the title inside the dialog, eg in MailNews. This is just plain ugly and not acceptable.

We could limit the "inside title" to non-chrome URIs, like this:

  if (/Mac/.test(navigator.platform)) {
  	if (!/^chrome:\/\//.test(opener.document.documentURI)) {
      var dialogTitle = document.getElementById("info.title");
      dialogTitle.hidden = false;
      setElementText("info.title", gCommonDialogParam.GetString(12), true);
    }
  }

This does fix the Secunia testcase and my MailNews testcase (group change via N). Do we need more exceptions than just chrome? Can someone spoof that?

>       <image id="info.icon" class="spaced"/>
>+      <description id="info.title" hidden="true"/>
>       <description id="info.header" class="header"/>
>       <vbox id="info.box"/>

Like I said above, I'd rather see these dots inside the id attributes killed.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2006-03-13 14:31:47 PST
what does toolkit do about the urls?
Comment 10 Karsten Düsterloh 2006-03-13 14:45:54 PST
> what does toolkit do about the urls?

Nothing.
See bug 301069 (which is the equivalent to this one).

Comment 11 neil@parkwaycc.co.uk 2006-03-13 16:20:58 PST
Comment on attachment 214926 [details] [diff] [review]
Corrected version

>+  #info\.title
This should probably use toolkit's dialogTitle class. It seems to be unique, so the #commonDialog selector isn't necessary.

>+    var dialogTitle = document.getElementById("info.title");
>+    dialogTitle.hidden = false;
unHideElementById for consistency (also put it after setElementText).

>+      <description id="info.title" hidden="true"/>
class="dialogTitle" as per toolkit.

(In reply to comment #8)
>(From update of attachment 214926 [details] [diff] [review])
>>+  #info\.title
>I really don't like that "dotted" id.
Well, you'd better persuade toolkit to change their ids too then ;-) As for the title, perhaps you should look to see if it begins with a URL prefix something like /^\w+:\/\/\w/.test(dialogTitle);
Comment 12 Karsten Düsterloh 2006-03-13 23:07:49 PST
> >>+  #info\.title
> >I really don't like that "dotted" id.
> Well, you'd better persuade toolkit to change their ids too then ;-)

Such ids just complicate things without any gain. I'm not going start a crucade, though, but when touching an infected area anyway, it should be changed...

> As for the title, perhaps you should look to see if it begins with a URL
> prefix something like /^\w+:\/\/\w/.test(dialogTitle);

I don't think that filtering on the title's content to decide if it is shown inside is a good thing, as it unnecessarily restricts our choice of title texts. 
Comment 13 Stefan [:stefanh] 2006-03-14 16:20:22 PST
Created attachment 215080 [details] [diff] [review]
New version

This version uses Mnyromyr's suggestion (chrome check) and addresses Neil's js/css comments. I kept the "dotted" id's.
Comment 14 neil@parkwaycc.co.uk 2006-03-15 04:12:30 PST
(In reply to comment #8)
>We could limit the "inside title" to non-chrome URIs, like this:
> 
>         if (!/^chrome:\/\//.test(opener.document.documentURI)) {

I don't think that works because of cases like this:

data:text/html,<body onclick="location = 'invalid:';">

(Before clicking, ensure your invalid scheme really is invalid, of course!)
Comment 15 Karsten Düsterloh 2006-03-15 12:58:37 PST
> >We could limit the "inside title" to non-chrome URIs, like this:
> > 
> >         if (!/^chrome:\/\//.test(opener.document.documentURI)) {
> 
> I don't think that works because of cases like this:
> 
> data:text/html,<body onclick="location = 'invalid:';">

Okay, we probably can leave out the \/\/ part as not every scheme needs those -   but I haven't seen any such usage of that yet for chrome.

Anyway, the point of my condition above is to turn the anti-spoof off for chrome URIs *only* (and your example is something we shouldn't do in chrome and warn if it happens in the URL bar or the browser in general), so the only question is: do we have valid *internal* callers of commonDialog.xul whose documentURI is *not* chrome? 

Your example causes the default "Alert" text to be shown inside the Mac dialog, but I think it's more a problem with the docShell passing the wrong document. nsDocShell ll. 2991-3001 are quite dubious imo - the message box is opened by the chrome window, not by the current page.
Comment 16 Karsten Düsterloh 2006-03-15 13:21:22 PST
What I mean is this: nsDocShell.cpp, ll. 2990+
>        // The prompter reqires that our private window has a document (or it
>        // asserts). Satisfy that assertion now since GetDocument will force
>        // creation of one if it hasn't already been created.
>        nsCOMPtr<nsPIDOMWindow> pwin(do_QueryInterface(mScriptGlobal));
>        if (pwin) {
>            nsCOMPtr<nsIDOMDocument> doc;
>            pwin->GetDocument(getter_AddRefs(doc));
>        }
>
>        // Display a message box
>        prompter->Alert(nsnull, messageStr.get());

Shouldn't that mScriptGlobal not actually be something like mTreeOwner->mXULWindow->mDocShell->mScriptGlobal, to get the top chrome window?
Comment 17 Stefan [:stefanh] 2006-03-16 12:02:55 PST
Comment on attachment 215080 [details] [diff] [review]
New version

Hmm, this will make us display the title inside the security alerts as well (like leaving/entering encrypted pages). Not sure what to do here.
Comment 18 Karsten Düsterloh 2006-03-16 13:22:52 PST
Okay, Neil explained my error in reasoning to me, so everybody please ignore my comments #15 and #16. Move on, nothing to see there, thank you for your cooperation. ;-)

So, the only way to pass a non-default title to the dialog is via the prompt service, which isn't available to webpages/unprivileged code. Thus, Neil's suggested test of comment #11 ( /^\w+:\/\/\w/.test(dialogTitle); ) looks fine by me - any caller from chrome who wants to have a URI title withou the URI in the dialog content on Mac can just prepend a blank. :-P
Comment 19 neil@parkwaycc.co.uk 2006-03-16 15:36:00 PST
Note that alerts from (say) data: URLs still use [JavaScript Application].
Comment 20 Stefan [:stefanh] 2006-03-18 06:13:16 PST
Created attachment 215487 [details] [diff] [review]
New version with Neil's suggestion

OK, this version uses Neil's suggested URI test instead.

> Note that alerts from (say) data: URLs still use [JavaScript Application].

Yeah, I think we're fine with that. We  didn't display [JavaScript Application] before the patch either. Since the Mac alerts are sheets popping out from the beneath the top window the title bar is obscured by top windows title bar.

Also, Mac standard  dialogs do have an empty title bar, see http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_17_section_6.html
Comment 21 neil@parkwaycc.co.uk 2006-03-18 08:24:31 PST
Comment on attachment 215487 [details] [diff] [review]
New version with Neil's suggestion

>+    if (/^\w+:\/\/\w/.test(titleString)) {
biesi, do you think this test suffices?

>+      <!-- title -->
>+      <description id="info.title" class="dialogTitle" hidden="true"/>
>       <!-- text -->
>       <description id="info.header" class="header"/>
Hmm... I did a scan of the tree, and I can't find where this existing header is used; although commonDialog.js fills it from parameter 3 of the dialog block nsPromptService.cpp never actually sets it.
Comment 22 Christian :Biesinger (don't email me, ping me on IRC) 2006-03-18 10:08:44 PST
(In reply to comment #21)
> (From update of attachment 215487 [details] [diff] [review] [edit])
> >+    if (/^\w+:\/\/\w/.test(titleString)) {
> biesi, do you think this test suffices?

What is it trying to accomplish?
Comment 23 neil@parkwaycc.co.uk 2006-03-18 15:15:06 PST
Comment on attachment 215487 [details] [diff] [review]
New version with Neil's suggestion

>+      <!-- title -->
>+      <description id="info.title" class="dialogTitle" hidden="true"/>
>       <!-- text -->
>       <description id="info.header" class="header"/>
Sorry to make you rewrite the patch again, but jag confirmed that info.header hasn't been used since bug 46859 was checked in 5 years ago, so we should reuse that instead. The good news is that the regexp check is fine :-)
Comment 24 Stefan [:stefanh] 2006-03-19 15:31:38 PST
Created attachment 215611 [details] [diff] [review]
Final version (hopefully)

Since using only info.header means that you'll have to compensate for the loss of margin (will need more space between title and text) Neil (irc) thought it was better keeping the info.title. I just addressed one comment he had - using class="header" instead of introducing the dialogTitle class. That saves us the css changes.
Comment 25 Robert Kaiser (not working on stability any more) 2006-03-20 04:59:54 PST
Comment on attachment 215611 [details] [diff] [review]
Final version (hopefully)

agreed blokcer for 1.0.1 and important scurity fix, so a=me for both SeaMonkey 1.1 and 1.0.1 - for checkin of the latter we need to wait for a word on drivers though, as the tree is closed right now.
Comment 26 Daniel Veditz [:dveditz] 2006-03-20 11:20:00 PST
Comment on attachment 215611 [details] [diff] [review]
Final version (hopefully)

seamonkey-only fixed approved, a=dveditz
Comment 27 Karsten Düsterloh 2006-03-20 13:45:52 PST
Comment on attachment 215611 [details] [diff] [review]
Final version (hopefully)

Checked in on trunk, MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
Comment 28 Stefan [:stefanh] 2006-03-20 14:12:16 PST
--> Fixed

Note You need to log in before you can comment on or make changes to this bug.