Closed Bug 1241100 Opened 9 years ago Closed 9 years ago

A form with POST method in local HTML file can create/overwrite arbitrary local file.

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox43 --- unaffected
firefox44 + verified
firefox45 + verified
firefox46 + verified
firefox47 --- verified
firefox-esr38 --- unaffected

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: regression, reporter-external, sec-high, Whiteboard: [adv-main47-])

Attachments

(2 files, 1 obsolete file)

Backout changeset 8bbed05c1661
3.49 KB, patch
lizzard
: approval-mozilla-aurora+
ritu
: approval-mozilla-release+
abillings
: sec-approval+
Details | Diff | Splinter Review
Part 2: Add testcase.
3.34 KB, patch
mayhemer
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review 
A form with POST method and action with local file in local HTML file can create or overwrite arbitrary file in user's local disk.

While the content of the file is not fully controllable, overwriting login script (.bashrc etc) can launch arbitrary local application with provided arguments, on next login.

******** WARNING: this testcase is dangerous!!! *******

Steps to reproduce:
  1. create local HTML file with following content
  2. open it in Firefox

==== begin ====
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>create file</title>
</head>
<body onload="document.getElementById('form').submit();">

<form id="form" action="this_file_is_created_automatically.txt" method="post" enctype="text/plain">
<input type="hidden" name="foo" value="bar echo 'hello!'">
<input type="submit">
</form>

</body>
</html>
==== end ====

Expected result:
Firefox opens File not found page.

Actual result:
A local file named "this_file_is_created_automatically.txt" is created in same directory, with following content, and Firefox opens the file, and opens save dialog.

==== begin ====
Content-Type: text/plain
Content-Length: 24

foo=bar echo 'hello!'
==== end ====

Regression window:
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3f3dbf14d937&tochange=eca3009f91f6

possibly from bug 1208124, as it touches POST method and upload.
Forgot to note that the value of action attribute can be absolute path, and relative path with/without ".." in its component.
https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/docshell/base/nsDocShell.cpp#10770
>   nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(channel));

uploadChannel should be cleared to nullptr at least when the channel is nsIFileChannel.
Not sure if there is other exploitable channel tho.
nsIFTPChannel will also be affected (not yet tested tho).
Also, there seems to be some channels with nsIUploadChannel interface in comm-central.

Anyway, if there could be several interfaces that should not accept POST, checking the channel by interface should be error-prone, and will be overlooked in future update.

IMO, the bug 1208124 patch should be backed out first, and, if necessary, add some flag to nsIChannel, that indicates the channel accepts upload with POST (or form), explicitly.  and then check the flag on POST, and get nsIUploadChannel only if the flag is set.  so that custom channel can accept POST, safely.

bz, can I have your opinion?
Flags: needinfo?(bzbarsky)
In case we should/could backout bug 1208124 patch (revision 8bbed05c1661), here's the patch to back it out (there is whitespace change since the original patch landed).
this is applicable both to mozilla-central and mozilla-beta.
Sorry for the lag....

This really needs necko folks to look at it.  It looks like filechannel does in fact implement nsIUploadChannel, which is pretty surprising.  It's not surprising for FTP, but does look undesirable for this case.

My gut feeling is that we should have an nsIPOSTDataChannel interface (name bikeshedding welcome) which inherits from nsIUploadChannel and is otherwise empty.  Then HttpChannel can implement it, and so can any add-ons that want to have protocols supporting POST.  We'd obviously have to somehow publicize that said add-ons should implement this interface.

The problem with just backing out bug 1208124 without putting anything in place as a replacement is that this would break add-ons like the one described in bug 1208124, right?
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bzbarsky)
> Then HttpChannel can implement it

