Page MenuHomePhabricator
Create Task
Maniphest T242689

SocialProfile: classic CSRF (no token check) in various special pages which perform write actions
Closed, ResolvedPublicSecurity
Actions

Description

A "fun" little thing I noticed while implementing T227345: Actor support for social tools for SocialProfile: a bunch of special pages which allow the user to perform database-altering write actions correctly check that the request was POSTed but totally lack a token check, making them vulnerable to CSRF.

Affected special pages (and their subcomponents in the /extensions/SocialProfile dir):

  • Special:PopulateAwards (SystemGifts)
    • This is just scary. The special page should probably implement a "Are you sure you wanna do this?" screen (which would contain the form containing the anti-CSRF token) instead of running the operation right away.
  • Special:RemoveMasterSystemGift (SystemGifts)
  • Special:SystemGiftManager (SystemGifts)
  • Special:SystemGiftManagerLogo (SystemGifts)
  • Special:SendBoardBlast (UserBoard)
  • Special:GiftManager (UserGifts)
  • Special:GiftManagerLogo (UserGifts)
  • Special:GiveGift (UserGifts)
  • Special:RemoveGift (UserGifts)
  • Special:RemoveMasterGift (UserGifts)
  • Special:AddRelationship (UserRelationship)
  • Special:RemoveRelationship (UserRelationship)
  • Special:ViewRelationshipRequests (UserRelationship) or rather, the backend code (API module, /extensions/SocialProfile/UserRelationship/includes/api/ApiRelationshipResponse.php and the JS code calling it in /extensions/SocialProfile/UserRelationship/resources/js/UserRelationship.js -- JS should use ( new mw.Api() ).postWithToken( 'csrf', { ... } ); like how VoteNY and such tools use to implement appropriate anti-CSRF measures)
  • Special:EditProfile (UserProfile)
  • Special:PopulateExistingUsersProfiles (UserProfile)
    • Literally equally scary and problematic as SystemGifts' Special:PopulateAwards page and needs similar changes
  • Special:RemoveAvatar (UserProfile)
    • This uses OOUI and thus implements CSRF token checking properly already.
  • Special:ToggleUserPage
  • Special:UpdateProfile (UserProfile)
  • Special:GenerateTopUsersReport (UserStats)
    • It's just like Special:PopulateAwards or Special:PopulateExistingUsersProfiles
  • Special:UpdateEditCounts
    • Scary like Special:PopulateAwards, Special:PopulateExistingUsersProfiles & Special:PopulateAwards and could use a confirmation screen

I didn't check the uploading special pages (italicized above) too closely, but they probably are also vulnerable as Special:Upload (from which those are derived, originally anyway) started requiring the CSRF token around MW 1.18/1.19; Special:UploadAvatar, the user-oriented special page for uploading an avatar, does implement anti-CSRF measures properly.

I will start working on this once the work on T227345 is complete as the changes needed in SocialProfile for that task are best described as "massive", whereas this task should be relatively straightforward and easy to implement (much like T241735).

Details

Author Affiliation
Other (Please specify in description)
Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
[SECURITY] Add token checks to prevent CSRF to various places which do write actionsJack Phoenixmediawiki/extensions/SocialProfilemaster+551 -95
Customize query in gerrit

Related Objects

Event Timeline

ashley created this task.Jan 13 2020, 11:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 13 2020, 11:47 PM
ashley claimed this task.Jan 13 2020, 11:48 PM
ashley added projects: SocialProfile, Vuln-CSRF.
Restricted Application added a project: Social-Tools. · View Herald TranscriptJan 13 2020, 11:48 PM
ashley moved this task from Backlog to SocialProfile on the Social-Tools board.Jan 13 2020, 11:51 PM
• chasemp moved this task from Incoming to Watching on the Security-Team board.Jan 14 2020, 6:01 PM
ashley updated the task description. (Show Details)Jan 29 2020, 10:27 AM
ashley added a comment.Jan 31 2020, 3:20 PM

SP-CSRF.patch44 KBDownload

ApiUserProfileType.php3 KBDownload

Proposed patch(es), somewhat tested. The new API module and the related shuffling of the JS code loading in UserProfilePage fix T202272, hopefully.

ashley added a comment.Jan 31 2020, 3:33 PM

SP-CSRF.patch49 KBDownload

New version of the main patch which also properly adds a confirmation form + token checking to Special:ToggleUserPage.

• chasemp added a project: Security.Feb 10 2020, 10:50 PM
ashley added a subscriber: Bawolff.Feb 17 2020, 11:46 AM

cc'ing @Bawolff for some thoughts as I'd like to get this merged during February.

Bawolff added a comment.Feb 17 2020, 12:00 PM
@@ -146,6 +150,7 @@ class RemoveMasterSystemGift extends UnlistedSpecialPage {
 				<input type="button" class="site-button" value="' . $this->msg( 'ga-remove' )->plain() . '" size="20" onclick="document.form1.submit()" />
 				<input type="button" class="site-button" value="' . $this->msg( 'cancel' )->plain() . '" size="20" onclick="history.go(-1)" />
 			</div>
+			<input type="hidden" name="wpEditToken" value="' . $this->getUser()->getEditToken() . '" />
 		</form>';

Best practice would be to htmlescape the token, although in reality it doesn't really matter, and there are other much worse things in that extension.

Ideally it would be using a libarary like HtmlForm to take care of CSRF tokens for it. But that would be a rather big change.

I did not test any of this, and I didn't look through the rest of the extension to see if there was anything missed, but generally this patch looks to be the right approach. So consider this a +1 :)

ashley mentioned this in rESPR824bc59fe9b0: [SECURITY] Add token checks to prevent CSRF to various places which do write….Feb 20 2020, 3:03 PM
ashley closed this task as Resolved.Feb 20 2020, 3:14 PM
ashley added a subscriber: Legoktm.

Now fixed in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/SocialProfile/+/573580/ , thanks to @Legoktm for the +2. :)

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Feb 20 2020, 3:15 PM
Legoktm changed the edit policy from "Custom Policy" to "All Users".
• chasemp removed a project: acl*security.Feb 20 2020, 8:11 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits