call CheckLoadURI consistently (Security related)

VERIFIED FIXED in mozilla0.9

Status

()

Core
Security
P3
normal
VERIFIED FIXED
18 years ago
6 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
mozilla0.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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

Comment 1

18 years ago
Adding rtm keyword.
Status: NEW → ASSIGNED
Keywords: rtm
Target Milestone: --- → M19

Comment 2

18 years ago
Are there any user-visible consequences of this bug?  Seems like this should be
minus.
Whiteboard: [need info]
(Assignee)

Comment 3

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

18 years ago
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.

Updated

18 years ago
QA Contact: czhang → junruh
(Assignee)

Comment 5

18 years ago
Created attachment 18606 [details] [diff] [review]
Patch: adding CheckLoadURI to open-in-new-window.
(Assignee)

Comment 6

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

Updated

18 years ago
Whiteboard: [need info] → [rtm+]

Updated

18 years ago
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 sr=brendan@mozilla.org, 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

18 years ago
sr=jband
(Assignee)

Comment 9

18 years ago
Created attachment 18607 [details] [diff] [review]
final patch with Brendan's suggestions.

Comment 10

18 years ago
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: sr=brendan@mozilla.org on the final patch.  It doesn't
need r= again.  Get this in the trunk, ok?

/be

Comment 12

18 years ago
rtm++, please check into the branch ASAP.
Whiteboard: [rtm+] → [rtm++]
(Assignee)

Comment 13

18 years ago
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++]

Comment 14

18 years ago
Mass changing QA to ckritzer.
QA Contact: junruh → ckritzer
(Assignee)

Updated

18 years ago
Blocks: 29079

Comment 15

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

18 years ago
Nominationg for Mozilla0.8. I guess, some nice attacks could be constructed with
this bug.
Keywords: mozilla0.8
(Assignee)

Comment 17

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

18 years ago
modifications made in contentAreaUtils.js are causing bug 62964

Comment 19

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

18 years ago
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.

Updated

17 years ago
Depends on: 69070

Updated

17 years ago
Keywords: mozilla0.9

Updated

17 years ago
Keywords: mozilla0.8.1

Updated

17 years ago
Keywords: mozilla0.8

Comment 21

17 years ago
Mass-change: Do not remove nominations (even if Milestone passed). Readding
mozilla0.8 nomination.
Keywords: mozilla0.8
(Assignee)

Comment 22

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

Comment 23

17 years ago
Setting milestone to Moz0.9.
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 24

17 years ago
Created attachment 31050 [details] [diff] [review]
Rewrite of CheckLoadURI, more checks for context menu
(Assignee)

Comment 25

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

17 years ago
sr=hyatt
(Assignee)

Comment 27

17 years ago
Created attachment 31072 [details] [diff] [review]
Revised patch with Patrick's suggestions

Comment 28

17 years ago
r=beard (FWIW)
(Assignee)

Comment 29

17 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 30

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

17 years ago
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?
(Assignee)

Comment 32

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

17 years ago
Created attachment 35649 [details]
testcase

Comment 34

17 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.