More precisely, HttpBaseChannel, NullHttpChannel, and nsViewSourceChannel, with that last doing it conditional on its underlying channel implementing it.
I guess NullHttpChannel doesn't implement nsIUploadChannel, so doesn't need this new thing either.
fwiw, bug 151867 implemented nsIUploadChannel on file channel (and there will be the reason why).
https://github.com/mozilla/gecko-dev/commit/24fd40bd6fed6c34a812dac23387598051e418de

(In reply to Boris Zbarsky [:bz] from comment #5)
> The problem with just backing out bug 1208124 without putting anything in
> place as a replacement is that this would break add-ons like the one
> described in bug 1208124, right?

yes, it will break the add-on on beta/aurora/nightly channels.
it should already be broken on release channel, as bug 1208124 happens from Firefox 41, and fixed in Firefox 44.
(not sure if they've done some workaround for versions 41-43 tho)
So, backing it out means just delaying the fix, and I thought it's not so much issue, compared to the security flaw.

of course it would be better to fix with extra interface that can be used by such add-on :)
[Tracking Requested - why for this release]: Critical security issue

Oh, good point.  We haven't shipped 44 yet.

In that case, I agree we should back out bug 1208124 on 44.  I think the more correct fix is out of scope for 44 at this point.
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox-esr45: --- → ?
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

Approval Request Comment
[Feature/regressing bug #]: Bug 1208124
[User impact if declined]: Any file:// web page can overwrite any files the user
   has write access to.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low risk, short of add-on issues as discussed in this bug.  Just
   a backout to the state we were in for 43.
[String/UUID change made/needed]: None.

r=me for 44.
Attachment #8709986 - Flags: approval-mozilla-beta?
Because this is a sec-critical bug that affects more than mozilla-central, you need to get sec-approval+ before landing, so please fill that out.
Flags: needinfo?(arai.unmht)
Do we need sec-approval to land on just the 44 relbranch?  And if so, how long will that take and how does compare with the release schedule?
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.

Which older supported branches are affected by this flaw?  44, 45.

If not all supported branches, which bug introduced the flaw? Bug 1208124.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Backporting is simple.  But we probably want a different fix for 46 here that actually solves the problem instead of just backing out.  And we may want that for 45 too.  So for now we just want to land the backout on 44.

How likely is this patch to cause regressions; how much testing does it need?  Not likely and not much.  Just a straight backout to the state we were in for 43.
Attachment #8709986 - Flags: sec-approval?
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

It takes less time if you ping me or Dan directly in IRC.
sec-approval+
Attachment #8709986 - Flags: sec-approval? → sec-approval+
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

mccr8 convinced me we should just land this on m-c and aurora too, then do a followup for the "right" fix.
Attachment #8709986 - Flags: approval-mozilla-aurora?
This is bad and we need a rebuild of 44 for it if we can land it on relbranch.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Clearing ni? because bz filled out the sec-approval.
Flags: needinfo?(arai.unmht)
OK. We are already preparing to do an RC2 for 44 desktop. Ritu over to you.
Assignee: nobody → arai.unmht
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

Bz and Al chatted with me about this one. This clearly meets the bar as it's a (easily exploitable) sec-crit issue. Taking it in Fx44 build2 (RC2).
Flags: needinfo?(rkothari)
Attachment #8709986 - Flags: approval-mozilla-beta? → approval-mozilla-release+
it's not a web drive-by (user would have to first download) so I'm lowering rating to sec-high. That doesn't mean it's any less important to back out of the 44 release! Nice catch, Tooru.
Blocks: 1208124
Keywords: sec-critical → sec-high
Group: core-security → core-security-release
Flags: in-testsuite?
(In reply to Boris Zbarsky [:bz] from comment #5)
> Sorry for the lag....
> 
> This really needs necko folks to look at it. 

And what about a new protocol handler flag like ALLOW_POST_DATA?
Flags: needinfo?(honzab.moz)
If we have free flags there, sure.  I don't have a strong opinion...
OTOH, the idea with an interface may be more convenient and flexible.  It fits this docshell case nicely and from top of my head I don't see any issues with it.

Also, we could just blacklist file:// schema in docshell (quick and dirty).
That's good for file://, but doesn't address ftp://
as an aside what's the attack against ftp here? It seems that would just allow the uploading of a file if you've got permissions to do it.. which seems roughly the same semantic as http.. Maybe we just don't want to do that out of docshell..

I don't have a strong opinion about a new flag vs a new interface..
Flags: needinfo?(mcmanus)
> as an aside what's the attack against ftp here?

Allows a random website to upload a file to some ftp server you have permissions to upload to, right?
Thank you for approval and landing!
If there is anything else I can do for backing out for aurora/nightly, please tell me.


then, prepared a patch for new interface (nsIPOSTDataChannel) way.
not sure whether it's better addressing it in this bug or separated bug tho,
can I get some feedback on it?

it adds nsIPOSTDataChannel interface under nsIUploadChannel.
and adds the interface to HttpBaseChannel, nsHttpChannel, and nsViewSourceChannel.

in DocShell, it checks if the channel has nsIPOSTDataChannel interface or not.
the nsICacheInfoChannel/cacheKey things are now done completely separately.
Attachment #8710097 - Flags: feedback?(mcmanus)
Comment on attachment 8710097 [details] [diff] [review]
(DO NOT LAND THIS NOW) followup: Implement nsIPOSTDataChannel for the channel accepts form POST.

Review of attachment 8710097 [details] [diff] [review]:
-----------------------------------------------------------------

honza seems to have an opinion on what he'd like it to look like, so I'll ask him handle the patch as he sees fit. probly should be a different bug.
Attachment #8710097 - Flags: feedback?(mcmanus) → feedback?(honzab.moz)
(In reply to Boris Zbarsky [:bz] from comment #27)
> > as an aside what's the attack against ftp here?
> 
> Allows a random website to upload a file to some ftp server you have
> permissions to upload to, right?

sounds a lot like http POST :) (ok, the default server handling is probly different and that's meaningful.)
Comment on attachment 8710097 [details] [diff] [review]
(DO NOT LAND THIS NOW) followup: Implement nsIPOSTDataChannel for the channel accepts form POST.

Review of attachment 8710097 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good to me!  With comments addressed (or discussed) we can proceed to review.

::: netwerk/base/nsIPOSTDataChannel.idl
@@ +8,5 @@
> +/**
> + * nsIPOSTDataChannel
> + *
> + * An uploado channel may optionally implement this interface if it supports
> + * the notion of HTTP POST.

* Channel classes that want to be allowed for HTML form POST action must implement this interface.

@@ +11,5 @@
> + * An uploado channel may optionally implement this interface if it supports
> + * the notion of HTTP POST.
> + */
> +[scriptable, uuid(fc826b53-0db8-42b4-aa6a-5dd2cfca52a4)]
> +interface nsIPOSTDataChannel : nsIUploadChannel

I think a bit better name would be nsIFormPOSTActionChannel
Attachment #8710097 - Flags: feedback?(honzab.moz) → feedback+
Flags: sec-bounty?
can I land the backout patch to m-i?  or should I wait for aurora approval?
Blocks: 1241377
Comment on attachment 8710097 [details] [diff] [review]
(DO NOT LAND THIS NOW) followup: Implement nsIPOSTDataChannel for the channel accepts form POST.

thank you for your feedback :)
attached updated patch to bug 1241377.
Attachment #8710097 - Attachment is obsolete: true
(In reply to Tooru Fujisawa [:arai] from comment #32)
> can I land the backout patch to m-i?  or should I wait for aurora approval?

You can land on m-i, as the patch has sec-approval. (Well, and as it has already landed publicly.)
sylvestre, can you take a look at the approval-mozilla-aurora?
Flags: needinfo?(sledru)
Once it landed in m-c, sure :)
Flags: needinfo?(sledru)
(In reply to Wes Kocher (:KWierso) from comment #26)
> https://hg.mozilla.org/releases/mozilla-release/rev/4eb866ca6a12
> 
> Unsure if landing this means any status flags need to be updated...

I think this means that it is fixed in 44, so I'm updating the status flag.
status-firefox44: affected → fixed
Flags: needinfo?(lhenry)
This appears to have been merged to m-c already:
https://hg.mozilla.org/mozilla-central/rev/9eee2ef7aad1
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8709986 [details] [diff] [review]
Backout changeset 8bbed05c1661

Backout for sec issue, has sec-approval. Please uplift to aurora.
Attachment #8709986 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
thank you all :)

"in-testsuite?" flag means that we should land testcase basically does same thing as comment #0, maybe later, right?
if so, will try creating the patch.
should I attach it to this bug, or different bug?


Also, what's the current status of firefox-esr45?  if the branch is not yet separated from aurora, I guess we could mark it as "fixed".
just in case, here's the patch that adds testcase.
it uses almost same way as bug 1241377's testcase, so will ask review after that.

anyway, this testcase describes how to exploit the flaw, so would be better waiting for a while.
(is update for all channels available yet?)
Flags: sec-bounty? → sec-bounty+
clearing firefox-esr45 tracking flag, as the branch is not yet separated from 45, and is already fixed in 45.
status-firefox-esr45: affected → ---
tracking-firefox-esr45: ? → ---
Attachment #8711439 - Flags: review?(honzab.moz)
Attachment #8711439 - Flags: review?(honzab.moz) → review+
Comment on attachment 8711439 [details] [diff] [review]
Part 2: Add testcase.

thank you for reviewing :)

Asking separated sec-approval, as this contains more critical information than backout patch.
I'd like to know when to land it.


[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Very easy.
It clearly describes how to exploit the flaw.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Yes.
The testcase tries to overwrite local file by form POST.

> Which older supported branches are affected by this flaw?

This is test-only patch for backout patch that was landed to 44, 45, 46 branches.
(not sure which branch needs testcase...)

> If not all supported branches, which bug introduced the flaw?

Bug 1208124.

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

This should be applicable to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

Less likely, as this does not change any non-test code.
Automation should catch any regression if happens.
Attachment #8711439 - Attachment description: (not yet reviewed) Part 2: Add testcase. → Part 2: Add testcase.
Attachment #8711439 - Flags: sec-approval?
Comment on attachment 8711439 [details] [diff] [review]
Part 2: Add testcase.

sec-approval+ for tests. We didn't ship an affected version of this code so we can just check in the tests.
Attachment #8711439 - Flags: sec-approval? → sec-approval+
it would be nice to uplift it to avoid breaking this again by other uplifting patch tho, I'm not sure the rule.

dveditz, can I have your opinion?
Flags: needinfo?(dveditz)
We don't usually uplift the test patches because most of the time anything that would be uplifted and break a branch would have triggered the test cases when it landed on mozilla-central. Not always, though, if we take a different branch patch than trunk.

In this case, though, we didn't "fix" it as much as un-fix bug 1208124. That wasn't re-opened because it was re-fixed (correctly) in bug 1241377. If there's pressure to uplift bug 1241377 we should land the test patch along with it, but otherwise I don't think we need to.
Flags: needinfo?(dveditz)
thanks :)

IMO bug 1241377 patch is not so urgent and no need to uplift, for now.
Reproduced this issue on 44.0b6.

Confirming the fix on Mac OS X 10.9.5, Ubuntu 12.04x86 and Windows 10x64 using the following builds:
* Latest 47.0a2 Aurora
* Firefox 46.0RC build 3
* Firefox 45.0.2
* Firefox 44.0.2
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
status-firefox45: fixed → verified
status-firefox46: fixed → verified
status-firefox47: fixed → verified
QA Contact: cornel.ionce
Whiteboard: [adv-main47-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: