321589 - WMP in <EMBED> with non-file:/// URI allows linking to local content (checkloaduri policy ignored)

Status: RESOLVED WORKSFORME

(Core :: Security, defect, P1)

Description

[Uriah Narmed]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

First mentioned in this mozillaZine thread: http://forums.mozillazine.org/viewtopic.php?t=359811

The Windows Media Player plugin can link to local content in an <embed> from a remote web page if the src attribute does not use the file: scheme. E.g., src="C:\WINDOWS\clock.avi" will load the AVI. src="file:///c:/WINDOWS/clock.avi" will not.

Reproducible: Always

Steps to Reproduce:
Load a remote web page containing the HTML <embed type="application/x-mplayer2" src="C:\WINDOWS\clock.avi"> under Windows XP with the Windows Media Player plugin installed.
Actual Results:  
The AVI is displayed in the browser.

Expected Results:  
The AVI should not load, and a Security Error should be displayed in the JavaScript Console.

On the page at the URL for this bug, there are four <embed>s. Each references an AVI file found in the default installation of Windows 2000 and XP. The first two use the file:// scheme and are blocked by the checkloaduri policy. One of the second two should load (depending on whether you're using 2000 or XP), as they do not use the file:// scheme.

Comment 1

[Bob Clary [:bc] (inactive)]

confirmed with winxpsp2/wmp9

Comment 2

[Tim Riley [:timr]]

WIll re-consider when Dan has a chance to look at it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So is the problem that we have no URI, so we just instantiate the plug-in... and WMP itself decides to treat the string as a filename?

As in, it seems to me like this is a bug in the plug-in.  There's no way we can control what plug-ins do, unfortunately.

Comment 4

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

> So is the problem that we have no URI

something's odd here. if we have no URI, we fallback. I suspect we treat it as relative URI though...

Comment 5

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

