Closed Bug 448611 Opened 12 years ago Closed 10 years ago

File upload can be forged using a textarea


(Core :: Security, defect)

Windows Server 2003
Not set



Tracking Status
blocking2.0 --- final+


(Reporter: mao, Assigned: sicking)


(Blocks 1 open bug)


(Keywords: html5, Whiteboard: [sg:low][need decision from dveditz])


(1 file, 1 obsolete file)

Bug 442010 can be exploited the other way around, i.e. uploading the content of a textarea as it was a file by injecting a content-type like this:

<form enctype="multipart/form-data" method="post" action="">
<textarea name='pagefile"; filename="forgedFileName.html
Content-Type: text/html; '>
forged content

One possible attack scenario is using CSRF against a CMS allowing page uploads in order to inject a malicious web pages.
OK, I read more carefully bug 442010 and that's definitely not the same bug as this, albeit related.
Here the insertion point is the *field* name, rether than the file name, making it suitable for cross-site attacks. 
OS: Windows Server 2003 → All
I just verified that Internet Explorer does exactly the same thing as us, the resulting POST data looks like this:

Content-Disposition: form-data; name="pagefile"; filename="forgedFileName.html"
Content-Type: text/html; "

forged content


Opera however will escape double quotes:

Content-Disposition: form-data; name="pagefile\"; filename=\"forgedFileName.html\"
Content-Type: text/html; "

forged content


Of course you can do this kind of thing with XMLHttpRequest already but it is restricted by the same-origin policy. Faking file upload with regular forms allows CSRF as well as XSS if the site doesn't validate the file name properly (which it often doesn't).
OS: All → Windows Server 2003
Checked this input against PHP interpreter - it accepts the data sent by Firefox as a file (obviously, there is nothing "wrong" with it). Opera's request is interpreted as a field named |pagefile";_filename="forgedFileName_html"| which is in line with RFC 822 (see quoted-string definition).
Blocks: csrf
This looks like a public blog post from last February so there may not be much point in the security-sensitive flag.

Not sure how to rate the severity. It can't be used to hack Firefox itself or the user's machine, but it's definitely a potential csrf risk in the right circumstance.
Flags: wanted1.8.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Flags: blocking1.8.1.17?
Whiteboard: [sg:low]
Flags: blocking1.8.1.17?
Jonas: does this fall in your area?
Assignee: nobody → jonas
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.3+
Flags: blocking1.8.1.18+
Yeah, it's well known that you can launch CSRF attacks using browsers. Server operators need to protect themselves using any of the available methods. You can read more about it here:

This is very bad indeed, but hardly new information. Not really sure what we can do about it. I don't think forging file uploads are any more or less bad than anything else really. All the CSRF attacks that I have heard about so far hasn't needed file upload at all. For example banking transactions usually don't use file uploads.
How about doing the same thing as Opera - escaping double quotes in field names? Newline characters should be removed or replaced by spaces.
So both cross-site XHR as well as microsofts XDR is currently designed to allow POSTing arbitrary data with the Content-Type "text/plain", with the rationale that <form>s already allow this. (neither XHR nor XDR will allow the data to be read unless the page opts in, but at that point the request has already happened of course).

I think the fact of the matter is that for the vast majority of sites exploitable to CSRF the data it parses is simple <form> data. Simply because that is what it expects (it just doesn't expect it from cross site). So you don't have to get fancy with field names and the like, the browser will encode the data in exactly the format the server want.

So sure, we could put limits on what field names we send out, however it is unlikely to help many servers. And in order to be effective we'd have to put a stop to the XDR and cross-site XHR specs.

Or to put it another way: Have you ever encountered a server that would be helped by the remedies you are proposing? Or would we be making sacrifices for no real added value?

There are other proposals for how to help servers deal with CSRF. The prevailing idea right now is using the Origin header. However there are other bugs and forums that cover that topic.
Jonas, nobody proposed fixing this bug as a CSRF panacea, so I'm not sure of the reason why you're pointing general CSRF info and remedies like the Origin header here.
What we were suggesting is just that if a form is vulnerable to CSRF but not to "conventional" XSS (a common case, I suspect) and stores file uploads in a public area (e.g. a CMS), you could leverage an otherwise low-impact CSRF attack to inject persistent XSS pages or malware.
Without this bug, an attacker could not upload arbitrary "files" through CSRF, and this is what most people expects, I guess.
Jonas, I don't see how cross-site XHR is relevant here - files are sent with Content-Type: multipart/form-data and not text/plain. Note also that reflective XSS is impossible with XHR. As to which servers will be helped - quite a few. Above I tested the PHP interpreter and it processes escaped quotation marks in field names correctly. So removing line breaks and escaping quotes will immediately eliminate the CSRF/XSS threat for all PHP-based sites as far as file uploads go. Granted, that's a small improvement but it is an improvement.
The thing is that it seems like your proposal seems to fix a very narrow problem. It fixes CGIs that fulfill all of the following requirements:
1. Has no current CSRF protection (this is all too common).
2. Accepts file data.
3. Not exploitable unless file data is attached.
4. Accepts multipart/form-data encoded data.
5. Enforces the mimetype, i.e. is not exploitable when Content-Type is set to
6. Honors quotes in filenames (apparently PHP does this)
6. The user can't be asked to attach what to the user seems like a harmless file
   (i.e. contains no data that the user thinks is private to him/her).

The cost is that we couldn't implement future specs that would allow setting the value of a <input type=file> control based on data provided by the script.

I guess i'm not exited about the arbitrary restriction for file data, while we allow all sorts of other data (including file data with the "wrong" Content-Type "text/plain").
Points 4, 5 and 6 should be met by all PHP-based sites. Note also that 4 follows directly from 2 (there is no other way to send files). Second point 6 :) isn't a real restriction - if the user attaches the file, the attack requires user interaction, and the attacker cannot control the file name (in particular, he cannot introduce special characters like <> or | in the file name, at least Windows won't accept file names like this). Even to upload a file with given content he has to ask the user to download that file to disk and attach it then, that's even less likely successful then.

