448611 - File upload can be forged using a textarea

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Giorgio Maone [:ma1]]

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="http://some.cms.com/upload_web_page.php">
<textarea name='pagefile"; filename="forgedFileName.html
Content-Type: text/html; '>
forged content
</textarea>
</form>
<script>document.forms[0].submit()</script>

One possible attack scenario is using CSRF against a CMS allowing page uploads in order to inject a malicious web pages.

Comment 1

[Giorgio Maone [:ma1]]

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. 

Comment 2

[Wladimir Palant]

I just verified that Internet Explorer does exactly the same thing as us, the resulting POST data looks like this:

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

forged content

-----------------------------7d82ab2b1660736--

Opera however will escape double quotes:

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

forged content

------------ZsAplNTq9gnW1lGO0F8093--

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).

Comment 3

[Wladimir Palant]

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).

Comment 4

[Daniel Veditz [:dveditz]]

This looks like a public blog post from last February so there may not be much point in the security-sensitive flag.
http://kuza55.blogspot.com/2008/02/csrf-ing-file-upload-fields.html

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.

Comment 5

[Daniel Veditz [:dveditz]]

Jonas: does this fall in your area?

Comment 6

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

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:

http://en.wikipedia.org/wiki/CSRF

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.

Comment 7

[Wladimir Palant]

How about doing the same thing as Opera - escaping double quotes in field names? Newline characters should be removed or replaced by spaces.

Comment 8

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

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.

Comment 9

[Giorgio Maone [:ma1]]

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.

Comment 10

[Wladimir Palant]

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.

Comment 11

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

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
   "text/plain".
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").

Comment 12

[Wladimir Palant]

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.

Comment 13

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

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.

Comment 14

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

Sorry, that should be escaping quotes in field names, not in file names.

Comment 15

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

Not blocking unless we can show that this is significantly different than CSRF in general.

Comment 16

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

Dan: What's your take here. Is this really something we want to change?

Comment 17

[Daniel Veditz [: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.

Comment 18

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

Dan: really? Did you read my previous comments in this bug?

Comment 19

[uamjet602]

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.

Comment 20

[Paul Stone]

Chrome isn't affected by this bug as it URL encodes quotes and newlines in field names.

Comment 21

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

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();
xhr.open("POST", "http://somewhere.com");
xhr.withCredentials = true;
xhr.setRequestHeader("Content-Type", "multipart/form-data");
xhr.send(myencodeddata);

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.

Comment 22

[Paul Stone]

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

Comment 23

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

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).

Comment 24

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

Attachment: WIP

Lacks tests

Comment 25

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

Attachment: 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.

Comment 26

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

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");
Ditto.


> 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.

Comment 27

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

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.

Comment 28

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

Checked in: http://hg.mozilla.org/mozilla-central/rev/f65bef71ac10

Smaug, please do let me know if the comment doesn't explain things. I'm happy to explain here or amend the comment.

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Do we need to fix this on branches too?

Comment 32

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

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Can the blobs be submitted cross-site without preflights, etc?

Comment 34

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

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.

Comment 36

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

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 evil.com and picks a "dangerous" file in a filepicker, such a file can be used to attack bunnies.com.

I really wouldn't expect users to understand that.