219029 - Attempting to open link to file:// can open link in new tab under certain circumstances

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Ian Neal]

Tested on BuildID 2003090204, BuildID 2003091104-trunk and BuildID
2003091022-1.5 branch on Windows XP SP1.
Steps to reproduce:
1. Get sent an email with link to a file://
2. Click on link
3. Open JS console

Expected result:
1. Following message in JS console:
Security Error: Content at imap://ian@mail1:143/fetch%3EUID%3E/INBOX%3E44166 may
not load or link to file:///H|/Management/GUIDANCE%20NOTES.doc.

Actual result:
1. Following message in JS console:
Security Error: Content at imap://ian@mail1:143/fetch%3EUID%3E/INBOX%3E44166 may
not load or link to file:///H|/Management/GUIDANCE%20NOTES.doc.
Error: [Exception... "'Load of file:///H|/Management/GUIDANCE%20NOTES.doc
denied.' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult:
"0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]
Security Error: Content at imap://ian@mail1:143/fetch%3EUID%3E/INBOX%3E44166 may
not load or link to file:///H|/Management/GUIDANCE%20NOTES.doc.
Error: uncaught exception: Load of file:///H|/Management/GUIDANCE%20NOTES.doc
denied.

Comment 1

[Ian Neal]

Regression appears to happen between BuildID 2003060108 and BuildID 2003061108,
looks like from checkin to bug 204902 which didn't have the subsequent bug spun
off from what I can see. CCing relevant people from that bug.

Comment 2

[Matthias Versen [:Matti]]

yes, but that's expected !
checkloaduri was broken for left click bot not for opening in a new tab/window
and I filed that bug 204902 to correct that.
(Either remove the check for new Tab/new wndow or add it again for the left click
and they added the broken check again for the left click.

Comment 3

[Matthias Versen [:Matti]]

sorry, I should read your bug better.

You mean exception and not that Mailnews blocks the link...

Comment 4

[Ian Neal]

Attachment: Patch v0.2

This patch addresses several issues:
a) Security issue - previously if you only have non-browser windows open and
you select "Open Link in New Tab" we would open a new window no matter what the
link was. This patch adds a call urlSecurityCheck prior to opening a new
window.
b) Duplicating calls to urlSecurityCheck - previously we'd call
urlSecurityCheck on a click event and again (well in most circumstances - see
a) when opening a tab or window. This patch removes the call to
urlSecurityCheck from within the function handleLinkClick.
c) Throwing an exception as well as a security error - previously as well as
generating a security error we would throw an exception. This patch changes the
throw into a dump and makes the function urlSecurityCheck return a boolean
value, true if it's okay to load the url, false if it's not.

I'm pretty sure I've captured all ways mozilla opens links unless we let pages
alter the context menu, which I don't think we do.

Comment 5

[Ian Neal]

Moving to Security: General because of issue a)

Comment 6

[Ian Neal]

Tweaking summary to reflect issue a).

Comment 7

[Ian Neal]

Accepting.

Comment 8

[Bradley Baetz (:bbaetz)]

Iain wanted this marked as securit-sensitive (on IRC).

Comment 9

[Ian Neal]

Adding Neil to cc as I've asked him to review it ;-)

Comment 10

[Ian Neal]

Hmmm, added benefit Neil has just pointed out:
shift-left click's behaviour is now consistent with the behavior of right-click
and "Save Link Target As...". Previously the former would just fail and the
latter would bring up a "Save As" dialog box, with the patch they both bring up
a "Save As" dialog box.

Comment 11

[[e-mail]]

Comment on attachment 131390 [details] [diff] [review]
Patch v0.2

>   try {
>     secMan.checkLoadURIStr(sourceURL, url, nsIScriptSecurityManager.STANDARD);
>   } catch (e) {
>-    throw "Load of " + url + " denied.";
>+    dump("Load of " + url + " denied.");
>+    return false;
>   }
>+  return true;
Nit: Since checkLoadURIStr already dumps the message to both the console and
the JS console I don't think you should dump anything yourself.

Comment 12

[Ian Neal]

