From d8eaae539c9b6d5233f71f0b8719a8b2adc42e4f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Thu, 1 Nov 2018 16:29:22 -0700 Subject: [PATCH] Separate right for foreign user js redirects Require a new editmyuserjsredirect permission for users to edit Javascript redirects in their userspace when the redirect target is not in their userspace (unless they have edituserjs and can edit any user JS anyway). This is to prevent attacks where a popular userscript has been moved into the system namespace or another safe location but many users still load it through the original userspace redirect, and the attacker manages to take over the userspace by compromising the account or getting it renamed. Since this is only a concern on large community wikis, by default all users have the editmyuserjsredirect permission. Bug: T207750 Change-Id: I36a879d5da04cb6f49ed1bc40dbe144f6862c6a1 Depends-On: I072cf857c1fff4578402904aa9cb5a0c8833f16f --- RELEASE-NOTES-1.34 | 3 + includes/DefaultSettings.php | 1 + includes/Permissions/PermissionManager.php | 23 ++++ includes/ServiceWiring.php | 1 + languages/i18n/en.json | 3 + languages/i18n/qqq.json | 5 +- .../Permissions/PermissionManagerTest.php | 107 ++++++++++++++++-- 7 files changed, 135 insertions(+), 8 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 60d5d96a7e..3deeaab6d6 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -36,6 +36,9 @@ For notes on 1.33.x and older releases, see HISTORY. * $wgEnableSpecialMute (T218265) - This configuration controls whether Special:Mute is available and whether to include a link to it on emails originating from Special:Email. +* editmyuserjsredirect user right – users without this right now cannot edit JS + redirects in their userspace unless the target of the redirect is also in + their userspace. By default, this right is given to everyone. ==== Changed configuration ==== * $wgUseCdn, $wgCdnServers, $wgCdnServersNoPurge, and $wgCdnMaxAge – These four diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 3bfc8f8241..107c546724 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -5173,6 +5173,7 @@ $wgGroupPermissions['user']['minoredit'] = true; $wgGroupPermissions['user']['editmyusercss'] = true; $wgGroupPermissions['user']['editmyuserjson'] = true; $wgGroupPermissions['user']['editmyuserjs'] = true; +$wgGroupPermissions['user']['editmyuserjsredirect'] = true; $wgGroupPermissions['user']['purge'] = true; $wgGroupPermissions['user']['sendemail'] = true; $wgGroupPermissions['user']['applychangetags'] = true; diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index 98a5b17608..483c1718f7 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -23,6 +23,8 @@ use Action; use Exception; use Hooks; use MediaWiki\Linker\LinkTarget; +use MediaWiki\Revision\RevisionLookup; +use MediaWiki\Revision\RevisionRecord; use MediaWiki\Session\SessionManager; use MediaWiki\Special\SpecialPageFactory; use MediaWiki\User\UserIdentity; @@ -55,6 +57,9 @@ class PermissionManager { /** @var SpecialPageFactory */ private $specialPageFactory; + /** @var RevisionLookup */ + private $revisionLookup; + /** @var string[] List of pages names anonymous user may see */ private $whitelistRead; @@ -130,6 +135,7 @@ class PermissionManager { 'editmyusercss', 'editmyuserjson', 'editmyuserjs', + 'editmyuserjsredirect', 'editmywatchlist', 'editsemiprotected', 'editsitecss', @@ -184,6 +190,7 @@ class PermissionManager { /** * @param SpecialPageFactory $specialPageFactory + * @param RevisionLookup $revisionLookup * @param string[] $whitelistRead * @param string[] $whitelistReadRegexp * @param bool $emailConfirmToEdit @@ -195,6 +202,7 @@ class PermissionManager { */ public function __construct( SpecialPageFactory $specialPageFactory, + RevisionLookup $revisionLookup, $whitelistRead, $whitelistReadRegexp, $emailConfirmToEdit, @@ -205,6 +213,7 @@ class PermissionManager { NamespaceInfo $nsInfo ) { $this->specialPageFactory = $specialPageFactory; + $this->revisionLookup = $revisionLookup; $this->whitelistRead = $whitelistRead; $this->whitelistReadRegexp = $whitelistReadRegexp; $this->emailConfirmToEdit = $emailConfirmToEdit; @@ -1134,6 +1143,20 @@ class PermissionManager { && !$user->isAllowedAny( 'editmyuserjs', 'edituserjs' ) ) { $errors[] = [ 'mycustomjsprotected', $action ]; + } elseif ( + $page->isUserJsConfigPage() + && !$user->isAllowedAny( 'edituserjs', 'editmyuserjsredirect' ) + ) { + // T207750 - do not allow users to edit a redirect if they couldn't edit the target + $rev = $this->revisionLookup->getRevisionByTitle( $page ); + $content = $rev ? $rev->getContent( 'main', RevisionRecord::RAW ) : null; + $target = $content ? $content->getUltimateRedirectTarget() : null; + if ( $target && ( + !$target->inNamespace( NS_USER ) + || !preg_match( '/^' . preg_quote( $user->getName(), '/' ) . '\//', $target->getText() ) + ) ) { + $errors[] = [ 'mycustomjsredirectprotected', $action ]; + } } } else { // Users need editmyuser* to edit their own CSS/JSON/JS subpages, except for diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 7d2b3cb14f..8f9044e5ba 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -466,6 +466,7 @@ return [ $config = $services->getMainConfig(); return new PermissionManager( $services->getSpecialPageFactory(), + $services->getRevisionLookup(), $config->get( 'WhitelistRead' ), $config->get( 'WhitelistReadRegexp' ), $config->get( 'EmailConfirmToEdit' ), diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 0f401213bc..272b97d808 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -1273,6 +1273,7 @@ "right-editmyusercss": "Edit your own user CSS files", "right-editmyuserjson": "Edit your own user JSON files", "right-editmyuserjs": "Edit your own user JavaScript files", + "right-editmyuserjsredirect": "Edit your own user JavaScript files that are redirects", "right-viewmywatchlist": "View your own watchlist", "right-editmywatchlist": "Edit your own watchlist. Note some actions will still add pages even without this right.", "right-viewmyprivateinfo": "View your own private data (e.g. email address, real name)", @@ -1403,6 +1404,7 @@ "action-editmyusercss": "edit your own user CSS files", "action-editmyuserjson": "edit your own user JSON files", "action-editmyuserjs": "edit your own user JavaScript files", + "action-editmyuserjsredirect": "edit your own user JavaScript files that are redirects", "action-viewsuppressed": "view revisions hidden from any user", "action-hideuser": "block a username, hiding it from the public", "action-ipblock-exempt": "bypass IP blocks, auto-blocks and range blocks", @@ -4236,6 +4238,7 @@ "passwordpolicies-policy-passwordnotinlargeblacklist": "Password cannot be in the list of 100,000 most commonly used passwords.", "passwordpolicies-policyflag-forcechange": "must change on login", "passwordpolicies-policyflag-suggestchangeonlogin": "suggest change on login", + "mycustomjsredirectprotected": "You do not have permission to edit this JavaScript page because it is a redirect and it does not point inside your userspace.", "easydeflate-invaliddeflate": "Content provided is not properly deflated", "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage", "userlogout-continue": "Do you want to log out?" diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 965f1633b1..3032a550eb 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -1481,7 +1481,8 @@ "right-editsitejs": "{{doc-right|editsitejs}}", "right-editmyusercss": "{{doc-right|editmyusercss}}\nSee also:\n* {{msg-mw|Right-editusercss}}", "right-editmyuserjson": "{{doc-right|editmyuserjson}}\nSee also:\n* {{msg-mw|Right-edituserjson}}", - "right-editmyuserjs": "{{doc-right|editmyuserjs}}\nSee also:\n* {{msg-mw|Right-edituserjs}}", + "right-editmyuserjs": "{{doc-right|editmyuserjs}}\nSee also:\n* {{msg-mw|Right-edituserjs}}\n* {{msg-mw|Right-editmyuserjsredirect}}", + "right-editmyuserjsredirect": "{{doc-right|editmyuserjsredirect}}\nSame as {{msg-mw|Right-editmyuserjs}} except if page is a redirect.\n\nSee also:\n* {{msg-mw|Right-edituserjs}}", "right-viewmywatchlist": "{{doc-right|viewmywatchlist}}", "right-editmywatchlist": "{{doc-right|editmywatchlist}}", "right-viewmyprivateinfo": "{{doc-right|viewmyprivateinfo}}", @@ -1612,6 +1613,7 @@ "action-editmyusercss": "{{doc-action|editmyusercss}}", "action-editmyuserjson": "{{doc-action|editmyuserjson}}", "action-editmyuserjs": "{{doc-action|editmyuserjs}}", + "action-editmyuserjsredirect": "{{doc-action|editmyuserjsredirect}}", "action-viewsuppressed": "{{doc-action|viewsuppressed}}", "action-hideuser": "{{doc-action|hideuser}}", "action-ipblock-exempt": "{{doc-action|ipblock-exempt}}", @@ -4445,6 +4447,7 @@ "passwordpolicies-policy-passwordnotinlargeblacklist": "Password policy that enforces that a password is not in a list of 100,000 number of \"popular\" passwords.", "passwordpolicies-policyflag-forcechange": "Password policy flag that enforces changing invalid passwords on login.", "passwordpolicies-policyflag-suggestchangeonlogin": "Password policy flag that suggests changing invalid passwords on login.", + "mycustomjsredirectprotected": "Error message shown when user tries to edit their own JS page that is a foreign redirect without the 'mycustomjsredirectprotected' right. See also {{mw-msg|mycustomjsprotected}}.", "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly", "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected", "userlogout-continue": "Shown if user attempted to log out without a token specified. Probably the user clicked on an old link that hasn't been updated to use the new system. $1 - url that user should click on in order to log out." diff --git a/tests/phpunit/includes/Permissions/PermissionManagerTest.php b/tests/phpunit/includes/Permissions/PermissionManagerTest.php index 03b35b5331..61dcac4ac2 100644 --- a/tests/phpunit/includes/Permissions/PermissionManagerTest.php +++ b/tests/phpunit/includes/Permissions/PermissionManagerTest.php @@ -3,21 +3,25 @@ namespace MediaWiki\Tests\Permissions; use Action; +use ContentHandler; use FauxRequest; -use MediaWiki\Session\SessionId; -use MediaWiki\Session\TestUtils; -use MediaWikiLangTestCase; -use RequestContext; -use stdClass; -use Title; -use User; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\Restriction\NamespaceRestriction; use MediaWiki\Block\Restriction\PageRestriction; use MediaWiki\Block\SystemBlock; +use MediaWiki\Linker\LinkTarget; use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\PermissionManager; +use MediaWiki\Revision\MutableRevisionRecord; +use MediaWiki\Revision\RevisionLookup; use Wikimedia\ScopedCallback; +use MediaWiki\Session\SessionId; +use MediaWiki\Session\TestUtils; +use MediaWikiLangTestCase; +use RequestContext; +use stdClass; +use Title; +use User; use Wikimedia\TestingAccessWrapper; /** @@ -698,6 +702,64 @@ class PermissionManagerTest extends MediaWikiLangTestCase { ); } + public function testJsConfigRedirectEditPermissions() { + $revision = null; + $user = $this->getTestUser()->getUser(); + $otherUser = $this->getTestUser( 'sysop' )->getUser(); + $localJsTitle = Title::newFromText( 'User:' . $user->getName() . '/foo.js' ); + $otherLocalJsTitle = Title::newFromText( 'User:' . $user->getName() . '/foo2.js' ); + $nonlocalJsTitle = Title::newFromText( 'User:' . $otherUser->getName() . '/foo.js' ); + + $services = MediaWikiServices::getInstance(); + $revisionLookup = $this->getMockBuilder( RevisionLookup::class ) + ->setMethods( [ 'getRevisionByTitle' ] ) + ->getMockForAbstractClass(); + $revisionLookup->method( 'getRevisionByTitle' ) + ->willReturnCallback( function ( LinkTarget $page ) use ( + $services, &$revision, $localJsTitle + ) { + if ( $localJsTitle->equals( Title::newFromLinkTarget( $page ) ) ) { + return $revision; + } else { + return $services->getRevisionLookup()->getRevisionByTitle( $page ); + } + } ); + $permissionManager = new PermissionManager( + $services->getSpecialPageFactory(), + $revisionLookup, + [], + [], + false, + false, + [], + [], + [], + MediaWikiServices::getInstance()->getNamespaceInfo() + ); + $this->setService( 'PermissionManager', $permissionManager ); + + $permissionManager->overrideUserRightsForTesting( $user, [ 'edit', 'editmyuserjs' ] ); + + $revision = $this->getJavascriptRevision( $localJsTitle, $user, '/* script */' ); + $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle ); + $this->assertSame( [], $errors ); + + $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $otherLocalJsTitle, $user ); + $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle ); + $this->assertSame( [], $errors ); + + $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $nonlocalJsTitle, $user ); + $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle ); + $this->assertSame( [ [ 'mycustomjsredirectprotected', 'edit' ] ], $errors ); + + $permissionManager->overrideUserRightsForTesting( $user, + [ 'edit', 'editmyuserjs', 'editmyuserjsredirect' ] ); + + $revision = $this->getJavascriptRedirectRevision( $localJsTitle, $nonlocalJsTitle, $user ); + $errors = $permissionManager->getPermissionErrors( 'edit', $user, $localJsTitle ); + $this->assertSame( [], $errors ); + } + /** * @todo This test method should be split up into separate test methods and * data providers @@ -1684,4 +1746,35 @@ class PermissionManagerTest extends MediaWikiLangTestCase { $this->assertFalse( $permissionManager->userHasRight( $this->user, 'move' ) ); } + /** + * Create a RevisionRecord with a single Javascript main slot. + * @param Title $title + * @param User $user + * @param string $text + * @return MutableRevisionRecord + */ + private function getJavascriptRevision( Title $title, User $user, $text ) { + $content = ContentHandler::makeContent( $text, $title, CONTENT_MODEL_JAVASCRIPT ); + $revision = new MutableRevisionRecord( $title ); + $revision->setContent( 'main', $content ); + return $revision; + } + + /** + * Create a RevisionRecord with a single Javascript redirect main slot. + * @param Title $title + * @param Title $redirectTargetTitle + * @param User $user + * @return MutableRevisionRecord + */ + private function getJavascriptRedirectRevision( + Title $title, Title $redirectTargetTitle, User $user + ) { + $content = ContentHandler::getForModelID( CONTENT_MODEL_JAVASCRIPT ) + ->makeRedirectContent( $redirectTargetTitle ); + $revision = new MutableRevisionRecord( $title ); + $revision->setContent( 'main', $content ); + return $revision; + } + } -- 2.20.1