PollNY: Stored XSS (CVE-2020-29003)
Closed, ResolvedPublicSecurity
Actions

Assigned To

Authored By

	ashley
	Oct 26 2020, 8:43 PM

Description

Like T266400 but only for PollNY this time around...

Prerequisites: social tools setup (MW 1.34 with SocialProfile, and for this particular bug, also need PollNY)

Create a poll via Special:CreatePoll, or edit an existing one via Special:UpdatePoll
Have an answer option contain something like <script>alert('XSS')</script>
Save changes
When viewing the poll's page in the NS_POLL namespace, the malicious code gets executed despite that it damn well shouldn't

The last step is also true for polls embedded on other wiki pages via the pollembed parser tag.

Details

	Subject	Repo	Branch	Lines +/-
	[SECURITY] Fix stored XSS via poll choices on Poll: pages etc.	mediawiki/extensions/PollNY	REL1_35	+62 -19
	[SECURITY] Fix stored XSS via poll choices on Poll: pages etc.	mediawiki/extensions/PollNY	master	+62 -19

Customize query in gerrit

Related Objects

Mentioned In: T263810: Write and send supplementary release announcement for extensions and skins with security patches (1.31.11/1.35.1)
Mentioned Here: T248390: Better NoJS support
T266400: RandomGameUnit: Stored XSS (CVE-2020-27957)

Event Timeline

ashley created this task.Oct 26 2020, 8:43 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 26 2020, 8:43 PM

ashley claimed this task.Oct 26 2020, 8:44 PM

ashley added projects: Vuln-XSS, PollNY.

Restricted Application added a project: Social-Tools. · View Herald TranscriptOct 26 2020, 8:44 PM

ashley updated the task description. (Show Details)Oct 26 2020, 8:45 PM

PollNY-Stored-XSS-T266508.patch7 KBDownload

Proposed patch which fixes the issues noted here and includes some unrelated no-JS work (T248390); the relevant chunks are obviously the ones where htmlspecialchars is mentioned, except for the last one ("next poll" URL stuff), that's strictly no-JS related and not related to this ticket.

@@ -230,8 +230,9 @@ class PollNYHooks {
 
 					foreach ( $poll_info['choices'] as $choice ) {
 						$output .= "<div class=\"poll-choice\">
-						<input type=\"radio\" name=\"poll_choice\" data-poll-id=\"{$poll_info['id']}\" data-poll-page-id=\"{$poll_page_id}\" id=\"poll_choice\" value=\"{$choice['id']}\">{$choice['choice']}
-						</div>";
+						<input type=\"radio\" name=\"poll_choice\" data-poll-id=\"{$poll_info['id']}\" data-poll-page-id=\"{$poll_page_id}\" id=\"poll_choice\" value=\"{$choice['id']}\">";
+						$output .= htmlspecialchars( $choice['choice'], ENT_QUOTES );
+						$output .= '</div>';
 					}
 
 					$output .= Html::submitButton( wfMessage( 'poll-submit-btn' )->escaped(), [ 'class' => 'poll-vote-btn-nojs' ] );

Do you want to apply the same (int) casting for these ids just to make it obvious these are safe? Or switch to the Html wrapper which should escape attributes I believe?

Other stuff LGTM.

sbassett mentioned this in T263810: Write and send supplementary release announcement for extensions and skins with security patches (1.31.11/1.35.1).Oct 27 2020, 8:16 PM

new-stored-xss-patch.patch8 KBDownload

@Legoktm Thanks for the CR! While attempting to implement your changes, I found a few more nasty stored XSS lines in that same file...here's a new patch which hopefully catches 'em all, for good this time around.

Reedy removed a project: Security-Team.Nov 2 2020, 4:03 PM

cc'ing @Bawolff for thoughts

This looks like it fixes the XSS's properly, as far as that patch goes (I did not look at any of the other PollNY code beyond what was in the patch)

+1 to the patch at T266508#6583137. One thing I did notice was that in PollNY.hooks.php, the $choice['percent'] variable used to build the html string on line 263 isn't cast to an int as similar variables are within the patch. The cast likely isn't necessary, even though this does appear to be a database value as opposed to a computed value as the similar percent variables are within PollPage.class.php. Just pointing out for consistency's sake.

Legoktm closed this task as Resolved.Nov 16 2020, 6:35 PM

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".

Legoktm changed the edit policy from "Custom Policy" to "All Users".

sbassett renamed this task from PollNY: Stored XSS to PollNY: Stored XSS (CVE-2020-29003).Dec 1 2020, 5:47 PM

Change 651588 had a related patch set uploaded (by SBassett; owner: Jack Phoenix):
[mediawiki/extensions/PollNY@REL1_35] [SECURITY] Fix stored XSS via poll choices on Poll: pages etc.

https://gerrit.wikimedia.org/r/651588

gerritbot added a project: Patch-For-Review.Dec 22 2020, 9:20 PM

Change 651588 merged by jenkins-bot:
[mediawiki/extensions/PollNY@REL1_35] [SECURITY] Fix stored XSS via poll choices on Poll: pages etc.

https://gerrit.wikimedia.org/r/651588

Maintenance_bot removed a project: Patch-For-Review.Dec 22 2020, 10:10 PM