Page MenuHomePhabricator

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

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

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

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


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

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

@@ -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 :)

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