So real restrictions are only 2 and 3 - what we already said before. Still, I think this is worth doing.
Even if we take the actions suggested here we're still just protecting against *forging* file controls, we wouldn't protect against *using* file controls.

I.e. you could just use a normal file control and kindly ask the user to attach a file which is harmless to the user, but which would exploit the server. I.e. you would be relying on users to be protecting the server.

Additionally, if we ever in the future allowed something like

myFileInput.value = myFileObject;

(where myFileObject is a File object gotten through input.files.item(0) or some such.) we would be back to allowing the same submissions that we're trying to protect from here.

Sure, I'd be fine with quoting filenames, this seems useful anyways since it allows filenames with quotes. But I don't think it'll actually protect any servers.
Sorry, that should be escaping quotes in field names, not in file names.
Not blocking unless we can show that this is significantly different than CSRF in general.
Flags: blocking1.9.1? → blocking1.9.1-
Flags: blocking1.9.0.5?
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.19?
Flags: blocking1.8.1.18+
Flags: blocking1.9.0.5?
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19?
Flags: blocking1.8.1.19+
Dan: What's your take here. Is this really something we want to change?
Whiteboard: [sg:low] → [sg:low][need decision from dveditz]
Yes, we should fix this. Realistically it's not going to block a Firefox 2.0 release. The issue is public so I'm going to remove the security flag.
Group: core-security
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19+
Dan: really? Did you read my previous comments in this bug?
So it may, or may be not a security issue, it's still an RFC violation:

RFC822 3.4.4:
[...]A quotation-mark that is to  be  part  of  a quoted-string, a parenthesis that is to be part of a comment and a backslash that is to be part of either must  each be preceded by the quote-character backslash ("\").[...]

Added html5 keyword for the compliance issue.
Keywords: html5
Chrome isn't affected by this bug as it URL encodes quotes and newlines in field names.
I'll try to fix this for FF4.

However note that using CORS and cross-site XMLHttpRequest, you can generate a multipart/form-data request to any server which includes any file contents. One way of doing this is to simply generate the encoded data yourself and do something like:

xhr = new XMLHttpRequest();"POST", "");
xhr.withCredentials = true;
xhr.setRequestHeader("Content-Type", "multipart/form-data");

The above should work in all browsers that support CORS, including safari and chrome.

So there is no way to fix the "forge" aspect here. What we can do is to fix the part where a weird filename accidentally messes up a encoded submission.
blocking2.0: --- → final+
Presumably, the CORS stuff would only work if the server set the appropriate (insecure) headers? i.e. the server would have to set:

Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: True
No, you can always send the request, which obviously happens before any response headers have been inspected.

I would additionally expect specs to soon support the ability to set the value of a <input type=file> to a file provided by the page (*Not* to a path-name provided by the page obviously. That would allow reading arbitrary data from the users file system).
Attached patch WIP (obsolete) — Splinter Review
Lacks tests
Attached patch Patch to fixSplinter Review
Somewhat big patch, but mostly test changes. Found a couple of other instances where we don't behave like IE (text/plain newline encoding, and <textarea name="">) so fixed those as well.
Attachment #481153 - Attachment is obsolete: true
Attachment #485237 - Flags: review?(Olli.Pettay)
Comment on attachment 485237 [details] [diff] [review]
Patch to fix

