| BlankEclair | |
| Nov 13 2024, 1:14 PM |
| F57697670: T379749.patch | |
| Nov 13 2024, 1:16 PM |
I did a spontaneous security review of the extension for Miraheze (https://issue-tracker.miraheze.org/T12870), and here's what I found:
I gave up fully documenting the security vulnerabilities once I stumbled upon the CSRF, so I'll just submit a solitary patch instead.
Without testing, much of this patch seems reasonable to me. ext:DataTransfer isn't hosted in Wikimedia production so once any immediately-affected systems are patched, it should go through gerrit for formal code review. We can also mention it in the next supplemental security release.
@BlankEclair - thank you for this comprehensive review. I'm adding these fixes in in parts - here is the first one:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1090913
You do realize that you can apply patches and commit them with git am -3 /tmp/T379749.patch, right?
And that you can amend commits with the first two examples on https://git-rebase.io?
Yes, but I prefer not to have these kinds of omnibus patches, where it's harder later to figure out what exactly changed. (Also, I want to do a few things differently than you had it in this patch.) I have no problem with you submitting everything in one big patch, though - whatever is easiest for you is good.
Sorry, yes - I wasn't finished, though I did get distracted by other tasks. Anyway, here are two more changes, based on your patch - there might be 3-4 more after this:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1093924
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1093931
Redoing my security issue, everything looks good now. If you could merge it into REL1_42, then that'd be nice.
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/DataTransfer/+/d837cc5c427f38a1aa01df6272e62f1f085fa1f9/includes/specials/DT_ImportCSV.php#34 doesn't check if it's a POST request or not, but technically not a security issue.
What about the changes from getUser()->isAllowed() to getAuthority()->isDefinitelyAllowed() - aren't those still needed?
@BlankEclair - could you explain why this patch replaces this code:
- throw new PermissionsError( 'datatransferimport' );
...with this?
+ $out->prepareErrorPage(); + $out->setPageTitleMsg( $this->msg( 'permissionserrors' ) ); + $out->addWikiTextAsInterface( Html::errorBox( + $out->formatPermissionStatus( $status, 'datatransferimport' ) + ) );
Is it considered better to display an error directly, rather than throwing the error? Or is this somehow security-related also?
I stole it from MediaWiki core: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/6c1bd1173b46168288293b730bc168a77114b899/includes/specials/SpecialImport.php#83
I don't think it'll be much different if you throw a PermissionsError though
Alright, thanks. For the sake of simplicity, I'm leaving that part alone for now. I have checked in two more changes, based on your patch:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1094482
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1098528
As far as I know, the security issues you pointed out in Data Transfer have now all been fixed. Of course, please let me know if there are any more issues.
This is unhelpful for back porting. It means in this case 7 times the work and 7 times the things to forget when back porting.
Can you please back port the relevant changes to supported release branches?
Well, to be fair, all of these changes are helpful, but I wouldn't say many of them are critical - it's great to check whether an admin is blocked, for example, but I don't think there's a big danger of blocked administrators maliciously importing a CSV file worth of data into the wiki. It seems to me that the fixes that could be described as critical are:
Also in my defense, I don't recommend that anyone use the REL_ branch for Data Transfer (or any of my other extensions) - the master branch supports MW versions going back to 1.37, which I think is good enough.
That said, if it really is just these two fixes, and just a few branches, I'm happy to do some back porting. What are the important branches?
As far as I know, the security issues you pointed out in Data Transfer have now all been fixed. Of course, please let me know if there are any more issues.
The user check in the import job is not implemented
Oh, sorry, I missed that one. It's now checked in here:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1099719
@RhinosF1 - I believe this, too, counts as a "nice to have" rather than critical fix, for the purposes of backporting.
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/DataTransfer/+/a8defaff6aea15d09eca5287ec9dd47382e39ba3/includes/specials/DT_ImportCSV.php#41:
Missing POST check ($this->getRequest()->wasPosted())
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/DataTransfer/+/a8defaff6aea15d09eca5287ec9dd47382e39ba3/includes/specials/DT_ImportCSV.php#76:
Missing htmlescapechars( $error_msg, ENT_QUOTES )
@BlankEclair - sorry I missed those! Here are my new fixes:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1101914
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1101923
Thanks! I just checked in this fix here:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DataTransfer/+/1102317
I do think this approach is a little better, because it allows for more flexibility, like parsing of error messages if needed.
@mmartorana - thanks for copying these patches over to the REL1_41 and REL1_42 branches. For some reason the REL1_41 patches are not going through, though - maybe this branch is already locked or something?
jenkins-bot/CI might not be configured for the REL1_41 branch for this repo? Manual CR/Verfiy + Submit should work though.
Okay, I decided to "live dangerously" and verified and submitted the two REL1_41 patches manually.