Change Details

During today (2022-09-28) UTC afternoon B&C backport window, @taavi tried to run `extensions/DiscussionTools/maintenance/persistRevisionThreadItems.php` for testwiki. ```name=Output taavi@mwmaint1002:~$ time mwscript extensions/DiscussionTools/maintenance/persistRevisionThreadItems.php --wiki=testwiki --current --all | tee persistRevisionThreadItems.log Bad MediaWiki install path: /srv/mediawiki/php-1.39.0-wmf.26 ``` This is caused by [a check in the script itself](https://gerrit.wikimedia.org/g/mediawiki/extensions/DiscussionTools/+/6f5bcf2c09b46b83efef799fb7c70cc471802618/maintenance/persistRevisionThreadItems.php#24), which requires that `MW_INSTALL_PATH` may not contain dots. According to @Matmarex, this was added following #security-team's recommendation (discussion is at T242134, [patch](https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051)). The check makes it impossible to run the script in Wikimedia production, because the installation patch in WMF production is `/srv/mediawiki/php-1.39.0-wmf.26` (and includes a couple of dots). I have two possible solutions here: # Remove the mitigation entirely. Personally, I don't think it serves any meaningful purpose. If an attacker is in a position where they can control the content of `MW_INSTALL_PATH` (to get the two dots there in the first place), they are //also// very likely in a position where they can set the variable to an arbitrary string, rendering the protection ineffective. In theory, dots can be abused if an attacker has append-only write access to `MW_INSTALL_PATH` (instead of having the ability to point the variable to subdirs of what it was before, they'd have the ability to point it to an arbitrary directory). The second prohibited character, `~`, has no special meaning in PHP at all (tilde expansion is a shell feature, and even there it is only valid at the start of the path => an attacker would have to be able to set `MW_INSTALL_PATH` to an arbitrary path already anyway). # Replace the dot with two dots, as two dots is the special-meaning directory we're apparently concerned about here. Regardless of the solution we decide to go with, it should be unified across all extensions (or at least the ones we have deployed on our own servers).

During today (2022-09-28) UTC afternoon B&C backport window, @taavi tried to run `extensions/DiscussionTools/maintenance/persistRevisionThreadItems.php` for testwiki. ```name=Output taavi@mwmaint1002:~$ time mwscript extensions/DiscussionTools/maintenance/persistRevisionThreadItems.php --wiki=testwiki --current --all | tee persistRevisionThreadItems.log Bad MediaWiki install path: /srv/mediawiki/php-1.39.0-wmf.26 ``` This is caused by [a check in the script itself](https://gerrit.wikimedia.org/g/mediawiki/extensions/DiscussionTools/+/6f5bcf2c09b46b83efef799fb7c70cc471802618/maintenance/persistRevisionThreadItems.php#24), which requires that `MW_INSTALL_PATH` may not contain dots. According to @Matmarex, this was added following #security-team's recommendation (discussion is at T242134, [patch](https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/573051)). The check makes it impossible to run the script in Wikimedia production, because the installation patch in WMF production is `/srv/mediawiki/php-1.39.0-wmf.26` (and includes a couple of dots). I have two possible solutions here: # Remove the mitigation entirely. Personally, I don't think it serves any meaningful purpose. If an attacker is in a position where hethey can control the content of `MW_INSTALL_PATH` (to get the two dots there in the first place), they are //also// very likely in a position where they can set the variable to an arbitrary string, rendering the protection ineffective. In theory, dots can be abused if an attacker has append-only write access to `MW_INSTALL_PATH` (instead of having the ability to point the variable to subdirs of what it was before, they'd have the ability to point it to an arbitrary directory). The second prohibited character, `~`, has no special meaning in PHP at all (tilde expansion is a shell feature, rendering the protection ineffectiveand even there it is only valid at the start of the path => an attacker would have to be able to set `MW_INSTALL_PATH` to an arbitrary path already anyway). # Replace the dot with two dots, as two dots is the special-meaning directory we're apparently concerned about here. Regardless of the solution we decide to go with, it should be unified across all extensions (or at least the ones we have deployed on our own servers).