(compare http://lxr.mozilla.org/seamonkey/source/content/base/src/nsObjectLoadingContent.cpp#721)

Comment 6

[Boris Zbarsky [:bzbarsky]]

But the bug's on 1.8 branch.  Is this even an issue on trunk?  That's the first place to start...

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I can reproduce this on current branch and trunk.

I think this is basically the same issue as bug 334341. The URI created from a "C:\..." style patch has a scheme of "C", which in checkLoadURI allows as an unknown scheme, "risky from a security standpoint" case [1]. The URI is then "fixed up" to a file:// URI when it is loaded. In this case, I think the plugin does the "fixing", whereas in bug 334341 I think it's URIFixup. I think both cases would be fixed by changing checkLoadURI's behavior for unknown schemes, though I'm not sure what to change it to. How can we garantee that URIs passed through checkLoadURI aren't dangerous if they are later "fixed up"? I guess we can change callers to do the fixup before calling checkLoadURI, in cases where we are doing the fixup ourselves. If the plugin is doing the fixup, which I think is what is happening here, I don't think there's much we can do to solve the general issue beyond disallowing unknown schemes.

[1]: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.266.2.7.2.2#1379 

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I think both cases would be fixed by changing checkLoadURI's behavior for
> unknown schemes

That's not really doable, unfortunately.  Certainly not on stable branches.

> How can we garantee that URIs passed through checkLoadURI aren't dangerous if
> they are later "fixed up"?

We can't.  Anyone doing fixup should be doing CheckLoadURI.  So bug 334341 we should fix; this bug is pretty clearly a bug in WMP.

Note that even if we blocked unknown schemes in CheckLoadURI the plug-in could still shoot itself in the foot, so....

Comment 9

[Daniel Veditz [:dveditz]]

> [changing checkLoadURI's behavior is] not really doable, unfortunately.
> Certainly not on stable branches.

What if we only changed single character schemes? They're syntactically valid but the IANA hasn't actually assigned any (http://www.iana.org/assignments/uri-schemes.html) and IMHO isn't really likely to. So if we get a 1-char scheme we can pretty safely treat it as file: for purposes of checkLoadURI.

Should we really be fixing up windows filenames everywhere? I can buy it from the address bar and the "Open Location" dialog (places the user may type in), but web page references? But changing DocShell fixups risks much more breakage than checkLoadURI.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> but the IANA hasn't actually assigned any

But how many extensions use them, precisely because of that?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we fix this by doing our own fixup of the uri and give the plugin the fixedup uri?

Dan, do you think this should be blocking?

Comment 12

[Daniel Veditz [:dveditz]]

Attachment: fix v1

The changes in nsObjectLoadingContent.cpp are enough to fix the "C:\test\something.wmv" case. Windows, however, is perfectly happy with slashes in either direction and you can bypass this fix by using a forward slash (e.g. c:\test/something.wmv and c:/test/something.wmv).

If the path has no back-slashes at all the fixup code assumes it's a non-file. If it does have a back-slash then it gets through to nsLocalFileWin but that rejects it if there's a single forward slash. Both of these two minor tweaks could affect uses of fixupURI or nsLocalFile in other parts of the tree. It's worth worrying about a little, but I didn't see anything that went wrong.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We should probably do the security checks on both the original URI (which is what we might end up loading in some cases) and the fixed-up URI.  Better yet, we'd only do the latter when we might end up loading it...

The changes to nsLocalFileWin probably need to guard against running off the end of the input string.

Haven't had a chance to look at the substance of the fixup/localfile changes yet.

Comment 14

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

Comment on attachment 269823 [details] [diff] [review]
fix v1

In nsObjectLoadingContent, couldn't you just call CheckLoadURIStrWithPrincipal, which takes care of calling URI Fixup?

+      // (this routine seems not to report errors)

That doesn't seem like a comment that should be checked in. Error handling in this routine happens by falling back to fallback content, so you should call Fallback(PR_FALSE) here (though this is moot when changing to CheckLoadURIStrWithPrincipal)

also. you should really add a comment about why this is needed.

What's the reason for the nsDefaultURIFixup change?

Why not do those slash check in URI fixup instead of changing nsLocalFileWin?

Comment 15

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to comment #14)
> What's the reason for the nsDefaultURIFixup change?

As noted in comment 12, because windows is perfectly happy with paths like "c:/test/something.wmv" but currently URIFixup assumes that windows paths must have at least one back slash or be just a naked drive letter. Maybe we should drop the slash check and just check for the drive-letter pattern.

> Why not do those slash check in URI fixup instead of changing nsLocalFileWin?

Later on nsDefaultURIFixup further tests the filepath by calling NS_NewLocalFile() on it which will return NS_ERROR_FILE_UNRECOGNIZED_PATH if I don't make the nsLocalFileWin.cpp change. The Fixed-up URI comes from the nsIFile.

Comment 17

[Daniel Veditz [:dveditz]]

tree closing early, pushing to next release.

Comment 18

[Doug Turner (:dougt)]

regarding the xpcom/io changes:

1) what bz said - you are going to need to protect against dereferencing garbage:

    PRUnichar thirdChar  = *(++begin);
    PRUnichar fourthChar = *(++begin);


2) I am concerned about the change to accept paths that have a forward slash as the 3rd char as other code in the implementation makes assumptions that the "native path" does not contain such chars.  A quick grep through the file for \\ makes a change like this makes me worry alot about regressions.  For example, the Create() method will fail if the native path is c:/create_this_directory. 

Maybe we make convert the filepath from forward slashes to black slashes after the 3 char immediately when we enter InitWithPath()

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 269823 [details] [diff] [review]
fix v1

-'s based on my last commands and no response.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Is this still an issue? Could someone on Windows test?

Comment 21

[Al Billings [:abillings - ex-MoCo]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Is this still an issue? Could someone on Windows test?

Matt, could you see if this still reproduces?

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Cannot reproduce. Verified against nightly build 2012-10-31 on Windows 7.

I tried both forward and back slash notation as suggested by Dan, both seem OK.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Thanks!