do not allow http: documents to load images from file: via <IMG> (call checkloaduri for security)

RESOLVED FIXED in mozilla1.8alpha5

Status

()

Core
Security
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: Wolfgang Schwarz, Assigned: jst)

Tracking

({fixed-aviary1.0, fixed1.7.5, hang})

Trunk
mozilla1.8alpha5
fixed-aviary1.0, fixed1.7.5, hang
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(11 attachments)

(Reporter)

Description

17 years ago
It is easy for a webpage to determine whether an image was properly loaded or 
not. By including file://... images you can thus find out whether certain 
(image) files exist on the client's computer:

<img src="file:///c|/nonexistent/content.gif">
<img src="file:///c|/windows/content.gif">

<script>
 onload =function(){
   incl=(document.images[0].width!=document.images[1].width)? "" :"not ";
   alert("Windows is "+ incl +"installed in C:/WINDOWS/");
 }
</script>

NB: All versions of IE and NN are also affected by this problem.
(Reporter)

Comment 1

17 years ago
Created attachment 25443 [details]
testcase
I've seen variations of this before, and I know that image loading is one of the
places where we're not calling CheckLoadURI to enforce this restriction. I'll
see if I can get this written into the new ImageLib.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Reassigning to Pavlov. Pavlov, do you know where this security check should go?
We need to get the security manager service and call CheckLoadURI with the image
URI and the current document URI.
Assignee: mstoltz → pavlov
Status: ASSIGNED → NEW

Updated

17 years ago
Blocks: 55237

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1

Comment 4

16 years ago
Created attachment 33048 [details] [diff] [review]
add the security check.. doesn't quite work..

Comment 5

16 years ago
my call to CheckLoadURI seems to always return FALSE... whats up with that? :)

Comment 6

16 years ago
oh, right.  this is xpcom, it uses nsresults, not prbools.. heh...

Comment 7

16 years ago
ok, so i do:

  nsCOMPtr<nsIScriptSecurityManager> securityManager(do_GetService
(NS_SCRIPTSECURITYMANAGER_CONTRACTID));

  if (securityManager) {
    nsCOMPtr<nsIURI> baseURI;
    GetBaseURI(getter_AddRefs(baseURI));

    nsresult proceed = securityManager->CheckLoadURI(baseURI, aURI, 
nsIScriptSecurityManager::STANDARD);
    if (NS_FAILED(proceed))
      return PR_FALSE;
  }

i've posted a big patch to bug 78015 which has this and a bunch of other 
changes to the same file.  can you r= this part of the patch?
OS: Windows 98 → All
Priority: -- → P1
Hardware: PC → All

Updated

16 years ago
Keywords: patch
r=mstoltz.

Comment 9

16 years ago
I can't check this in until 78830 is fixed, otherwise some images won't load 
when they should.
Depends on: 78830

Comment 10

16 years ago
pav, somehow I doubt it's 78830 you mean, since that has to do with downloading
messages for offline use.

Updated

16 years ago
Depends on: 78831
No longer depends on: 78830

Comment 11

16 years ago
The code is checked in, #if 0'd out.  I need 78831 fixed before I can turn this 
code on.

Updated

16 years ago
Whiteboard: [fix in hand]

Updated

16 years ago
Priority: P1 → P5
I turned this on, but I think I'm going to have to back it out again, as it's
causing another regression in Composer. To fix that case would mean allowing
about:blank pages to link to file: content, and I need to verify that this is
safe - I think it may open an exploit. In the meantime, I'm going to have to
#ifdef 0 this check again. Reassigning to me.
Assignee: pavlov → mstoltz
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Status: NEW → ASSIGNED
Whiteboard: [fix in hand]
Over to Jesse. See if you can fix the regression (78831) and see if any other
regressions arise.
Assignee: mstoltz → jesse
Status: ASSIGNED → NEW
Priority: P5 → P1
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

16 years ago
Status: NEW → ASSIGNED

Comment 14

16 years ago
Here's what I plan to do:

1. Allow all pages to load images (but nothing else) from chrome:// URLs.  This
is necessary because smilies (shown in message compose, mail messages, and
instant messages) and several editor images come from chrome.  This will allow a
web page to find out what theme you're using, but they can do that anyway by
sending you a XUL document.

2. For now, continue to allow composer and message compose to load images from
file:// URLs.  Since scripts can't run in composer, I don't think this lets an
attacker find out whether you have the image on your hard drive or what
dimensions the image has.

3. Later, make it so composer and message compose can only load from file if (a)
the original document being edited or replied to was allowed to load from file
or (b) the user added the image using Insert->Image.  Also try to come up with a
sane policy about composer loading images via http.
*** Bug 91316 has been marked as a duplicate of this bug. ***

Comment 16

16 years ago
It's great that this bug already exists, but the marked duplicate describes a
remote denial of service against the Mozilla user, while this bug doesn't
mention it.  So there, I mentioned it :)
The remote attack was implied but not directly stated here. Often we try to be
vague about the potential consequences of a security-related bug so that we can
use Bugzilla to discuss a fix for the bug without advertising attacks on the
browser.

