Page MenuHomePhabricator

Action api should reject requests with unsupported http methods with a 405
Closed, ResolvedPublic

Description

In the recent pentest, it was pointed out as a low severity issue that the action api can suffer from "verb tampering"

Verb tampering ( https://web.archive.org/web/20170517030540/http://cdn2.hubspot.net/hub/315719/file-1344244110-pdf/download-files/Bypassing_VBAAC_with_HTTP_Verb_Tampering.pdf ) is when you restrict access to an endpoint via a WAF rule or other access control means, but only restrict a specific blacklist of http methods. The attacker evades the access control by using an HTTP method like DELETE, which our api would treat like a GET.

I don't really think this is in our threat model. But I think the fact that DELETE and PUT act like GET is potentially confusing to users. To that end, I would like to propose the action api responds with a 405 status code for any http method other then GET, HEAD, POST or OPTIONS

Instructions on how to complete task (for gci students)

The goal of this task is to respond with a 405 status code, when people use unsupported HTTP methods with the ActionAPI.

For example, the following command:

curl -X DELETE 'https://en.wikipedia.org/w/api.php?action=query&prop=info&titles=Main_Page&format=json'

responds as if the user specified GET instead of DELETE. This is confusing.

To change this:

  • Open up includes/api/ApiMain.php in the MediaWiki core code repository. Locate the setupExternalResponse method.
  • Near where the check is for POST, add another check. This check should check if the method is one of GET, HEAD, POST or OPTIONS (perhaps using an if ( !in_array(... check). See also the documentation.
  • In the event the method is one other than the allowed method, you should call $this->dieWithError(...). The fourth argument should be 405 to return a 405 HTTP status code
  • Add the message (starting with apierror-) you used in the call to dieWithError() to i18n/en.json and i18n/qqq.json. Do not worry about other translations, they are handled by translatewiki.
  • Test your change to make sure it works properly.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Anomie removed a project: Internet-Archive.

It wouldn't hurt to return an error for unexpected verbs, probably somewhere near the check for POST being required.

I'm a bit on the fence over whether it should return 405 as proposed or if it should match how the API currently handles when GET is used with an action that requires a POST. Leaning towards the former though.

It wouldn't hurt to return an error for unexpected verbs, probably somewhere near the check for POST being required.

I'm a bit on the fence over whether it should return 405 as proposed or if it should match how the API currently handles when GET is used with an action that requires a POST. Leaning towards the former though.

Ok, sounds good. I was leaning to the 405 as this seems more like a protocol level error than an application level error, but I agree its not clear cut which would be the best approach.

This task like a good GCI task, so I intend to turn it into one

Change 474448 had a related patch set uploaded (by Shreyasminocha; owner: Shreyasminocha):
[mediawiki/core@master] Send a 405 on unsupported HTTP methods in API

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

Change 474448 merged by jenkins-bot:
[mediawiki/core@master] Send a 405 on unsupported HTTP methods in API

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