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.

@Bawolff Could you review T185652#3922287?

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

In T185652#3929237, @Bawolff wrote:

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.

In T185652#3942604, @MarcoAurelio wrote:

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

In T185652#3946606, @Mainframe98 wrote:

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.

In T185652#3930297, @Mainframe98 wrote:

In T185652#3929237, @Bawolff wrote:

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:

Patch for T185652 - 21 KBDownload

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.

In T185652#3946620, @Bawolff wrote:

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.

bc3c9dab8896 displays correct authorship.

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

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

In T185652#3954614, @MarcoAurelio wrote:

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.

In T185652#3954509, @MarcoAurelio wrote:

@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

In T185652#3955332, @MarcoAurelio wrote:

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 (?)