Comment 18

16 years ago
*** Bug 91402 has been marked as a duplicate of this bug. ***

Comment 19

16 years ago
Changing the severity to critical as it involves a Unix/Linux DoS vulnerability
(see duplicate bugs 91316 and 91402).
Severity: normal → critical

Updated

16 years ago
Keywords: patch
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 20

16 years ago
changed summary to specificy problem clearly.
Summary: do not allow http: documents to load images from file: → do not allow http: documents to load images from file: via <IMG>

Comment 21

16 years ago
Created attachment 46183 [details] [diff] [review]
partial fix

Comment 22

16 years ago
The patch above doesn't handle background images, so it plugs the privacy hole 
but doesn't prevent the DoS attack.

Comment 23

16 years ago
Here are the changes in the patch:

docshell/base/nsIDocShell.idl
 Added an appType for composer docshells.

editor/base/nsEditorShell.cpp
 Added a setAppType call to tell the docshell it's a composer.  (Similar to code
in mailnews/base/src/nsMsgWindow.cpp.)

layout/html/base/src/nsImageFrame.cpp
 Made nsImageFrame::CanLoadImage call CheckLoadURI when the docshell's apptype
is not composer.  This involved rearranging some code in the CanLoadImage function.

caps/src/nsScriptSecurityManager.cpp
 Made it so chrome can link to file urls.  I did this to avoid breaking the
"insert image" dialog in composer.  This change might cause page info to be
vulnerable to the DoS attack.


Alt text isn't displayed for (inline) images not loaded due to the security
check added above.  I'm pretty sure that's bug 88722.

I looked into making appType inherited between docshells.  I'm hesitant to do
this because a docshell doesn't know its parent docshell when it's created (why
not?) and because it looks like the parent can change (although it usually doesn't).

Pavlov doesn't want imglib to depend on security, so he suggested adding similar
checks to bullet frames (similar to nsImageFrame), the code that loads
background images (not similar to nsImageFrame), and any other code that loads
images.  The check might be factored out into the security manager (or moved
into the content policy check?) so that these components don't all need to
depend on docshell directly.

Comment 24

16 years ago
From bug 96485: another type of image load is the javascript function 
new Image(), and it seems to go through a different code path than <img> since 
referer isn't sent.  (The other types of image loads I know about are <img>, 
css bullets, and background images.)

Comment 25

16 years ago
Over to Mitch.
Assignee: jruderman → mstoltz
Status: ASSIGNED → NEW
0.9.5
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.4 → mozilla0.9.5
time marches on...retargeting to 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
performance, footprint, feature work, and re-architecture bugs will be addressed
in 0.9.8
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Moz 1.1
Target Milestone: mozilla0.9.8 → mozilla1.1

Comment 30

16 years ago
How is that *security/critical* bug is not going to be fixed in Mozilla 1.0 ?

Comment 31

16 years ago
This CSS construction is also not subject to checkloaduri:
-moz-binding: url(test2.xml#divs)

Comment 32

16 years ago
Nominating a little bit sooner ;-) This is really serious for our embedded
solution. It can be workarounded but it's not so easy.
Keywords: mozilla1.0, topembed

Comment 33

16 years ago
minusing to topembed- as per edt triage since this is not a blocker.
Keywords: topembed → topembed-

Comment 34

16 years ago
As one of the original bug reporter of the related
bug (91316, etc.) I would like to
see this bug resolved (ESPECIALLY due to the
DoS attack like problem caused by opening a local
device file by visiting a remote HTML page).

Even Bill Gates finally realized the importance of
security over additional features of software product.
(Or so it seems.)

Why can't we?

Updated

15 years ago
Keywords: nsbeta1

Comment 35

15 years ago
Comment on attachment 46183 [details] [diff] [review]
partial fix

setting patch flag
Attachment #46183 - Attachment is patch: true
Attachment #46183 - Attachment mime type: text/html → text/plain

Updated

15 years ago
Keywords: mozilla1.0
Futuring.
Target Milestone: mozilla1.1alpha → Future
Severity: critical → normal
Keywords: nsbeta1 → nsbeta1-
warning : fixing this bug could make it impossible to put images into a new
edited document with Composer...

Comment 38

15 years ago
Regarding the Composer concern,

(i) Detecting existence of an (image) file on one's computer.

    Finding out whether an (image) file exists
    may not work in composer where JavaScript is disabled.
   (I am not sure about this, but it seems a resonable
    guess. Does composer enable JavaScript while we edit???) 

   If the detection scheme using JavaScript doesn't work
   in composer,
   then we can forget about the probem mentioned (in this
   bug per se) at least
   in the case of composer usage.

(ii) Local file access issue. (problems mentioned in
     duplicates.)
   
   Accessing device files and other non-ordinary files
   still can cause DoS attack-like scenario in
   the case of composer, too.

   In the case of composer use,
   I think it is more or less enough to check whether the
   to-be-loaded image files are plain files (or device files
   and others that can cause problems) BEFORE
   loading. If they are unsafe, don't load them.

