305565 - <object> can be used to determine whether some types of local files exist

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050822
Firefox/1.0+

1. Create a file named f2.rtf on your desktop.

2. Load the following HTML file from an http(s) URL:
<p>f1.rtf - <object data="file:///Users/admin/Desktop/f1.rtf">f1</object></p>
<p>f2.rtf - <object data="file:///Users/admin/Desktop/f2.rtf">f2</object></p>
<p>f3.rtf - <object data="file:///Users/admin/Desktop/f3.rtf">f3</object></p>

Result: "Click here to download plugin" appears only for f2.rtf; the rest show
alternate content.  This demonstrates that a web page can determine whether the
file exists.  (It also means some types of DoS might be possible on some OSes.)

Note: there are some filetypes for which this does not work (.txt and .gif are two).

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

so er, is there a checkLoadURI call missing somewhere?

Comment 2

[Jesse Ruderman]

Yes, or in the wrong place.  CheckLoadURI seems to be called if the extension is
.txt or .gif, regardless of whether the file exists.

Comment 3

[Boris Zbarsky [:bzbarsky]]

nsObjectFrame never calls CheckLoadURI.  If the plugin code never does either
(and I bet it doesn't), then yes.

Comment 4

[Boris Zbarsky [:bzbarsky]]

For known image or document types the call happens in nsImageLoadingContent and
nsFrameLoader respectively.

On trunk we should just fix this with biesi's changes, but on branch we need
more work.  :(

Comment 5

[Asa Dotzler [:asa]]

Biesi, can you own this?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcase (uses /tmp/f[1-3].rtf)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, the calling code in nsObjectFrame already does a content policy check... I
should probably remove one, probably the one in ObjectFrame, any opinions?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> hmm, the calling code in nsObjectFrame already does a content policy check...

Should we just put the CheckLoadURI() call up there too?  Or are there other
ways to get into the plugin code than through nsObjectFrame?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

no, the only caller of that is in the objectframe. pluginhost would be better in
the maybe unlikely case that another caller is ever added...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm...  OK, fair enough.  Let's just put it all in pluginhost.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

removes objectframe's contentpolicy code.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194203 [details] [diff] [review]
patch v2

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp
>+    nsresult rv =
>+      NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_OBJECT,
>+                                aURL,
>+                                doc->GetDocumentURI(),
>+                                doc, // XXX this is not good

Indeed.  If we can't get to the content node from here, let's leave the content
policy check in nsObjectFrame for now (but move the security check in here)
file a bug to move the code down here once we can get to the node off the
owner?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194203 [details] [diff] [review]
patch v2

turns out we can get the DOM element, I just didn't notice the function

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194525 [details] [diff] [review]
patch v3

I guess we can't get to the SHOW_ALT error code from here, eh?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, actually, I could get to it... (I wrote the pluginhost changes w/o being
aware of the objectframe conpol check). the question is, would it work? I
suppose it should.

ok, I'll include nsContentErrors.h and return that error code.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 194525 [details] [diff] [review]
patch v3

r=jst

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194525 [details] [diff] [review]
patch v3

I think we want this security fix on the branch... it's quite safe.

Comment 20

[Mike Schroepfer]

Comment on attachment 194525 [details] [diff] [review]
patch v3

Per bug traige meeting please land immediately on branch.

Comment 21

[Brendan Eich [:brendan]]

Attachment: branch patch attempt (about to test on my windows box)

Comment 22

[Brendan Eich [:brendan]]

Fixed on trunk and 1.8 branch.

/be

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: additional patch per comment 16 / comment 17

thanks for landing this for me.

I think we want this additional patch at least on trunk, not sure about branch.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in modules/plugin/base/src/nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <-- 
nsPluginHostImpl.cpp
new revision: 1.534; previous revision: 1.533
done

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 195135 [details] [diff] [review]
additional patch per comment 16 / comment 17

this additional patch improves the behaviour of <object>/<embed> when a content
policy implementation blocks the load; it's low risk.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

additional patch checked in to MOZILLA_1_8_BRANCH for 1.8b5.

Checking in modules/plugin/base/src/nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <-- 
nsPluginHostImpl.cpp
new revision: 1.532.2.3; previous revision: 1.532.2.2
done

Comment 27

[Bob Clary [:bc] (inactive)]

fyi, fails ff 1.0.8/winxp/20060221

Comment 28

[georgi - hopefully not receiving bugspam]

(In reply to comment #1)
> so er, is there a checkLoadURI call missing somewhere?
> 

can it be done checkLoadURI to be put in a place where it can't be missed?

what is called and checkLoadURI is missed?

Comment 29

[georgi - hopefully not receiving bugspam]

similar stuff in Bug 328454

Comment 30

[Boris Zbarsky [:bzbarsky]]

> can it be done checkLoadURI to be put in a place where it can't be missed?

Not easily, unfortunately.  :(  I'd thought about putting it in newChannel or something (a new api that takes the principal too), but that would not really work because some of the newChannel callers do not in fact have the right principal around...  We might be able to fix that, but we'd need a lot of API changes to get it right.  :(