Attachment: Revised Patch v0.2a

Tweaked patch, adding missing \n to dump which is still needed as
checkLoadURIStr only outputs to JS console in non-debug builds.

Comment 13

[Ian Neal]

Comment on attachment 131437 [details] [diff] [review]
Revised Patch v0.2a

Carrying forward r= from Neil

Comment 14

[Ian Neal]

Adding jag to cc so I can ask him for sr

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Um, why are you not throwing anymore from urlSecurityCheck?  That kind of scares
me and violates one of the basic premises of the caps model which is to throw
for security failures.  It opens up a huge can of worms for new callers to be
added which do not check the return value.  Additionally, and more to the point,
if there are any extensions out there which use this and (rightly) expect it to
throw, you will now potentially open them up to security holes.  I seem to
remember there are extensions which use this.  Did you get all the callers?

Comment 16

[Ian Neal]

Attachment: Patch v0.3

Revised patch that throws instead of just dumping just in case the previous
change breaks any extensions expecting a throw.

Comment 17

[Ian Neal]

Comment on attachment 131437 [details] [diff] [review]
Revised Patch v0.2a

Cancelling request on patch v0.2a

Comment 18

[[e-mail]]

Resummarizing.

Question for someone: why do we catch and throw?

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Neil: because otherwise the error message in the JS Console will make it look
like a chrome error rather than a content error.