The above might be enough for handling composer concern.

Just my two cents worth.

PS: I am afraid that obfusication of problems mentioned
in duplicates have made the real concern hidden and
the patch may be going in a slightly off-direction...
I think the local DOS still exists as of 1.x (on linux).
I tested this about a month ago.


Comment 39

15 years ago
Composer doesn't allow JS to run while editing.
If user is editing a local file or an unsaved new page, they definitely need to 
be able to insert "file:" images.
The scenario that needs to be tested is after publishing or when they 
edit a remote URL, e.g., a page on their web site. Users may wan't to insert
local images from their computer and then use publishing to upload those images
to their web site.

Comment 40

15 years ago
Corfirming topembed- [T2] per EDT triage.
Whiteboard: [T2]
So.. let me get this straight.  We are leaving a security hole open in the
broser so that various add-ons that were coded with bad assumptions won't break?
 Is that correct?

Composer is already running into a slew of problems with CheckLoadURI (eg they
can't add stylesheets to a newly edited document because of this exact problem).
 So the fix there would be to fix composer to use a resource:// uri for blank
documents or an about:something which has chrome privelegs, unlike about:blank
(see nsAboutRedirector -- setting about:newComposerDocument to redirect to
about:blank and setting its chrome privs to PR_TRUE should be pretty
straightforward).  Since JS is disabled, that should be safe (though we need to
be _really_ careful about Midas here.... but that's not using about:blank anyway).

Mail composition could use about:mailCompose instead of about:blank, I would
think.  Again, same deal as above.

This leaves smilies in mailreader and instant messenger.  For those, I am
somewhat at a loss for good solutions.... We could _white_list_ the smily urls
somewhere (eg a properties file, to make them easily changeable without
recompiling).  And only allow unpriveleged access to urls on that whitelist. 
Not the cleanest solution, but it works, I think.

Thoughts?  This is not an idle question; I'm rewriting image loading a bit, and
I would dearly like to re-enable this security check.
Keywords: privacy
Blocks: 134986

Comment 42

15 years ago
bz--The Composer bug for not using about:blank is bug 166166
Also, I'm wary for Midas; would prefer non-about scheme.  Regardless, that
discussion should be in bug 166166...
Depends on: 166166
Boris: changing Composer to use something other than about:blank sounds great to
me. I am going to make some changes to CheckLoadURI very shortly to make it less
restrictive in cases where it's not needed, but applying it to IMG tags is
probably still a good idea, and if you can fix the Composer regression, that
would be really great.

*** This bug has been marked as a duplicate of 7266 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE

Comment 45

14 years ago
heikki, I don't see how bug 7266 would be a dup. That bug talks about <input>,
which this one talks about simple <img>s.
*** Bug 212064 has been marked as a duplicate of this bug. ***
(not a dup)
Status: RESOLVED → REOPENED
QA Contact: ckritzer → ian
Resolution: DUPLICATE → ---
*** Bug 220356 has been marked as a duplicate of this bug. ***
Blocks: 7266
Blocks: 206459

Comment 49

14 years ago
*** Bug 232627 has been marked as a duplicate of this bug. ***

Updated

13 years ago
No longer blocks: 206459

Comment 50

13 years ago
*** Bug 206459 has been marked as a duplicate of this bug. ***

Comment 51

13 years ago
Why does bug this depend on bug 166166 instead of the other way around?

Comment 52

13 years ago
Does this
*** Bug 251579 has been marked as a duplicate of this bug. ***

Comment 54

13 years ago
Removed incorrect keywords, added correct ones (hang is caused by the ability of
documents to reference /dev/tty on unix systems), set target milestone a finite
distance in the future, and reassigned to default, hopefully to someone who
actually reads bug mail.  Next week I may reassign to self.
Assignee: security-bugs → dveditz
Status: REOPENED → NEW
Keywords: nsbeta1-, privacy, topembed- → hang
QA Contact: ian
Target Milestone: Future → mozilla1.9alpha

Comment 55

13 years ago
This bug has the "privacy" keyword because it allows web sites to detect local
image files and their dimensions.  It might allow detection of other local files.

Don't set the target milestone for bugs you don't own.  Also, reset priority
when you reassign.
Keywords: privacy
Priority: P1 → --
Target Milestone: mozilla1.9alpha → ---

Comment 56

13 years ago
Ok, it's mine now, then.  Removing the privacy keyword.

"privacy: Bugs relating to user privacy which do not belong in the Security:
General component."

http://bugzilla.mozilla.org/describekeywords.cgi

This is the Security: General component, so ...
Assignee: dveditz → jwbaker
Target Milestone: --- → mozilla1.9alpha

Updated

13 years ago
Status: NEW → ASSIGNED
Keywords: privacy
Summary: do not allow http: documents to load images from file: via <IMG> → do not allow http: documents to load images from file: via <IMG> (call checkloaduri for security)
It seems that
http://web.archive.org/web/19970428025719/http://www.ee.washington.edu/computing/iebug/
could be addressed by fixing that bug. this sounds like a rather bad implication
of lack of checkloaduri on <img>.
Okeee.  Given that this vulnerability actually allows sites to do useful things
like steal passwords, I feel that we should address it ASAP.  In particular, I
propose that we set a hard deadline of FireFox 1.0 and Mozilla 1.8 to address
this issue.  Shipping either with a known security hole like this would be
unacceptable in my mind.  (Ccing some drivers on the off chance they agree or
something).

There are several possibilities for "fixing" this bug:

1)  Fix bug 166166 (then this bug becomes a one-liner).
2)  Add a "composer" docshell type as done in one of those patches and hack
    nsImageLoadingContent as nsImageFrame is hacked there.
3)  Reimplement CheckLoadURI in nsImageLoadingContent for the express purpose of
    dealing with images.

#3 is not so great because it doesn't guard against a site loading an
about:blank document and then tossing images in there via the DOM...

#2 is a nasty hack, but I'd be willing to deal with it on a _temporary_ basis. 
Given the three-year history of this bug, I have grave doubts it would stay
temporary.

#1 involves someone who actually knows something about editor dealing.  Do we
actually have such people around?

Frankly, if push comes to shove, I would be in favor of closing the security
hole and leaving a small bit of editor functionality (never-yet-saved documents
linking to file:// images) broken temporarily until bug 166166 is fixed.  Even
more frankly, it is my opinion that unless we do that bug 166166 will never in
fact be fixed.

Comment 59

13 years ago
I'll probably get flamed for this by the other editor folks here (please tell me
why I'm wrong) but for the editing I do, it would be a relief if local file:
images inserted while I was editing about:blank failed to insert.  I can't count
the times when I've ended up with a file full of file:/// images because I
forgot to "save as" before inserting images, and only found out about it when
someone mailed me about all my broken images.

UI would be desirable to enforce that, and I don't see any UI that wouldn't
irritate users.  You could grey out the image button and let them wonder why
it's disabled (yuck), or have it pop up a dialog saying "Please save your file
first before inserting any images" -- neither option is good, though they're
arguably less bad than what we do now, giving the user the appearance that
there's an image there which actually won't work after the file is saved.

Boris: re #1, isn't the issue more on adding a new composer:blank uri (or
whatever, including deciding what it should be called), rather than making the
handful of s/about:blank/about:composer in the editor source?
Yeah, the issue with #1 is adding the new about: uri (or whatever we call it)
and teaching the security manager about it.  If there is some buy-in from editor
people, that can be done.

Biesi made another suggestion.  When a new document is first created, composer
could create a temp file (in $TEMP) for it.  Then use that as the document URI.
 Once the user saves, or the window is closed, whichever happens first, kill the
temp file...  Neither he nor I know enough about editor to decide whether that
would break anything else.

Comment 61

13 years ago
Hi kids.  Remember, I may be the "owner" but there's a little button that you
can use to change that if you have a patch :)

I'd like to point out that this bug is harder than <img> because we lack
checkLoadURI calls from other places (including CSS backgrounds).  It would be
folly to fix <img> without fixing the other ones.

Does anyone else endorse the plan of making this bug block 1.8 and firefox 1.0?
 Mark it and change the target milestone if so.

Comment 62

13 years ago
I have mixed feelings on this bug.  Also, I probably need to re-read much of
this bug.

I thought this bug was about inserting file urls into http or ftp urls as much
as the about:blank issue.

I do agree with Akkana that there are some usability issues to address in the
editor.  I haven't thought about it much lately but I'd probably approach it by
thinking about the publishing issue before the about:blank issue since I think
that the publishing solution is likely to adequately cover the about:blank issue
but not necessarily vice versa.

Is it possible for the editor (being chrome) to load the images from a cache or
similar or am I proposing one of bz's proposals?
> I'd like to point out that this bug is harder than <img> because we lack
> checkLoadURI calls from other places (including CSS backgrounds). 

Wrong, kiddo.  There's been a check on CSS backgrounds since bug 57607 got
fixed.  There are similar checks on list-style-image (as of bug 188955), and so
forth.

I'm fairly certain that <img> is about the only place we don't do a CheckLoadURI
check at this point; a lot of the other codepaths have been consolidated such
that they all run through the nsContentUtils::CanLoadImage code, which certainly
calls CheckLoadURI.

brade, I really don't see how we can allow general access to file:// urls from
ftp:// or http:// without being vulnerable to the exploit described in comment 57...

Special behavior based on us being "the editor" (and making image nodes know
that editor exists) is pretty much my option #2.  Like I said, it'd work
short-term, but bug 166166 points out that a lot of things other than images
would still continue not to work...
Actually, I was wrong.  I _did_ have a security check in the CanLoadImage code,
but had to remove it because <img> uses that code...
Blocks: 254365
Whiteboard: [T2] → [sg:fix]
*** Bug 257370 has been marked as a duplicate of this bug. ***

Comment 66

13 years ago
Shouldn't this be blocking Firefox 1.0?
This sounds like a Security-risk bug and has even been mentioned on slashdot
(today), not something we would want for the 1.0 release.
Nominating for blocking.
Flags: blocking-aviary1.0?

