I agree, mostly a browser bug. But,

any update ?

In T128209#2068032, @Anomie wrote:

I agree, mostly a browser bug. But,

SECURITY: API: Avoid some silliness with browser-guessed filenames.patch6 KBDownload

.

In certain configs, ORIG_PATH_INFO is actually the normal path to the script and not the "extra-path".

In particular this breaks on php 5.5.32 set up in cgi mode with a config like:

ScriptAlias /php-bin /path/to/my/phpbinary/cgi
AddHandler application/x-httpd-php5 php
Action application/x-httpd-php5 /php-bin/php-cgi
<Directory "/path/to/my/phpbinary/cgi">
    Order allow,deny
    Allow from all
</Directory>

Other than the ORIG_PATH_INFO stuff, this patch looks good.

One thing about this patch that gave me slight pause - is it really a good idea to suggest .php (for format=php) and .js (for format=json&callback=foo) as the extension, given php and js are executable format. But given the context, I guess that makes sense - almost no users are going to have php interperters, and even if they did, its not like the output of the api is going to be malicious. So I think that is actually fine.

Will those certain configs also break WebRequest? Or is that why we have $wgUsePathInfo?

Anyway, here's a version that doesn't check ORIG_PATH_INFO:

Is it fixed yet ?

In T128209#2701758, @Tod97 wrote:

Is it fixed yet ?

Sorry, not yet. It kind of got put at the bottom of the pile and slightly forgotten about due to some other more severe bugs.

Hey ,
any update ?

@Bawolff ?

Hi. Sorry.

Anyway, here's a version that doesn't check ORIG_PATH_INFO:

0001-SECURITY-API-Avoid-some-silliness-with-browser-guess.patch6 KBDownload

+1 to this patch.

Some notes about this issue:

• Requires link to be not-cross origin to work (except chrome, but this will probably change in chrome 61 - https://bugs.chromium.org/p/chromium/issues/detail?id=714373), which makes it much harder to exploit.
• Behaviour of firefox and chrome seem to both mildly differ from the standard (https://html.spec.whatwg.org/#downloading-resources), and most online docs.
  • The content-disposition: inline seems to totally override the download attribute in firefox causing firefox to not download the file.
  • Google chrome seems to totally ignore the suggested filename if content-disposition is not attachment (Hence this patch does not work for same-origin download attribute on chrome). But using content-disposition: attachment would be a pita.
    • But chrome will not use suggested extension when cross-origin, and it warns me about dangerous extensions like .sh when same origin.

So overall the patch is probably worth it, even if not 100% perfect coverage for chrome in that one case.

@Tod97 We will test this patch on Wikipedia, and if everything works, we will include it in the Next security release of MediaWiki (1.28.3 & 1.27.4) and make this bug public when 1.29.2 is released. I'm not sure what the schedule for the next security release is, but probably about 1.5 months.

@Bawolff Thanks for replay . Please add my name "Abdullah Hussam [@Abdulahhusam] " to the thanks page [https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Thanks] and notice me when the CVE will be assigned .

BR

We are now testing the patch on wikipedia.

We are getting some false positives for this patch due to things like: https://en.wikipedia.org/w/api.php/?format=json&formatversion=2&action=query&titles=Abraham+Lincoln&prop=revisions|images|info&rvprop=content|parsetree&imlimit=500&inprop=url ( https://logstash.wikimedia.org/goto/2dd875f4e42c7297d3320cc2a81c7ca6 )

We should perhaps allow a simple trailing / since that's safe. Or redirect.

[08:50] _joe_ bawolff: I'd issue a 301 to the correct url
[08:50] _joe_ there is no point in allowing that

ATM the patch above issues a 500, which among other things shows up in various ops dashboards. I think in this case we should return a 400 instead since there's nothing we can do

Proposed change, to use 301 instead:

[The above patch is based on top of the other one].

Combined version of the patch:

In T128209#3381573, @Bawolff wrote:

Proposed change, to use 301 instead:

Change 500 to 3011 KBDownload

[The above patch is based on top of the other one].

Combined version of the patch:

T128209-v2-combined.patch6 KBDownload

Godog +1'd this patch on irc, and I deployed the new version

I also just now sent a message to mediawiki-api (https://lists.wikimedia.org/pipermail/mediawiki-api/2017-June/004002.html ) . Maybe that should go to mediawiki-api-announce, but i don't think i have post rights there. In retrospect, maybe I should have tried to give people more warning beforehand (?)

@Bawolff Hi , I am gonna deactivate the notifications of this . And Just want to make sure that I will get the credit I really waited too long for the patch ( more than a year ) . So please confirm the credit so I can go on my way .

Thanks

In T128209#3381727, @Tod97 wrote:

@Bawolff Hi , I am gonna deactivate the notifications of this . And Just want to make sure that I will get the credit I really waited too long for the patch ( more than a year ) . So please confirm the credit so I can go on my way .

Thanks

Yes, ill make sure you are properly credited. However we are only going to make this public when the next version of MediaWiki comes out, which may be in a month or 2.

Hmm. there may be a similar issue in load.php

Regarding the 301, has anyone checked the browsers involved to make sure they use the redirected-to URL when determining the filename rather than the original URL? Ideally the Content-Disposition header will still override it, but better to fix it both ways.

Other disadvantages of a 301 that don't necessarily block using it:

• Some clients have historically turned a 301ed POST into a GET on the target URL. If someone is using a library with that misbehavior and doesn't notice the followed redirect, they may be confused when their request doesn't work right due to lost POST data.
• If it's not failing people won't fix it.

Regarding the 301, has anyone checked the browsers involved to make sure they use the redirected-to URL when determining the filename rather than the original URL? Ideally the Content-Disposition header will still override it, but better to fix it both ways.

Yes i did that. In the 301 case, it seems like either the content-disposition header rules for filename or the download behaviour is ignored and its displayed inline. (Checked firefox, chrome, and safari)

any update for the patch? @Bawolff

In T128209#3381573, @Bawolff wrote:

Proposed change, to use 301 instead:

Change 500 to 3011 KBDownload

Re-reviewing this patch, I note that your 301 is going to move the POST data into the GET part of the URL. You'd need to use $wgRequest->getQueryValues() instead. Although personally I'd still just change the 500 to a 400 and let people fix their broken code.

In T128209#3667760, @Tod97 wrote:

any update for the patch? @Bawolff

This is considered not serious enough to trigger a new version of mediawiki on its own, so its going to be included in the next security release of mediawiki (whenever that is). Its been a while since last security release, so a new one will probably happen sometime soonish, but I'm not exactly sure when.

In T128209#3673511, @Anomie wrote:

Re-reviewing this patch, I note that your 301 is going to move the POST data into the GET part of the URL. You'd need to use $wgRequest->getQueryValues() instead. Although personally I'd still just change the 500 to a 400 and let people fix their broken code.

I guess this patch needs some followup work?

This is CVE-2017-8809

Change 391411 had a related patch set uploaded (by Ejegg; owner: Anomie):
[mediawiki/core@fundraising/REL1_27] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391419 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_30] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391419 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391411 merged by jenkins-bot:
[mediawiki/core@fundraising/REL1_27] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391449 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@master] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391449 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391714 had a related patch set uploaded (by Chad; owner: Anomie):
[mediawiki/core@wmf/1.31.0-wmf.8] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

Change 391714 merged by jenkins-bot:
[mediawiki/core@wmf/1.31.0-wmf.8] SECURITY: API: Avoid some silliness with browser-guessed filenames

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

The redirect is being complained about in T215857: api.php PATH_INFO check infinite recursion, FYI.