Closed Bug 55237 Opened 19 years ago Closed 19 years ago

call CheckLoadURI consistently (Security related)


(Core :: Security, defect, P3)






(Reporter: security-bugs, Assigned: security-bugs)




(5 files)

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.
Adding rtm keyword.
Keywords: rtm
Target Milestone: --- → M19
Are there any user-visible consequences of this bug?  Seems like this should be
Whiteboard: [need info]
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. 
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.
QA Contact: czhang → junruh
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.
Whiteboard: [need info] → [rtm+]
Summary: call CheckLoadURI consistently → call CheckLoadURI consistently (Security related)
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, get in the trunk, and [rtm+].


P.S.  Thanks for the mid-air collision, jce2 -- why are you adding (Security
related) to the summary of a Security: General component bug?
So that people understand that this actually deserves to be a rtm bug and isn't
just "a nice thing to have."

jce2: The Security: General component is sufficient.

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

rtm++, please check into the branch ASAP.
Whiteboard: [rtm+] → [rtm++]
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.
Keywords: rtm
Whiteboard: [rtm++]
Mass changing QA to ckritzer.
QA Contact: junruh → ckritzer
Blocks: 29079
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?
Nominationg for Mozilla0.8. I guess, some nice attacks could be constructed with
this bug.
Keywords: mozilla0.8
  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.
modifications made in contentAreaUtils.js are causing bug 62964
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).
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.
Depends on: 69070
Keywords: mozilla0.9
Keywords: mozilla0.8.1
Keywords: mozilla0.8
Mass-change: Do not remove nominations (even if Milestone passed). Readding
mozilla0.8 nomination.
Keywords: mozilla0.8
Mass adding mozilla0.9 keyword (mass changing milestone doesn't seem to work).
Setting milestone to Moz0.9.
Target Milestone: --- → mozilla0.9
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.
r=beard (FWIW)
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.
Closed: 19 years ago
Resolution: --- → FIXED
> 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?
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?
  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.
Attached file testcase
Spiffy.  Works like a charm with the above added testcase.

-MacOS91 2001-05-21-15-trunk MOZILLA
-Win98SE 2001-05-22-06-trunk
-LinRH62 2001-05-22-05-trunk
You need to log in before you can comment on or make changes to this bug.