55237 - call CheckLoadURI consistently (Security related)

Status: VERIFIED FIXED

(Core :: Security, defect, P3)

Description

[Mitchell Stoltz (not reading bugmail)]

We need more consistent use of CheckLoadURI. It should probably be called
everywhere a necko load is initiated from web content. Most call sites are
checked (links, script tags, style tags) but some are not (images, "open link in
new window"). This should be consistent. It would make sense to combine this
with Shaver's nsContentPolicy functionality to cut down on the number of calls
occurring at these locations.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Adding rtm keyword.

Comment 2

[selmer (gone)]

Are there any user-visible consequences of this bug?  Seems like this should be
minus.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

The consequences are security holes. Let me add a specific case: right-clicking a 
link and choosing "open in new window" is not sbject to the URI check. THis could 
allow the loading of chrome documents or local files in an unsafe manner if a 
user could be convinced to do the right-click. I should have a fix ASAP. 

Comment 4

[Ben Bucksch (:BenB)]

outch, this would IMO be a pull-of-the-wire bug in any case. I do "open in new
window" for every second link, and I'm not alone.

Comment 5

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch: adding CheckLoadURI to open-in-new-window.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

I have a patch here for the most egregious omission, with r=beard. This patch
makes a call to CheckLoadURI in the function which gets called by the "open link
in new window" command. I had to make nsScriptSecurityManagr scriptable for this
to work.

This will prevent a malicious page from loading a chrome document or a local
file as a link, which is dangerous.

Comment 7

[Brendan Eich [:brendan]]

Couple of comments:

- use const, not var, for all those nsIStandardURL, etc., constants that you
added to openNewWindowWith.

- [nit] indent the second line of parameters to hang under the first line's
params (to start after the column that contains the '(' in the first line):

+    [noscript] void CheckFunctionAccess(in JSContextPtr cx, in voidPtr funObj,
                   in voidPtr targetObj);
 
Do this fast, and sr=[e-mail], get in the trunk, and [rtm+].

/be

P.S.  Thanks for the mid-air collision, jce2 -- why are you adding (Security
related) to the summary of a Security: General component bug?

Comment 8

[John Bandhauer]

sr=jband

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Attachment: final patch with Brendan's suggestions.

Comment 10

[Jason Eager]

So that people understand that this actually deserves to be a rtm bug and isn't
just "a nice thing to have."

Comment 11

[Brendan Eich [:brendan]]

jce2: The Security: General component is sufficient.

mstoltz, for the record: sr=[e-mail] on the final patch.  It doesn't
need r= again.  Get this in the trunk, ok?

/be

Comment 12

[selmer (gone)]

rtm++, please check into the branch ASAP.

Comment 13

[Mitchell Stoltz (not reading bugmail)]

Checked into branch, removing rtm++. I'm leaving this open because this patch
doesn't work on the trunk, it causes a strange exception. Working on it.

Comment 14

[John Unruh]

Mass changing QA to ckritzer.

Comment 15

[Jesse Ruderman]

Is it a regression that images urls aren't checked?  IIRC the attachments for 
bug 38643 (file:///// images freezing Win98) had to be saved locally in order 
to crash Mozilla and Win98.

The "Open Link in New Window" problem was previously discussed bug 35273.  
35273 was marked invalid, and the reason given was that "Open Link in New 
Window" doesn't give the page any more DOM access to the chrome/file page than 
tricking the user into typing a chrome/file url into the location bar.  So... 
is it a problem that webpages can get you to view or download local files?

Comment 16

[Ben Bucksch (:BenB)]

Nominationg for Mozilla0.8. I guess, some nice attacks could be constructed with
this bug.

Comment 17

[Mitchell Stoltz (not reading bugmail)]

Jesse,
  It's not about DOM access, it's about attacks like <SCRIPT
SRC="file:///home/mstoltz/.mozilla/default/prefs.js"> which can be used to read
your prefs file. Plus more heinous attacks involving chrome (similar to bug
56009). That's why it's bad that web content can get you to view or load local
files, even if the malicious script itself can't read the local file.

Comment 18

[R.K.Aa.]

modifications made in contentAreaUtils.js are causing bug 62964

Comment 19

[Jesse Ruderman]

Right now, clicking on the link from
data:text/html,<a href="chrome://navigator/content/navigator.xul">asdf</a>
doesn't do anything.  Selecting "Open Link in New Window" gives an error 
mentioning nsIScriptSecurityManager.CheckLoadURI.  Selecting "Edit Link in 
Composer" tries to open the browser in Composer, but Composer complains that it 
can't edit navigator.xul (after dispalying it and putting a few JS errors on 
the console).

Comment 20

[James Green]

Is this still doable by the branch date? Is there much risk in people seeing
this and exploiting it? If so it may be worth the effort for 0.8. If not
nomitate for 0.9.

Comment 21

[Ben Bucksch (:BenB)]

Mass-change: Do not remove nominations (even if Milestone passed). Readding
mozilla0.8 nomination.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Mass adding mozilla0.9 keyword (mass changing milestone doesn't seem to work).

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Setting milestone to Moz0.9.

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Attachment: Rewrite of CheckLoadURI, more checks for context menu

Comment 25

[Mitchell Stoltz (not reading bugmail)]

The above patch is a rewrite of CHeckLoadURI to use Gagan's schemeIs() function,
which avoids sting copying and strcmp. This will run faster. It also includes
error reporting to the JS console when the check fails(bug 40538). The check
code in the content area context menu is simplified and another check added for
"Edit Link." Finally, I've included David Baron's leak fixes in
nsScriptSecurityManager (bug 76091), which are already reviewed.

Comment 26

[David Hyatt]

sr=hyatt

Comment 27

[Mitchell Stoltz (not reading bugmail)]

Attachment: Revised patch with Patrick's suggestions

Comment 28

[Patrick C. Beard]

r=beard (FWIW)

Comment 29

[Mitchell Stoltz (not reading bugmail)]

Almost all of the bases are covered. Still waiting on a check in libpr0n, but
that's covered in a separate bug, so I'm closing this one out.

Comment 30

[Ben Bucksch (:BenB)]

> Almost all

Are any known checks left (apart from libimg2)? If there is a risk that you/we
missed some calls, shouldn't we file a bug to track/remind of the review?

Comment 31

[ckritzer (gone)]

Mitch, would this bug manifest itself as a captured right-click?  I've read 
through the bug history, and it's not completely clear to me how this would be 
tested for verification.  Any suggestions?

Comment 32

[Mitchell Stoltz (not reading bugmail)]

Chris,
  This bug is pretty general. Too general, in fact. There are still a couple of
places where we're not doing the right thing (images, which has some other
problems right now, is bug 69070). It's better if I file those as separate bugs.
To test what I've already done, try various ways of getting remote (http)
content to cause a load of local (file) content: with <A HREF="file:..., with
window.location="file:..., <SCRIPT SRC="file:..., and any other ways you can
think of.

Comment 33

[ckritzer (gone)]

Attachment: testcase

Comment 34

[ckritzer (gone)]

Spiffy.  Works like a charm with the above added testcase.

Marking VERIFIED FIXED on:
-MacOS91 2001-05-21-15-trunk MOZILLA
-Win98SE 2001-05-22-06-trunk
-LinRH62 2001-05-22-05-trunk