While attempting to code review for T185384, I discovered that this extension obtains proxy information from Wikimedia using an API call. Unfortunately, this is using the return type PHP, and it is passed directly to unserialize. See Line 121 of AutoProxyBlock.body.php.
I'm pretty sure I don't have to explain why this is a security issue. For details, see the PHP article on unserialize: http://php.net/manual/en/function.unserialize.php
| Status | Subtype | Assigned | Task | |
|---|---|---|---|---|
| · · · | ||||
| Resolved | MarcoAurelio | T185384 Convert AutoProxyBlock to use extension registration | ||
| Resolved | Mainframe98 | T185652 AutoProxyBlock uses unserialization on externally obtained php code | ||
| · · · |
More details regarding PHP serialization in MediaWiki can be found on T161647: RFC: Deprecate using php serialization inside MediaWiki.
Nice find :) Since this is hitting Wikipedia it's practically safe, but we should change it to JSON because there's no reason to use PHP serialization. Also the default URL is not using HTTPS, so someone could try injecting malicious code that way.
I've asked @MarcoAurelio to change the default url to use https, so that should be addressed in T185384. I was also thinking of using JSON. I'll see if I can whip up a diff.
I think I'll abandon the conversion patch until the extension is fixed. I have doubts as to if what I did is right or not, and I don't want to mess things.
It shouldn’t be necessary to abandon the patch, although eyeing the changes in that patch it might be better to merge the security fix first before converting to extension registration.
Yes I agree with merging your patch fixing this issue and after that converting the extension. I might need to run the conversion script again, etc. Thanks.
Patch for review by @Mainframe98 uploaded as a comment on this task. Somebody from Security please review.
Yes, this patch looks correct, and fixes the unserialization issue
However, for best security, one should also urlencode stuff when building the url. e.g. on line 117, instead of manually turning the array to a url, it should use wfArrayToCgi
@Bawolff If the patch above by @Mainframe98 is right now, should he just upload it to gerrit and get it quickly merged or follow a different step?
Yes, thats fine. Once patch is merged and backported to supported releases an email to mediawiki-l should be sent announcing the security fix.
How do I get it merged quickly? All https://www.mediawiki.org/wiki/Reporting_security_bugs tells me is that I shouldn't upload it to Gerrit.
That's more about Wikimedia deployed extensions. Ideally you'd get someone with +2 to merge immediately.
In any case, I can upload the patch if that's easier.
Taking a closer look at the patch, shouldn't this supply a second argument of true to json_decode to force it into associative mode? The rest of the code seems to treat the result as an associative array.
Correct, fixed. If you're available on IRC, can I use that to signal you when I've uploaded the patch on Gerrit? (Assuming you've got +2 rights to that repo) I'm available right now for the next hour or so. I'll add the improved patch version here regardless.
@Mainframe98 I've uploaded your patch at https://gerrit.wikimedia.org/r/#/c/408941/ and @Bawolff merged it. I think it respects your authorship as you're listed as the author, but I'm listed as the "owner" (I guess it's because the patch was uploaded using my ssh key). Let me know if we should revert/etc. I don't want to take credit for things I didn't do.
Cherry-picks to REL1_30 and REL1_29 now merged as well. I couldn't cherry-pick to REL1_28 nor REL1_27 using the Gerrit GUI due to a "merge conflict".
Change 408959 had a related patch set uploaded (by Brian Wolff; owner: Mainframe98):
[mediawiki/extensions/AutoProxyBlock@REL1_27] SECURITY: Don't query API with format=php
Change 408959 merged by Brian Wolff:
[mediawiki/extensions/AutoProxyBlock@REL1_27] SECURITY: Don't query API with format=php
Change 409006 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/extensions/AutoProxyBlock@REL1_28] SECURITY: Don't query API with format=php
@gerritbot seems to have caught the cherry-pick, so I assume that it went fine. Apparently, git got confused about a function named onRecentChangeSave. The good news is that the docs on cherry-picking are still accurate.
Thanks! I was running into a lot of:
Cannot find symbolic reference
The following command failed with exit code 1
"git symbolic-ref -q HEAD"Change 409006 merged by jenkins-bot:
[mediawiki/extensions/AutoProxyBlock@REL1_28] SECURITY: Don't query API with format=php