204902 - checkloaduri doesn't work for link from Mailnews

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Matthias Versen [:Matti]]

win2k build 20030508..

a) send yourself a mail with a a file:// link
b) click in the mail on this link (le3ft click)
c) File will open in the browser window

Expected : 
Doesn't load the link and get a a security error in the JS Console :
Security Error: Content at
mailbox:///C|/Dokumente%20und%20Einstellungen/Matti/Anwendungsdaten/Mozilla/Profiles/Matti2/gnj7ycwv.slt/Mail/server600-1/Inbox?number=575105
may not load or link to file:///c:/12.bat.


FYI: The URL is correctly blocked if you use "opne in new Tab/window" from the
context menu

Comment 1

[Mitchell Stoltz (not reading bugmail)]

That's not good...

Comment 2

[Matthias Versen [:Matti]]

FYI: I don't know if this is wanted or not but there should not be a difference
between "open in new Tab/window" and a left click. I don't know if there is a
true security violation...

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Confirmed, and this is definitely a regression since we used to do the right thing. 

Comment 4

[Mitchell Stoltz (not reading bugmail)]

When we click a link in the browser, CheckLoadURI is called from here:

nsGenericElement::TriggerLink line 2992
nsGenericHTMLElement::HandleDOMEventForAnchors line 1541 + 45 bytes
nsHTMLAnchorElement::HandleDOMEvent line 355
PresShell::HandleEventInternal line 6380 + 47 bytes

Strangely, in the mail window, when I mouseover the file:// link, we call
TriggerLink to set the status bar, but when  click on it, we don't appear to be
going through the above code path at all. How are link clicks handled in mail
messages? CCing some people who may know.

Comment 5

[(not reading, please use [e-mail] instead)]

I've changed this behaviour accidentally, for a performance / correctness issue.

See http://bugzilla.mozilla.org/show_bug.cgi?id=199360

We are still going through some traditional code paths to open these links:

+function openTopBrowserWith(url)
+{
+  var windowMediator =
Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator);
+  var browserWin = windowMediator.getMostRecentWindow("navigator:browser");
+
+  // if there's an existing browser window, open this url in one
+  if (browserWin)
+    browserWin.getBrowser().loadURI(url); // Just do a normal load.
+  else
+    window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
null, null);
+}

Those should be checking the load uri?  If not, we might have other code paths
(in nav or mail) that don't check the uri

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Yes, I think we should be calling CheckLoadURI in the function you mentioned
above. If this sounds right to you, I'll write up a patch.

Comment 7

[(not reading, please use [e-mail] instead)]

see 
http://lxr.mozilla.org/mozilla/source/browser/base/content/contentAreaUtils.js#114

we have:

 89 function openNewWindowWith(url, sendReferrer) 
 90 {
 91   urlSecurityCheck(url, document);

so I think just a well placed call to urlSecurityCheck() in openTopBrowserWith()
will do the trick.

but that will throw, and we need to double check that the callers to
openTopBrowserWith() handle it right (as the callers to openNewWindowWith do)

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - call URISecurityCheck in openTopBrowserWindow

I didn't do anything special to handle the exception, but it seems to be caught
and displayed just fine, just like openNewWindowWith().

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 124598 [details] [diff] [review]
Patch - call URISecurityCheck in openTopBrowserWindow

Requesting reviews.

Comment 10

[Samir Gehani]

adt: nsbeta1+/adt3

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 124598 [details] [diff] [review]
Patch - call URISecurityCheck in openTopBrowserWindow

r=sspitzer, assuming you veriied that the "ok" cases worked fine, and the
"failed" cases (where we threw the exception) did what we want.

please confirm that you tested that.

for the "failed" case, the caller of openTopBrowserWindow() is
messagePaneOnClick(event);

if it throws an exception, this line of messagePaneOnClick() won't get called:

236   openTopBrowserWith(href);
237   // we want to preventDefault, so that in
238   // nsGenericHTMLElement::HandleDOMEventForAnchors(), we don't try to
handle the click again
239   event.preventDefault();

is that a problem?  or is that desired?  do you want to handle it twice?

Comment 12

[Mitchell Stoltz (not reading bugmail)]

I tested as many "ok" and "failed" cases as I could find, all are working OK. My
test page (internal) is http://warp/u/mstoltz/bugs/clu.html. Mail that page to
yourself to run the test.

As for the preventDefault, I tried moving it before the openTopBrowserWith call,
but it didn't seem to behave any differently, so for simplicity's sake I'm going
to leave it as is.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 124598 [details] [diff] [review]
Patch - call URISecurityCheck in openTopBrowserWindow

a=mkaply for 1.4

Comment 14

[bobj]

a=adt

Comment 15

[Mitchell Stoltz (not reading bugmail)]

Checked in, trunk and 1.4 branch.

Comment 16

[(not reading, please use [e-mail] instead)]

thanks for testing thoroughly.

Comment 17

[Matthias Versen [:Matti]]

This works now but I get 3 Errors in the JS Console if I left-click a file link
in a mail (text) :

1)
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIWebNavigation.loadURI]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/browser.xml#browser.loadURIWithFlags() ::
loadURIWithFlags :: line 5"  data: no]
Source File: chrome://global/content/bindings/browser.xml#browser.loadURIWithFlags()
Line: 5

2)
Security Error: Content at
mailbox:///C|/Dokumente%20und%20Einstellungen/Matti/Anwendungsdaten/Mozilla/Profiles/Matti2/gnj7ycwv.slt/Mail/server600-1/Inbox?number=1212634
may not load or link to file://c/.

3)
Error: uncaught exception: Load of file://c/ denied.

I get 4 security errors with open in new tab/window :
Error 2, Error 3, Error 2, Error 3

I dunno if this is wanted/known but marking verified

Comment 18

[Mitchell Stoltz (not reading bugmail)]

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

Comment 19

[(not reading, please use [e-mail] instead)]

[e-mail], those errors sound like a result of us not catching the
exception we throw.

let's lot a spin off bug for that, but as long as things "work" for the user,
I'm ok with this as is, for 1.4 final.

Comment 20

[Mitchell Stoltz (not reading bugmail)]

That's what I thought too, but I tried catching the exception and I still saw
the 3 errors in the console.

Comment 21

[beanladen]

Does the patch respect the setting of security.checkloaduri ?

Comment 22

[Paul Wyskoczka]

Matti, was this fix verified on the trunk only?  Did you check the branch as well?

Comment 23

[Matthias Versen [:Matti]]

Verified with the trunk and the 1.4 branch

Comment 24

[Paul Wyskoczka]

Adding verified1.4 per Matti comments.  Thx Matti.

Comment 25

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.