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

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

(Blocks: 1 bug, {fixed1.4.2, fixed1.5, regression})

Trunk
x86
Windows XP
fixed1.4.2, fixed1.5, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

8.97 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review-
Details | Diff | Splinter Review
2.54 KB, text/plain
Details
1.34 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
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.
Depends on: 204902
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.
sorry, I should read your bug better.

You mean exception and not that Mailnews blocks the link...
(Assignee)

Comment 4

15 years ago
Created attachment 131390 [details] [diff] [review]
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.
(Assignee)

Comment 5

15 years ago
Moving to Security: General because of issue a)
Component: DOM Events → Security: General
(Assignee)

Comment 6

15 years ago
Tweaking summary to reflect issue a).
Assignee: events → bugzilla
Summary: Attempting to open link to file:// generates exceptions as well as expected message in JS console → Attempting to open link to file:// generates exceptions as well as expected secuity error message in JS console but can open link in new tab under certain circumstances
(Assignee)

Comment 7

15 years ago
Accepting.
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Attachment #131390 - Flags: review?(neil.parkwaycc.co.uk)
Iain wanted this marked as securit-sensitive (on IRC).
Group: security
Flags: blocking1.5?
Flags: blocking1.4.2?
(Assignee)

Comment 9

15 years ago
Adding Neil to cc as I've asked him to review it ;-)
(Assignee)

Updated

15 years ago
Blocks: 193255
(Assignee)

Comment 10

15 years ago
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

15 years ago
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.
Attachment #131390 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 12

15 years ago
Created attachment 131437 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #131390 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Carrying forward r= from Neil
Attachment #131437 - Flags: review+
(Assignee)

Comment 14

15 years ago
Adding jag to cc so I can ask him for sr
(Assignee)

Updated

15 years ago
Attachment #131437 - Flags: superreview?(jag)
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?
(Assignee)

Comment 16

15 years ago
Created attachment 131443 [details] [diff] [review]
Patch v0.3

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

Updated

15 years ago
Attachment #131437 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Cancelling request on patch v0.2a
Attachment #131437 - Flags: superreview?(jag)

Comment 18

15 years ago
Resummarizing.

Question for someone: why do we catch and throw?
Summary: Attempting to open link to file:// generates exceptions as well as expected secuity error message in JS console but can open link in new tab under certain circumstances → Attempting to open link to file:// can open link in new tab under certain circumstances
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.
(Assignee)

Comment 20

15 years ago
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).
(Assignee)

Updated

15 years ago
URL: file://
(Assignee)

Comment 21

15 years ago
Created attachment 131502 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #131443 - Attachment is obsolete: true
(Assignee)

Comment 22

15 years ago
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.
Attachment #131502 - Flags: review?(timeless)
(Assignee)

Comment 23

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

Comment 24

15 years ago
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 :)
Attachment #131502 - Flags: review?(timeless) → review?(caillon)
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.
Attachment #131502 - Flags: review?(caillon)
(Assignee)

Comment 26

15 years ago
Created attachment 131999 [details] [diff] [review]
Patch with revised comment
Attachment #131502 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #131999 - Flags: review?(bugmail)

Comment 27

15 years ago
need reviews and comment on this quickly if we are going to try and get it into
1.5..
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.
Attachment #131999 - Flags: review?(bugmail)
(Assignee)

Comment 29

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

Okay let's try jag
Attachment #131999 - Flags: review?(jag)
(Assignee)

Updated

15 years ago
Attachment #131999 - Flags: review?(jag) → review?(hjtoi-bugzilla)
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.
Attachment #131999 - Flags: review?(hjtoi-bugzilla) → review+
(Assignee)

Comment 31

15 years ago
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?
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

15 years ago
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.
(Assignee)

Comment 34

15 years ago
Created attachment 132139 [details]
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 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.
Attachment #131999 - Flags: review+ → review-
(Assignee)

Comment 36

15 years ago
Created attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch
(Assignee)

Comment 37

15 years ago
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
Attachment #132160 - Flags: review?(caillon)
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.
Attachment #132160 - Flags: review?(caillon) → review+
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.
Attachment #132160 - Flags: approval1.5?
Comment on attachment 132160 [details] [diff] [review]
Patch v0.4c patch for 1.5 branch

sr=jst
Attachment #132160 - Flags: superreview+

Comment 41

15 years ago
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.
Attachment #132160 - Flags: approval1.5? → approval1.5+
I checked attachment 132160 [details] [diff] [review] into the 1.5 branch so leaf can respin builds.

/be
Keywords: fixed1.5
QA Contact: ian → nobody
(Assignee)

Comment 43

15 years ago
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.
(Assignee)

Comment 45

15 years ago
It is on the trunk, but not on 1.4 branch, is it needed for that branch?
(Assignee)

Updated

15 years ago
Attachment #132160 - Flags: approval1.4.2?

Updated

15 years ago
Flags: blocking1.5?

Comment 46

15 years ago
Does this problem exist on the 1.4 branch?
(Assignee)

Comment 47

15 years ago
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

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

a=mkaply for 1.4.2
Attachment #132160 - Flags: approval1.4.2? → approval1.4.2+

Comment 49

15 years ago
Fix checked in to the 1.4 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.4.2
Resolution: --- → FIXED

Updated

14 years ago
Flags: blocking1.4.2?
Group: security
You need to log in before you can comment on or make changes to this bug.