329959 - Dialog Origin Spoofing not fixed on Mac for SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[Stefan [:stefanh]]

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]]

Attachment: How Firefox treats the test

This is how the test displays on Firefox. IMHO, we should do the  same.

Comment 2

[Stefan [:stefanh]]

Attachment: 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]]

Since I'm working on a patch for this...

Comment 4

[Stefan [:stefanh]]

Attachment: 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]]

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]]

Attachment: Screenshot, showing behaviour described in comment #5

Comment 7

[Stefan [:stefanh]]

Attachment: 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]

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)]

what does toolkit do about the urls?

Comment 10

[Karsten Düsterloh]

> what does toolkit do about the urls?

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

Comment 11

[[e-mail]]

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]

> >>+  #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]]

Attachment: 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

[[e-mail]]

(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]

> >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]

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]]

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]

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

[[e-mail]]

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

Comment 20

[Stefan [:stefanh]]

Attachment: 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

[[e-mail]]

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)]

(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

[[e-mail]]

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]]

Attachment: 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]

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]]

Comment on attachment 215611 [details] [diff] [review]
Final version (hopefully)

seamonkey-only fixed approved, a=dveditz

Comment 27

[Karsten Düsterloh]

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]]

--> Fixed