Page MenuHomePhabricator

No rate limits on uploading files
Closed, ResolvedPublic

Description

We don't rate limit uploading files. We should.


patch:

  • 1.25 - same as master ()
  • 1.24 - same as master ()
  • 1.23 -

affected versions:
type: dos
CVE: CVE-2015-8003

Event Timeline

csteipp raised the priority of this task from to Medium.
csteipp updated the task description. (Show Details)
csteipp added a project: acl*security.
csteipp changed the visibility from "Public (No Login Required)" to "Custom Policy".
csteipp changed the edit policy from "All Users" to "Custom Policy".
csteipp changed Security from None to Software security bug.
csteipp subscribed.

Ran into my first stab at this while working on something else, so figured I should just finish it.

I don't like checking all the entrypoints, but that's where we do the rights and block checks, so seemed like a logical place. Not sure if there are other ways to upload files? Maybe extensions?

@dpatrick / @Anomie, could you take a look at the patch and see if it seems sane to you?

To test, I'm using

and

$wgRateLimits = array(
	'upload' => array(
		'user' => array( 1, 60 ),
		'newbie' => array( 1, 60 ),
		'ip' => array( 1, 60 ),
	)
);

@csteipp, I observed that the upload was appropriately throttled, from both the API and the web form. However, I also noticed that simply viewing the web form is also throttled. If I navigate to http://localhost:8080/wiki/Special:Upload, then hit refresh without selecting a file or submitting the form, I receive the action-throttled message in the browser.

In the API bit, a chunked upload would be applying the throttle to each chunk of the file.

It also throttles the polling for status if the client is waiting for an async upload or an upload-from-url. Even if the former is intended, this probably isn't.

Moved the throttling later in both the special page and api, so chunks and warnings are correctly handled. This allows a user to upload to the stash unthrottled. Otherwise in UploadWizard, the upload to the stash was one count against the throttle, then finalizing the upload was a second. I'm not sure if that's the right behavior or not. As a side effect, you can add more files to the initial step of UploadWizard than the throttle allows, and UploadWizard just fails to move the file from the stash. Except for missing an api-error-ratelimited message, the failure is fairly graceful.

@MarkTraceur, is there someone on the multimedia team who can comment on this?

My two cents: This sounds fine, James_F might want to comment, but UploadWizard already throttles to 3 uploads to stash or 3 stash upload finalizations at once (per window, I guess), so this doesn't look like a huge issue from that end.

@dpatrick / @Anomie, could you take a look at the new patch?

API bit looks ok. Haven't tested.

When uploading at http://localhost:8080/wiki/Special:Upload, I now get throttled message after the final form submission, after the file has been uploaded and compared to existing files, and I've entered a description. This might be frustrating for users. Other than this, the patch looks good.

Made Special:Upload use a RecoverableUploadError instead of throwing an exception. So error message is a little nicer.

Deployed yesterday

(2015-09-08) 21:02 csteipp: deployed patches for T108616 T91850 T91205 to wmf21 & 22

Patch applies to all branches cleanly except REL1_23. Will work on alternate patch.

This comment was removed by demon.
demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 16 2015, 6:13 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

Change 246868 had a related patch set uploaded (by Chad):
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246868

Change 246873 had a related patch set uploaded (by Chad):
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246873

Change 246879 had a related patch set uploaded (by Chad):
Add throttle check in ApiUpload and SpecialUpload.

https://gerrit.wikimedia.org/r/246879

Change 246884 had a related patch set uploaded (by Chad):
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246884

Change 246868 merged by jenkins-bot:
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246868

Change 246879 merged by jenkins-bot:
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246879

Change 246873 merged by Chad:
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246873

Change 246973 had a related patch set uploaded (by Chad):
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246973

Change 246884 merged by jenkins-bot:
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246884

Change 246973 merged by jenkins-bot:
SECURITY: Throttle uploads

https://gerrit.wikimedia.org/r/246973

This was assigned CVE-2015-8003, with the caveat:

Use CVE-2015-8003. An important note here is that the MITRE CVE team
accepted this CVE request only because it came from the organization
that wrote the code. In the general case, adding completely new
functionality such as an upload rate limit is a security enhancement
and not eligible for a CVE ID.

Since MediaWiki generally considers uploads as an edit, and edits are throttled, I think the CVE here was appropriate.