Comment 67

13 years ago
Agree with blocking 1.0, but not my decision.  Adding helpwanted: I just don't
have the time.
Keywords: helpwanted

Comment 68

13 years ago
who can help?  jst?
Assignee: jwbaker → jst
Status: ASSIGNED → NEW
Flags: blocking-aviary1.0? → blocking-aviary1.0+

Comment 69

13 years ago
Jst cannot according to this posting today:
http://developers.slashdot.org/comments.pl?sid=124646&cid=10452959

Isn't this a security issue as it can cause a DOS?
Should it be nominated for review as a security risk?

Comment 70

13 years ago
Created attachment 161357 [details]
Testcase with Background Image

Comment 71

13 years ago
Jed: that slashdot comment was made by Jeffrey Baker, who the bug was assigned
to at the time (see comment 67). chofmann reassigned it to jst after that
(comment 68). As for nominating for security review - many of the comments here
already are from members of the security group, so they're obviously aware of it.
*** Bug 263938 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 73

13 years ago
Created attachment 161994 [details] [diff] [review]
Prevent loading of file: URI's from webpages, yet permit it from (mail) compose
Comment on attachment 161994 [details] [diff] [review]
Prevent loading of file: URI's from webpages, yet permit it from (mail) compose

>Index: content/base/src/nsContentUtils.cpp

