r56793 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r56792‎ | r56793 | r56794 >
Date:23:44, 22 September 2009
Author:brion
Status:ok (Comments)
Tags:
Comment:
Workaround for bugs with Commonist (mwapi-based) and other upload bots.
The upload form recently started checking for wpEditToken, but this is only actually needed when we do uploads by URL -- file uploads can't be injected by a CSRF script. Now skipping the token check if the token was empty and we're doing a regular file upload.
Confirmed that Commonist current JNLP release can upload to Wikimedia Commons with this patch.
Modified paths:
  • /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialUpload.php
@@ -59,9 +59,6 @@
6060 # filename and description
6161 return;
6262 }
63 - //if it was posted check for the token (no remote POST'ing with user credentials)
64 - $token = $request->getVal( 'wpEditToken' );
65 - $this->mTokenOk = $wgUser->matchEditToken( $token );
6663
6764 # Placeholders for text injection by hooks (empty per default)
6865 $this->uploadFormTextTop = "";
@@ -73,13 +70,24 @@
7471 $this->mCopyrightStatus = $request->getText( 'wpUploadCopyStatus' );
7572 $this->mCopyrightSource = $request->getText( 'wpUploadSource' );
7673 $this->mWatchthis = $request->getBool( 'wpWatchthis' );
77 - $this->mSourceType = $request->getText( 'wpSourceType' );
 74+ $this->mSourceType = $request->getVal( 'wpSourceType', 'file' );
7875 $this->mDestWarningAck = $request->getText( 'wpDestFileWarningAck' );
7976
8077 $this->mReUpload = $request->getCheck( 'wpReUpload' ); // retrying upload
8178
8279 $this->mAction = $request->getVal( 'action' );
8380 $this->mUpload = UploadBase::createFromRequest( $request );
 81+
 82+ // If it was posted check for the token (no remote POST'ing with user credentials)
 83+ $token = $request->getVal( 'wpEditToken' );
 84+ if( $this->mSourceType == 'file' && $token == null ) {
 85+ // Skip token check for file uploads as that can't be faked via JS...
 86+ // Some client-side tools don't expect to need to send wpEditToken
 87+ // with their submissions, as that's new in 1.16.
 88+ $this->mTokenOk = true;
 89+ } else {
 90+ $this->mTokenOk = $wgUser->matchEditToken( $token );
 91+ }
8492 }
8593
8694 public function userCanExecute( $user ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r56794Merge r56793 from trunk -- fix for uploads w/ Commonist and other bot tools t...brion23:45, 22 September 2009
r114429* (bug 34212) ApiBlock/ApiUnblock allow action to take place without a token ...reedy19:43, 22 March 2012

Comments

#Comment by Simetrical (talk | contribs)   01:08, 23 September 2009

Why can't file uploads be faked via JS?

#Comment by Aaron Schulz (talk | contribs)   03:35, 23 September 2009

With same-domain-policy *off* it seems like JS could POST uploads via session forgery.

#Comment by Simetrical (talk | contribs)   15:09, 23 September 2009

If same-domain policy is off, you already have XSS, which is a strict superset of CSRF in terms of what it could do AFAICT. If you have XSS, you could create a hidden iframe to create the form, and scrape the edit token from it, so the token doesn't help in this case.

AFAICT, CSRF (for POSTs, let's say) is when you have a form on your site that looks like it submits to your site but actually submits to Wikipedia or whatever. For instance, a form asking you to enter a comment on the blog post, but really the form has action="[http://en.wikipedia.org/w/index.php?title=Special:Upload&action=submit http://en.wikipedia.org/w/index.php?title=Special:Upload&action=submit]", and the comment field has a meaningless name attribute that Wikipedia will ignore, and there are a bunch of hidden inputs like <input type=hidden name=wpUploadFile value="...appropriately encoded hello.jpg...">. Then the user submits the form and, if they're logged into Wikipedia, they just uploaded an evil image in a manner only traceable if you're logging Referers (and then only if they're sending them). But if there's a hidden token that's not guessable, this becomes impossible: you need XSS to scrape the token.

I don't understand why file uploads wouldn't be vulnerable to this.

#Comment by Mdale (talk | contribs)   15:40, 23 September 2009

Simetrical is essentially correct. File upload will be vulnerable to the cross site uploading with other persons credentials as described.

Although I don't think you can encode an upload via a form value attribute. But I think you can construct a XHR POST request with image data via javascript.

Even if the browser forbids XHR cross domain POST checks and does not enable form value attribute encoding... If you can get the user has to select an image the wikitext description could be changed without a token. (although the image won't be of the attackers choosing rather something the user thought they where contributing to a blog post )

With the editToken this is not so possible since you need XSS to scrape the token.

We should encourage Commonist and other upload bots to switch to the new upload api instead of submitting thing via the upload form. Maybe make notice that the wpToken will be enabled at X date to give them time to update their scripts?

#Comment by Simetrical (talk | contribs)   15:50, 23 September 2009

Although I don't think you can encode an upload via a form value attribute.

Why not?

Even if the browser forbids XHR cross domain POST checks

Is there any browser that permits cross-domain XHR at all (without opt-in by the target)? See the W3C spec: "If the origin of url is not same origin with the XMLHttpRequest origin the user agent should raise a SECURITY_ERR exception and terminate these steps."

#Comment by Mdale (talk | contribs)   16:20, 23 September 2009

Because I think you need the browser to send some extra things for it to be interpreted as a file upload by php something like:

Content-Disposition: form-data; name="wpUploadFile"; filename="1Rosa-dos-ventos.JPG"

Content-Type: image/jpeg


ÿØÿàÿ (image data here) 

instead of what you could generate with a web form:

Content-Disposition: form-data; name="wpUploadFile"


ÿØÿàÿ (user attribute image data here)

And your right about the XHR POST I was thinking that file uploads could be mangled via javascript that way .. but can't recreate that locally or find documentation indicating its a problem.

At any-rate its a good idea to check the token for the last case described. (uploading an image to a compromised blog and having that redirect to a commons upload with your credentials )


#Comment by Tgr (talk | contribs)   22:08, 23 September 2009

"Although I don't think you can encode an upload via a form value attribute."

In theory you can't, in practice browser errors might allow it. See an older example for such an attack here: http://kuza55.blogspot.com/2008/02/csrf-ing-file-upload-fields.html

So it's good practice to have layered security and protect against CSRF even in cases where it is not obviously necessary.

#Comment by Tgr (talk | contribs)   22:14, 23 September 2009

"AFAICT, CSRF (for POSTs, let's say) is when you have a form on your site that looks like it submits to your site but actually submits to Wikipedia or whatever."

Normally you can create the form by javascript and send it in a hidden iframe or something, the user doesn't need to initiate it or even notice there is a POST happening. That's not supposed to work for uploads as input type=file cannot be tampered with from javascript, but it's best to not rely on that.

#Comment by Aaron Schulz (talk | contribs)   03:24, 24 September 2009

How does the CSRF form select the source of the file upload?

#Comment by Aaron Schulz (talk | contribs)   03:30, 24 September 2009

I suppose it could be an upload form to WP that looks like it is local to the site.

Status & tagging log