>@@ -665,19 +662,24 @@ nsFSTextPlain::GetEncodedSubmission(nsIU
>     escapedBody.Adopt(escapedBuf);
>     path += NS_LITERAL_CSTRING("&force-plain-text=Y&body=") + escapedBody;
>     rv = aURI->SetPath(path);
>   } else {
>     // Create data stream
>+    nsCString cbody;
>+    EncodeVal(mBody, cbody, false);
>+    cbody.Adopt(nsLinebreakConverter::
>+                ConvertLineBreaks(cbody.get(),
>+                                  nsLinebreakConverter::eLinebreakAny,
>+                                  nsLinebreakConverter::eLinebreakNet));
>     nsCOMPtr<nsIInputStream> bodyStream;
>-    rv = NS_NewStringInputStream(getter_AddRefs(bodyStream),
>-                                          mBody);
>+    rv = NS_NewCStringInputStream(getter_AddRefs(bodyStream), cbody);
Could you explain this, and even add some comment.

>diff --git a/content/html/content/src/nsHTMLTextAreaElement.cpp b/content/html/content/src/nsHTMLTextAreaElement.cpp
>--- a/content/html/content/src/nsHTMLTextAreaElement.cpp
>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
>@@ -905,17 +905,18 @@ nsHTMLTextAreaElement::SubmitNamesValues
>   if (IsDisabled()) {
>     return NS_OK;
>   }
>   //
>   // Get the name (if no name, no submit)
>   //
>   nsAutoString name;
>-  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::name, name)) {
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::name, name);
>+  if (name.IsEmpty()) {
>     return NS_OK;
>   }
This is a bit risky, but seems like input element does the same.

> nsresult nsLinebreakConverter::ConvertLineBreaksInSitu(char **ioBuffer, ELinebreakType aSrcBreaks,
>             ELinebreakType aDestBreaks, PRInt32 aSrcLen, PRInt32* outLen)
> {
>   NS_ASSERTION(ioBuffer && *ioBuffer, "Null pointer passed");
>   if (!ioBuffer || !*ioBuffer) return NS_ERROR_NULL_POINTER;
>-  NS_ASSERTION(aDestBreaks != eLinebreakAny, "Invalid parameter");
>+  NS_ASSERTION(aDestBreaks != eLinebreakAny &&
>+	       aSrcBreaks != eLinebreakSpace, "Invalid parameter");
You have a tab here. Use spaces.

> PRUnichar* nsLinebreakConverter::ConvertUnicharLineBreaks(const PRUnichar* aSrc,
>             ELinebreakType aSrcBreaks, ELinebreakType aDestBreaks, PRInt32 aSrcLen, PRInt32* outLen)
> {
>-  NS_ASSERTION(aDestBreaks != eLinebreakAny, "Invalid parameter");
>+  NS_ASSERTION(aDestBreaks != eLinebreakAny &&
>+	       aSrcBreaks != eLinebreakSpace, "Invalid parameter");

> nsresult nsLinebreakConverter::ConvertUnicharLineBreaksInSitu(PRUnichar **ioBuffer,
>             ELinebreakType aSrcBreaks, ELinebreakType aDestBreaks, PRInt32 aSrcLen, PRInt32* outLen)
> {
>   NS_ASSERTION(ioBuffer && *ioBuffer, "Null pointer passed");
>   if (!ioBuffer || !*ioBuffer) return NS_ERROR_NULL_POINTER;
>-  NS_ASSERTION(aDestBreaks != eLinebreakAny, "Invalid parameter");
>+  NS_ASSERTION(aDestBreaks != eLinebreakAny &&
>+	       aSrcBreaks != eLinebreakSpace, "Invalid parameter");
And here.

> nsresult nsLinebreakConverter::ConvertStringLineBreaks(nsString& ioString,
>           ELinebreakType aSrcBreaks, ELinebreakType aDestBreaks)
> {
>-  NS_ASSERTION(aDestBreaks != eLinebreakAny, "Invalid parameter");
>+  NS_ASSERTION(aDestBreaks != eLinebreakAny &&
>+	       aSrcBreaks != eLinebreakSpace, "Invalid parameter");
And here

r=me, assuming the first comment gets an explanation.
Attachment #485237 - Flags: review?(Olli.Pettay) → review+
I'm not sure what you think is unclear in the first thing you commented on. I've added the following comment to the code:

    // We do want to send the data through the charset encoder and we want to
    // normalize linebreaks to use the "standard net" format (\r\n), but we
    // don't want to perform any other encoding. This means that names and
    // values which contains '=' or newlines are potentially ambigiously
    // encoded, but that how text/plain is specced.
Checked in:

Smaug, please do let me know if the comment doesn't explain things. I'm happy to explain here or amend the comment.
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #25)
> Created attachment 485237 [details] [diff] [review]
> Patch to fix
> Somewhat big patch, but mostly test changes. Found a couple of other instances
> where we don't behave like IE (text/plain newline encoding, and <textarea
> name="">) so fixed those as well.

Are those in the spec? If not, could you please file bugs?
Duplicate of this bug: 627967
Do we need to fix this on branches too?
I don't actually think this is a security issue at all. There are already webstandards which allow you to generate Blobs which can then be submitted without the need for "forging".

The fix which landed here mostly aligned us as far as submission format with other browsers.
Can the blobs be submitted cross-site without preflights, etc?
Yes, CORS allow cross-site POSTs without preflight as long as you're staying within a small set of content-types (multipart/form-data is one of them), and don't set any other exotic headers.
A site that accepts files from users and performs "dangerous" actions on them, without any CSRF protection, is playing a very dangerous game.

Such as site relies on the fact that the user understands that if he goes to and picks a "dangerous" file in a filepicker, such a file can be used to attack

I really wouldn't expect users to understand that.
You need to log in before you can comment on or make changes to this bug.