Closed
Bug 321589
Opened 19 years ago
Closed 12 years ago
WMP in <EMBED> with non-file:/// URI allows linking to local content (checkloaduri policy ignored)
Categories
(Core :: Security, defect, P1)
Tracking
()
RESOLVED
WORKSFORME
mozilla1.9alpha8
People
(Reporter: unarmed, Assigned: dveditz)
References
()
Details
(Keywords: qawanted, sec-low, Whiteboard: [sg:low P3])
Attachments
(1 file)
(deleted),
patch
|
Biesinger
:
review-
dougt
:
superreview-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.9a1+
Flags: blocking1.8.0.1?
Whiteboard: [sg:low]
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Comment 2•19 years ago
|
||
WIll re-consider when Dan has a chance to look at it.
Flags: blocking1.7.13?
Flags: blocking1.7.13-
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8-
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Comment 3•19 years ago
|
||
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•19 years ago
|
||
> 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•19 years ago
|
||
Comment 6•19 years ago
|
||
But the bug's on 1.8 branch. Is this even an issue on trunk? That's the first place to start...
Comment 7•19 years ago
|
||
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•19 years ago
|
||
> 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....
Assignee | ||
Comment 9•19 years ago
|
||
> [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•19 years ago
|
||
> but the IANA hasn't actually assigned any
But how many extensions use them, precisely because of that?
Updated•18 years ago
|
Flags: blocking1.9?
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?
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9alpha6
Assignee | ||
Comment 12•18 years ago
|
||
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.
Attachment #269823 -
Flags: superreview?
Attachment #269823 -
Flags: review?
Assignee | ||
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.9a1+
Flags: blocking1.8.1.5?
Comment 13•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #269823 -
Flags: superreview?(dougt)
Attachment #269823 -
Flags: superreview?
Attachment #269823 -
Flags: review?(cbiesinger)
Attachment #269823 -
Flags: review?
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.1.5? → blocking1.8.1.5+
Comment 14•18 years ago
|
||
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•18 years ago
|
||
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Assignee | ||
Comment 16•18 years ago
|
||
(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.
Assignee | ||
Updated•18 years ago
|
Whiteboard: [sg:low] → [sg:low] need reviews
Assignee | ||
Comment 17•18 years ago
|
||
tree closing early, pushing to next release.
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Comment 18•17 years ago
|
||
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()
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Priority: -- → P2
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.12+
Comment 19•17 years ago
|
||
Comment on attachment 269823 [details] [diff] [review]
fix v1
-'s based on my last commands and no response.
Attachment #269823 -
Flags: superreview?(dougt) → superreview-
Updated•17 years ago
|
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
Priority: P2 → --
Priority: -- → P1
Assignee | ||
Updated•13 years ago
|
Whiteboard: [sg:low] need reviews → [sg:low P3]
Updated•13 years ago
|
Attachment #269823 -
Flags: review?(cbiesinger) → review-
Comment 20•12 years ago
|
||
Is this still an issue? Could someone on Windows test?
Comment 21•12 years ago
|
||
(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?
Keywords: qawanted
Comment 22•12 years ago
|
||
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.
Thanks!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Updated•9 years ago
|
Group: core-security → core-security-release
Assignee | ||
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•