Page MenuHomePhabricator

AutoProxyBlock uses unserialization on externally obtained php code
Closed, ResolvedPublic

Description

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

Event Timeline

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

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

Done in:

@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?

We should also backport this fix to past REL1_XX branches once merged in master.

@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.

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.

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

Done in:

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.

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".

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Feb 8 2018, 12:57 AM

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

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

Change 408959 merged by Brian Wolff:
[mediawiki/extensions/AutoProxyBlock@REL1_27] SECURITY: Don't query API with format=php

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

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".

I did 1.27 manually.

@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.

Nah, this is perfectly alright, thanks for uploading!

Can we cherry-pick to REL1_28 as well?

It’s not letting me do it, I’m given the same error message you’ve gotten.

I guess we'll have to go git cherry-pick, etc.? Umh, unexplored lands for me here :)

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

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

@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

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

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"

Maybe you needed to run git pull first (?)