Error: uncaught exception: [Exception... "foo"  location: "JS frame ::
chrome://foo.js :: etc]

That would probably confuse users into thinking its a browser bug when it is a
problem with the site.

Comment 20

[Ian Neal]

A user might still find the following exceptions confusing, the former more than
the latter I suspect.

From a left click on a link they'd get given: 
Error: [Exception... "'Load of file:///C:/MUD/NEWS.TXT denied.' when calling
method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x8057001e
(NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]

and from a right click, open link in a new...:
Error: uncaught exception: Load of file:///C:/MUD/NEWS.TXT denied.

This is as well as for both:
Security Error: Content at imap://ian@mail1:143/fetch%3EUID%3E/INBOX%3E3334 may
not load or link to file:///C:/MUD/NEWS.TXT.

But at least with this latest patch, the user only gets the one pair of errors
in the JS console, rather than two pairs (or none at all).

Comment 21

[Ian Neal]

Attachment: Patch v0.4

This patch addresses the issue of an exception being thrown in a listener by
removing the listener and changing the function that onclick calls to be an
amended version of the listener's original function that in turn calls the
onclick's original function.

Comment 22

[Ian Neal]

Comment on attachment 131502 [details] [diff] [review]
Patch v0.4

Oh and many thanks to Neil for point out this solution to me on IRC.

Comment 23

[Ian Neal]

To keep "Save As" consistent (as per comment#10), we do not want to call
urlSecurityCheck prior to calling saveURL

Comment 24

[timeless]

Comment on attachment 131502 [details] [diff] [review]
Patch v0.4

ok, i think this looks right but it now has too much dom stuff for my taste :)

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 131502 [details] [diff] [review]
Patch v0.4

So I'm wondering if it would be good to just keep a global variable
|canLoadURLChecked| or something.  This way we can leave the checks to happen
as early as they are, and avoid the double check.  Resetting it may be a pain
though, so maybe not....


>Index: xpfe/communicator/resources/content/contentAreaClick.js

>+    // Checking to make sure we are allowed to open
>+    // this URL (call to urlSecurityCheck) is now
>+    // done within openNew... functions

Lose the "now".  If someone cares about "before", they can check past
revisions.  Also, it sounds like you mean |openNew*|.  The comment is overly
long.  Just say

  // We ensure we can load |href| within the openNew* functions


The rest seems to look okay.  r=caillon, but get another reviewer as well if
you can.

Comment 26

[Ian Neal]

Attachment: Patch with revised comment

Comment 27

[chris hofmann]

need reviews and comment on this quickly if we are going to try and get it into
1.5..

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 131999 [details] [diff] [review]
Patch with revised comment

Sorry, I don't feel comfortable reviewing this code since it's a security issue
and I don't this code well enough.

Comment 29

[Ian Neal]

Comment on attachment 131999 [details] [diff] [review]
Patch with revised comment

Okay let's try jag

Comment 30

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 131999 [details] [diff] [review]
Patch with revised comment

r=heikki

My only concern is that handleLinkClick would change behavior in cases where
event.button is not 0 or 1. Previously it would throw, now it wouldn't. To be
safe, I would add a default: case with the security check. It seems unlikey
this would be a security problem either way, but adding the check would take
the more paranoid approach.

Another thing: it seems like text to thro from urlSecurityCheck() is hardcoded
to English. If we ever display this anywhere, it should be obtained from a
properties file so it could be translated. Please file a new bug for this.

Comment 31

[Ian Neal]

The question is do we want to throw on a right click as the throw doesn't
actually stop the context menu from being displayed it just adds two messages to
the JS console? Does handleLinkClick need it's 3rd argument anymore as it is now
only called from one place and that argument is null from there?

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

Why does it spit JS errors but not stop execution?  Is there something silently
catching an exception which should not be?  Venkman's stop for exceptions/errors
would be useful here....

Comment 33

[[e-mail]]

right-clicks and context menus are separate events, throwing from one won't stop
the other; IIRC context menus are summoned differently on each platform anyway:
Linux - right mouse down; Windows - right mouse up; Mac - hold left mouse down.

Comment 34

[Ian Neal]

Attachment: Output from Venkman for stops on error/exception

As requested output from Venkman that shows stops 4 times for the exceptions
when right clicking, in Windows XP, on a file:// link.

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 131999 [details] [diff] [review]
Patch with revised comment


>-    // Make sure we are allowed to open this URL
>-    urlSecurityCheck(href, document);
>+    // We ensure we can load |href| within the openNew* functions
> 
>     switch (event.button) {                                   
>       case 0:                                                         // if left button clicked
>         if (event.metaKey || event.ctrlKey) {                         // and meta or ctrl are down
>           if (openNewTabOrWindow(event, href, true))
>             return true;
>         } 
...

I was just about to land this and decided to have another look at the patch,
and found this:

240	    if (saveModifier) { 					  // if
saveModifier is down
241	      saveURL(href, linkNode ? gatherTextUnder(linkNode) : "");
242	      return true;
243	    }

which used to have a security check but does not any longer.  This would be
extremely bad to forego a security check on since a webpage can synthesize a
click MouseEvent, with the right event flags and would be then able to load any
page including chrome.

So review- from me.

Comment 36

[Ian Neal]

Attachment: Patch v0.4c patch for 1.5 branch

Comment 37

[Ian Neal]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

This patch just fixes the bare minimum for the 1.5 branch

Comment 38

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

I feel much better taking this for the branch.	r=caillon.

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

This patch removes the extra cruft and is just the simple, one-line fix of the
main problem.

Comment 40

[Johnny Stenback (:jst)]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

sr=jst

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

a=asa (on behalf of drivers) for checkin to the 1.5 branch.

Comment 42

[Brendan Eich [:brendan]]

I checked attachment 132160 [details] [diff] [review] into the 1.5 branch so leaf can respin builds.

/be

Comment 43

[Ian Neal]

Reworking of onclick/listener has been spun off into bug 220303 and of multiple
calls to urlSecurityCheck into bug 220711.
Patch v0.4c has been checked into trunk.

Comment 44

[Christopher Aillon (sabbatical, not receiving bugmail)]

Is this fixed then?

Comment 45

[Ian Neal]

It is on the trunk, but not on 1.4 branch, is it needed for that branch?

Comment 46

[Mike Kaply [:mkaply]]

Does this problem exist on the 1.4 branch?

Comment 47

[Ian Neal]

Yes it does, this regression was caused by the checkin for bug 180377 on 15th
Nov 2002. Checked on BuildID 2003060405 1.4 and that definitely has the problem.

Comment 48

[Mike Kaply [:mkaply]]

Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

a=mkaply for 1.4.2

Comment 49

[[e-mail]]

Fix checked in to the 1.4 branch.