>+  if (appType != nsIDocShell::APP_TYPE_MAIL &&

Please file a followup bug on this.  We should absolutely do a security check
for the message pane display area.  The compose window should just use
APP_TYPE_EDITOR and people checking for APP_TYPE_MAIL now to detect message
compose should be fixed (I don't expect there to be many, if any).

>+      CheckLoadURI(docURI, aURI, nsIScriptSecurityManager::ALLOW_CHROME);

On trunk, please use CheckLoadURIWithPrincipal.  It's faster and more correct.

>Index: layout/xul/base/src/nsImageBoxFrame.cpp
>Index: layout/xul/base/src/nsImageBoxFrame.h

Note that these changes have already been made on trunk, except the
GetLoadGroup() removal.  That's a good catch.

r+sr=bzbarsky with that.
Attachment #161994 - Flags: superreview+
Attachment #161994 - Flags: review+
(Assignee)

Comment 75

13 years ago
Created attachment 162004 [details] [diff] [review]
Do security checks for mail apps too

I realized that since we now set msg compose windows to be editor apps, we
*can* do a security check for mail apps. This patch does that. bz, I wasn't
able to eliminate the container nsCOMPtr after all, msg compose loads images in
windows w/o a script global object (in one of its odd cached windows, I guess).
Comment on attachment 161994 [details] [diff] [review]
Prevent loading of file: URI's from webpages, yet permit it from (mail) compose

content/xbl/src/nsXBLResourceLoader.cpp

     if (NS_FAILED(NS_NewURI(getter_AddRefs(url), curr->mSrc, nsnull, docURL)))

you didn't touch this, but do you feel like passing
doc->GetDocumentCharacterSet().get() as the charset here?
Comment on attachment 162004 [details] [diff] [review]
Do security checks for mail apps too

OK, makes sense.  This is much happier.  r+sr=bzbarsky modulo the trunk nits. 
Please do land this on trunk; I want to use builds that have this, dammit.  ;)
Attachment #162004 - Flags: superreview+
Attachment #162004 - Flags: review+

Comment 78

13 years ago
Comment on attachment 162004 [details] [diff] [review]
Do security checks for mail apps too

In nsMsgCompose.cpp, there is an XXX comment for bug 206793.  Is that still a
bug or can the comment be removed or is a separate (but related) issue?
(Assignee)

Comment 79

13 years ago
That comment was added by mscott (from what I could tell) when he landed the fix
for bug 206793 (which isn't reflected in the bug).
(Assignee)

Comment 80

13 years ago
Comment on attachment 162004 [details] [diff] [review]
Do security checks for mail apps too

Requesting approval.
Attachment #162004 - Flags: approval1.7.x?
Attachment #162004 - Flags: approval-aviary?

Comment 81

13 years ago
Comment on attachment 162004 [details] [diff] [review]
Do security checks for mail apps too

a=asa for aviary checkin and 1.7 branch checkin.
Attachment #162004 - Flags: approval1.7.x?
Attachment #162004 - Flags: approval1.7.x+
Attachment #162004 - Flags: approval-aviary?
Attachment #162004 - Flags: approval-aviary+
(Assignee)

Comment 82

13 years ago
Created attachment 162025 [details] [diff] [review]
Same thing for the trunk.
+      nsContentUtils::LoadImage(url, doc, nsnull, nsIRequest::LOAD_BACKGROUND,
+                                getter_AddRefs(req));

doesn't this have an additional referrer argument on trunk?
(Assignee)

Comment 84

13 years ago
Yeah, just realized that. Fix checked in (along with bz's suggestion to use the
documents character set when creating uris).
that was my suggestion ;)

+ nsContentUtils::LoadImage(url, doc, doc->GetDocumentURI(), nsnull,

you could use docURL, I think.

also, the comment above:
// Passing NULL for pretty much everything -- cause we don't care!
// XXX: initialDocumentURI is NULL!

doesn't seem too relevant - the only NULL is the observer; and there is no
initialDocumentURI arg.
You also need to fix nsTreeBodyFrame.cpp on the branch (but not on the trunk). 
And you may find some other relevant branch/trunk differences by looking through
the patches in bug 236889, bug 253627 (which you may want to use, at least
partly, for the branch -- but watch for regression bug 256172), bug 249809, bug
249168, and bug 249171.

(Also note that for things from stylesheets, the trunk is doing
stylesheet-relative checks and the branch is doing document-relative checks, but
that should be OK.)
(Assignee)

Comment 87

13 years ago
(In reply to comment #85)
> that was my suggestion ;)

Sorry, my bad.

> 
> + nsContentUtils::LoadImage(url, doc, doc->GetDocumentURI(), nsnull,
> 
> you could use docURL, I think.

Fixed. I left the comment for someone else to clean up...
(Assignee)

Comment 88

13 years ago
Created attachment 162036 [details] [diff] [review]
Final branch patch for checkin.

This fixes nsTreeBodyFrame and nsBulletFrame, and fixes the regression that
this would've caused (bug 256172).
(Assignee)

Comment 89

13 years ago
Created attachment 162040 [details] [diff] [review]
1.7 branch diff

I had to change what I did in editor.js in the earlier patches, had to find the
root docshell and set the app type on that, not in the editors immediate
docshell. Made the same change on trunk and aviary.
(Assignee)

Comment 90

13 years ago
Fixed on trunk and branches.
Status: NEW → RESOLVED
Last Resolved: 14 years ago13 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED
*** Bug 7266 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 92

13 years ago
Created attachment 162161 [details] [diff] [review]
Use CheckLoadURIWithPrincipal() and clean up some string fu in nsScriptSecurityManager
(Assignee)

Updated

13 years ago
Attachment #162161 - Flags: superreview?(bzbarsky)
Attachment #162161 - Flags: review?(bzbarsky)
Comment on attachment 162161 [details] [diff] [review]
Use CheckLoadURIWithPrincipal() and clean up some string fu in nsScriptSecurityManager

>Index: caps/include/nsScriptSecurityManager.h

>+    GetBaseURIScheme(nsIURI* aURI, nsAFlatCString& aScheme);

nsAFlatCString is effectively deprecated... it's just a typedef for nsCString
now.  Just use that.

>Index: caps/src/nsScriptSecurityManager.cpp

>     static const char aboutScheme[] = "about";
>-    if(nsCRT::strcasecmp(scheme.get(), aboutScheme) == 0)
>+    if(nsCRT::strcasecmp(aScheme.get(), aboutScheme) == 0)

How about removing kAboutScheme and just doing:

  if(aScheme.LowerCaseEqualsLiteral("about")) 

?

>         if(NS_FAILED(uri->GetAsciiSpec(spec)))

Use GetPath() instead?	Then you don't need to do the weird addition of
sizeof(aboutScheme).

>+        (nsCRT::strcasecmp(sourceScheme.get(), "mailbox")  == 0 ||
>+         nsCRT::strcasecmp(sourceScheme.get(), "imap")     == 0 ||
>+         nsCRT::strcasecmp(sourceScheme.get(), "news")     == 0))

Again, use LowerCaseEqualsLiteral.

>+    if (nsCRT::strcasecmp(targetScheme.get(), sourceScheme.get()) == 0)

I suspect |targetScheme.EqualsASCII(sourceScheme.get(), sourceScheme.Length())|
may be faster, but check with darin?

>-        if (nsCRT::strcasecmp(targetScheme, protocolList[i].name) == 0)
>+        if (nsCRT::strcasecmp(targetScheme.get(), protocolList[i].name) 

EqualsASCII (you can probably get the length via NS_ARRAY_LENGTH too, depending
on what the type of .name is).

r+sr=bzbarsky with that.
Attachment #162161 - Flags: superreview?(bzbarsky)
Attachment #162161 - Flags: superreview+
Attachment #162161 - Flags: review?(bzbarsky)
Attachment #162161 - Flags: review+
(Assignee)

Comment 94

13 years ago
(In reply to comment #93)
> >+    if (nsCRT::strcasecmp(targetScheme.get(), sourceScheme.get()) == 0)
> 
> I suspect |targetScheme.EqualsASCII(sourceScheme.get(), sourceScheme.Length())|
> may be faster, but check with darin?

But that isn't case insensitive, or did we decide that that's ok?

> >-        if (nsCRT::strcasecmp(targetScheme, protocolList[i].name) == 0)
> >+        if (nsCRT::strcasecmp(targetScheme.get(), protocolList[i].name) 
> 
> EqualsASCII (you can probably get the length via NS_ARRAY_LENGTH too, depending
> on what the type of .name is).

Same thing, not case insensitive.
there is LowerCaseEqualsASCII
Ah, true.  Could do .Equals(whatever, CaseInsensitiveCStringComparator()), but I
really don't know how that compares perf-wise to strcasecmp....

Ccing darin in case he knows.
(Assignee)

Comment 97

13 years ago
Created attachment 162213 [details] [diff] [review]
Updated version of the above attachment.
(Assignee)

Comment 98

13 years ago
Last attachment checked in.

Comment 99

13 years ago
> .Equals(whatever, CaseInsensitiveCStringComparator())

hard to say if that is better or worse than PL_strcasecmp.  Both calls equate to
a DSO call, but Equals must instantiate the comparator, which just equate to a
pointer assignment for the vtable member.  Equals will always test the string
length before comparing chars, but when it does compare chars, it will invoke a
virtual function to ask the comparator to compare two runs of text.  since
strings are always single fragment now, there is only one call into the
comparator.  so... i don't know which is faster.

Comment 100

13 years ago
Are the branches going to get the new version of this patch?
The new version is just changes from the branch version to make use of the
better APIs available on trunk.  So no.
*** Bug 265243 has been marked as a duplicate of this bug. ***
I think the fix here caused the regression in Bug 267248.

Comment 104

13 years ago
Um, does this bugfix break my website? If you go here:
http://www.devimg.net/Post.php
and scroll down to the Images section, typing in the path to an image attempts
to show a thumbnail of the image and report its size. This works in Firefox 1.0
Preview and Internet Explorer. I think it's a nice feature to show you what
image you've entered into the file dialog. Does this no longer work on the new
1.0? That would be annoying.
(In reply to comment #104)
> Does this no longer work on the new 1.0?

That's correct.

> That would be annoying.

That's too bad, but it's even more annoying for users when sites can read random
things off their hard drives.

Note that your site doesn't work in Opera, Konqueror, or Netscape 4 either.  If
you really think we should emulate IE's security bugs, feel free to mail me
privately and I'll try to convince you otherwise.  ;)

Comment 106

13 years ago
It's not a security vulnerability if the image path is identical to the path of
any of the file inputs, since file inputs only accept user input and are already
secure. So if someone types in C:\Images\Game.jpg into a file input, meaning
they are going to upload that file to the server, it is obviously perfectly fine
for the javascript to display that image in an <img> tag.

I know this is not important for most people, but I would really like this to be
allowed... local images should be allowed if the path matches any of the file
inputs on the page.

Comment 107

13 years ago
If someone uploads an image to the server, serve it from the server.

Comment 108

13 years ago
(In reply to comment #106)
To argue my point further, the entire point of this bug report was that
javascript can be used to verify whether or not an image exists. For files to be
uploaded, isn't that a good thing? Would you prefer to submit a post and find
out you entered an invalid image, and have to re-type your entire post, or would
you want to have a pop-up tell you your path is invalid when you try to submit,
without redirecting the page and potentially losing the information you entered?
The browser sometimes remembers what you typed, but not always. Also, in my
case, someone might have images named Screenshot45.jpg, Screenshot36.jpg, etc.,
so showing a thumbnail of the image makes it more obvious to them which 
image they entered.

Regarding comment #107, images *are* displayed from the server, but only after a
post has been submitted. The image preview is used to provide user feedback so
they can easily see which images they're uploading in the first place, and to
make sure they didn't mistype the path. Likewise, JavaScript is used to prevent
the post from being submitted if they forget to enter a password, or the two
passwords they entered don't match.

Comment 109

13 years ago
Content loaded from HTTP cannot be allowed to interact with local files.  That's a basic rule and I'm 
afraid that yes, it does break some very rarely-used functionality.

This bug has a huge CC list and perhaps you would like to continue the discussion in a newsgroup.  Or 
you could open another bug about the problem you are having and see if it gets any traction.
*** Bug 206459 has been marked as a duplicate of this bug. ***
> since file inputs only accept user input and are already secure

A user can easily type the wrong value in a file input (or select the wrong
file), then notice it and correct it.  This is ok, because the server can't get
at the data until the file input is actually submitted, at which point the user
is confirming that they made the right selection.

Consider a user typing a filepath in a file input and the page trying to load
that file URI on every keystroke, say.  That's not something we want to allow by
any means and your proposal to allow the load if the URI matches one of the file
inputs would allow it.

Comment 112

13 years ago
Let's continue this discussion over at Bug 268793.

Comment 113

13 years ago
*** Bug 268778 has been marked as a duplicate of this bug. ***

Comment 114

13 years ago
*** Bug 206962 has been marked as a duplicate of this bug. ***

Comment 115

13 years ago
Could u at least please make it configurable in the settings dialog?

I'm 2nd chef of an onlinegame and due to high traffic and so on we give the
users the posibility to download the complete imagepack from the server and use
it, so ISDN and modem users have a faster loading and the server less traffic.
With your "buxfix" you crashed our imagepacks. I know some other games, which
are using this technology, too, and we all try to make our games compatible to
firefox, but now we have white pages, because the images aren't shown.

Thx,
Fireslam
In current builds, this is configurable on a per-site basis.  It'll be release
noted accordingly.  Note that changing that configuration opens the user up to
attacks from the site being whitelisted, so there will be no UI for it.
(In reply to comment #116)
> In current builds, this is configurable on a per-site basis.  It'll be release
> noted accordingly.

Where can I found an example of the "syntax" to use ?

Target Milestone: mozilla1.9alpha → mozilla1.8alpha5
The pref mentioned at bug 139676 comment 5 probably works. Probably not a safe
pref to change for day to day browsing.
that pref is not what bz meant though. try bug
84128 comment 194, as linked to from bug 233108
> Where can I found an example of the "syntax" to use ?

It's linked from the "Security" section of the release notes for 1.8a5 (though
it may take a few minutes for the link to say "http://" instead of "thttp://").
 Didn't I say it would be relnoted?  Shouldn't that be indication to... check
the release notes?
(In reply to comment #118)
> The pref mentioned at bug 139676 comment 5 probably works. Probably not a safe
> pref to change for day to day browsing.

|security.checkloaduri| works fine,
but it's the one that I want to stop using.

(In reply to comment #119)
> that pref is not what bz meant though. try bug
> 84128 comment 194, as linked to from bug 233108

(I had "read" this bug (and others), but I did not follow that link :-()
Thanks, that teached me what I was looking for :-)
Then, I saw that there was examples in bug 84128 comment 199 and following ;->

(In reply to comment #120)
> It's linked from the "Security" section of the release notes for 1.8a5 (though
> it may take a few minutes for the link to say "http://" instead of "thttp://").
>  Didn't I say it would be relnoted?  Shouldn't that be indication to... check
> the release notes?

Let me explain:

You did write it, I did read it,
but I guess I still did not figure out what release note you are refering to:
I usually check <http://www.mozilla.org/releases/mozilla1.8a5/, README.html>,
and all it says is "Untrusted documents can no longer load images from the
user's hard drive (bug 69070).".
This is why I posted my question here, rather than in bug 233108 (where it belongs).

Now, about |"Security" section of the release notes for 1.8a5|,
an URL would help me, as I don't know what/where this is :-(
Keywords: helpwanted
http://www.mozilla.org/releases/mozilla1.8a5/known-issues.html#psm
(In reply to comment #122)
> http://www.mozilla.org/releases/mozilla1.8a5/known-issues.html#psm

Thanks, that's it :-)

I would have checked this too, if |README.html > New Issues > General| was
saying so, instead of being empty :-<
Hello, I am a web developer worried about this bug (actually, worried about the
resolution of this bug, which has affected me with the 1.0 release of Firefox).
I understand the point of view, but I agree in some way to Ed (comment #106).
Making a user configurable option would be fine, but I think this option must
have a UI.

Just a question, is there a way to know with JavaScript code if the browser is
allowing this operation? Because if the user-agent of the visitor has this bug
fixed I want to show him a dialog (window.alert) with this information (so as to
teach him to configure Mozilla or Firefox to allow it).

Thanks.
Images that don't load should fire error events targeted at the image.  I don't
know whether we actually do that for images blocked by this patch in Firefox
1.0; I seem to recall a bug filed on that.

Past that, configuring the browser to allow a website to do this CANNOT be done
safely in Firefox 1.0 and cannot be done with reasonable safety in current trunk
builds (you need to _really_ trust the website in question).  So it's not really
appropriate to suggest such settings to users.

Comment 126

13 years ago
I understand dealing with this vulnerability was important, yet as mentioned
somewhere above the way it was fixed actually breaks some projects.

As an example, I'm working on a site where high-resolution images are required,
there is a pack of images the visitor can download, then images are accessed
through file:///install/path/images/image.png in <img> tags or in CSS background
style properties. That takes user pleasure a much higher degree, along with a
huge bandwidth gain and no loading time. Worked like a charm untill this fix.

So unless there's a GUI to allow local images to be displayed on selected sites,
the way the current fix is implemented, this project will not run on FF, which
happens to be my default browser...

Isn't there a way to fix the secrutiry hole where it is, and not where it is
not? Since the real problem is that JS can get properties of non-existant files,
isn't there a way NOT to declare those non-existent local files in the DOM tree,
thus preventing JS from attempting to do anything with it? document.images[0]
would be undefined, which it actually is.
> Since the real problem is that JS can get properties of non-existant files,

I don't know where people got this idea and why they keep repeating this
falsehood.  For example, allowing sites to load local images allows them to hang
or crash various existing operating systems (eg Win9x) with no JS involved.

The JS getting properties thing is one of the problems fixed by this change, but
not the only one, and in my opinion not even the most serious one.

Updated

13 years ago
Blocks: 248511

Comment 128

13 years ago
What sort of implications may this have for loading images from an intranet site?

Eg. a site could tell who was visitng from inside a certain company, by whether
they could load an image from the company's intranet.

etc.

Updated

12 years ago
QA Contact: benc
You need to log in before you can comment on or make changes to this bug.