⚓ T357101 CVE-2024-34502: Special:MergeLexemes makes edits on GET requests without edit tokens
Page MenuHomePhabricator
Create Task
Maniphest T357101

CVE-2024-34502: Special:MergeLexemes makes edits on GET requests without edit tokens
Closed, ResolvedPublicSecurity
Actions

Assigned To
Lucas_Werkmeister_WMDE
Authored By
Lucas_Werkmeister_WMDE
Feb 9 2024, 10:02 AM
Tags
Referenced Files
F42387848: 0001-SECURITY-Check-edit-token-in-Special-MergeLexemes.patch
Mar 4 2024, 10:20 AM
F41826173: 0001-SECURITY-Check-edit-token-in-Special-MergeLexemes.patch
Feb 9 2024, 11:00 AM
Subscribers
Aklapper
gerritbot
jnuche
Lucas_Werkmeister_WMDE
Lydia_Pintscher
mmartorana
Mstyles
sbassett

Description

Loading Special:MergeLexemes?from-id=L12345&to-id=L12346 will (attempt to) merge L12345 into L12346, even if the request was not a POST request, and even if it doesn’t contain an edit token.

Special:MergeItems and Special:RedirectEntity don’t have this problem, by the way – the TokenCheckInteractor checks both that the request has a valid token and that it was POSTed. (Though I don’t think it checks whether the token was in the POST body or in the request URL. I’m not sure if that’s a problem.) Special:MergeLexemes should use this interactor as well (and add the necessary hidden input to the form, of course).

Details

Risk Rating
Low
Author Affiliation
Wikimedia Deutschland
SubjectRepoBranchLines +/-
Clean up edit token check in SpecialMergeLexemesmediawiki/extensions/WikibaseLexememaster+56 -14
SECURITY: Check edit token in Special:MergeLexemesmediawiki/extensions/WikibaseLexemeREL1_39+13 -0
SECURITY: Check edit token in Special:MergeLexemesmediawiki/extensions/WikibaseLexemeREL1_40+13 -0
SECURITY: Check edit token in Special:MergeLexemesmediawiki/extensions/WikibaseLexemeREL1_41+13 -0
SECURITY: Check edit token in Special:MergeLexemesmediawiki/extensions/WikibaseLexememaster+13 -0
Customize query in gerrit

Related Objects

Event Timeline

Lucas_Werkmeister_WMDE created this task.Feb 9 2024, 10:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 9 2024, 10:02 AM
Lucas_Werkmeister_WMDE added projects: Wikidata, Wikidata Lexicographical data.Feb 9 2024, 10:03 AM

I really hope we can fix this in a way that doesn’t edit conflict with T356764, because I wouldn’t like to fix this task in public.

Lucas_Werkmeister_WMDE added a project: Wikidata Dev Team (Wikidata.org Slice).EditedFeb 9 2024, 11:00 AM

Okay, I think this should work:

0001-SECURITY-Check-edit-token-in-Special-MergeLexemes.patch2 KBDownload

diff --git a/src/MediaWiki/Specials/SpecialMergeLexemes.php b/src/MediaWiki/Specials/SpecialMergeLexemes.php
index dca2111390..71de16fd34 100644
--- a/src/MediaWiki/Specials/SpecialMergeLexemes.php
+++ b/src/MediaWiki/Specials/SpecialMergeLexemes.php
@@ -168,6 +168,18 @@ private function anonymousEditWarning(): string {
 	private function mergeLexemes( $serializedSourceId, $serializedTargetId ): void {
 		$sourceId = $this->getLexemeId( $serializedSourceId );
 		$targetId = $this->getLexemeId( $serializedTargetId );
+		// TODO inject interactor+localizer and move this check down a bit once this is public
+		// phpcs:disable MediaWiki.Classes.FullQualifiedClassName.Found
+		try {
+			\Wikibase\Repo\WikibaseRepo::getTokenCheckInteractor()
+				->checkRequestToken( $this->getContext(), 'wpEditToken' );
+		} catch ( \Wikibase\Repo\Interactors\TokenCheckException $e ) {
+			$message = \Wikibase\Repo\WikibaseRepo::getExceptionLocalizer()
+				->getExceptionMessage( $e );
+			$this->showErrorHTML( $message->parse() );
+			return;
+		}
+		// phpcs:enable
 
 		if ( !$sourceId ) {
 			$this->showInvalidLexemeIdError( $serializedSourceId );
 			return;
 		}
 		if ( !$targetId ) {
 			$this->showInvalidLexemeIdError( $serializedTargetId );
 			return;
 		}
 
 		try {
 			$this->mergeInteractor->mergeLexemes(

The mergeLexemes() call at the end there will probably be touched soon, so I put the check a bit higher up to (hopefully) avoid merge conflicts.

Lucas_Werkmeister_WMDE moved this task from In Task Breakdown to Ready for Peer Review on the Wikidata Dev Team (Wikidata.org Slice) board.Feb 9 2024, 11:00 AM
sbassett changed the task status from Open to In Progress.Feb 12 2024, 5:31 PM
sbassett triaged this task as Medium priority.
sbassett moved this task from Incoming to In Progress on the Security-Team board.
sbassett added projects: SecTeam-Processed, Vuln-CSRF.
sbassett added subscribers: mmartorana, sbassett.

@mmartorana to review.

mmartorana added a comment.Feb 12 2024, 6:53 PM

Hey @sbassett thanks for the triaging. Upon reviewing the provided code, there don't seem to be any apparent security concerns.

sbassett moved this task from In Progress to Security Patch To Deploy on the Security-Team board.Feb 12 2024, 8:34 PM
Mstyles subscribed.Feb 12 2024, 10:40 PM
In T357101#9528452, @Lucas_Werkmeister_WMDE wrote:

Okay, I think this should work:

0001-SECURITY-Check-edit-token-in-Special-MergeLexemes.patch2 KBDownload

diff --git a/src/MediaWiki/Specials/SpecialMergeLexemes.php b/src/MediaWiki/Specials/SpecialMergeLexemes.php
index dca2111390..71de16fd34 100644
--- a/src/MediaWiki/Specials/SpecialMergeLexemes.php
+++ b/src/MediaWiki/Specials/SpecialMergeLexemes.php
@@ -168,6 +168,18 @@ private function anonymousEditWarning(): string {
 	private function mergeLexemes( $serializedSourceId, $serializedTargetId ): void {
 		$sourceId = $this->getLexemeId( $serializedSourceId );
 		$targetId = $this->getLexemeId( $serializedTargetId );
+		// TODO inject interactor+localizer and move this check down a bit once this is public
+		// phpcs:disable MediaWiki.Classes.FullQualifiedClassName.Found
+		try {
+			\Wikibase\Repo\WikibaseRepo::getTokenCheckInteractor()
+				->checkRequestToken( $this->getContext(), 'wpEditToken' );
+		} catch ( \Wikibase\Repo\Interactors\TokenCheckException $e ) {
+			$message = \Wikibase\Repo\WikibaseRepo::getExceptionLocalizer()
+				->getExceptionMessage( $e );
+			$this->showErrorHTML( $message->parse() );
+			return;
+		}
+		// phpcs:enable
 
 		if ( !$sourceId ) {
 			$this->showInvalidLexemeIdError( $serializedSourceId );
 			return;
 		}
 		if ( !$targetId ) {
 			$this->showInvalidLexemeIdError( $serializedTargetId );
 			return;
 		}
 
 		try {
 			$this->mergeInteractor->mergeLexemes(

The mergeLexemes() call at the end there will probably be touched soon, so I put the check a bit higher up to (hopefully) avoid merge conflicts.

thank you for the patch @Lucas_Werkmeister_WMDE, it's been deployed

Mstyles mentioned this in T353904: Write and send supplementary release announcement for extensions and skins with security patches (1.39.7/1.40.3/1.41.1).Feb 12 2024, 10:45 PM
Mstyles moved this task from Security Patch To Deploy to Watching on the Security-Team board.
ArthurTaylor moved this task from Ready for Peer Review to Ready for Tech Verification on the Wikidata Dev Team (Wikidata.org Slice) board.Feb 13 2024, 7:26 AM
Lucas_Werkmeister_WMDE changed the task status from In Progress to Stalled.Feb 13 2024, 1:48 PM
Lucas_Werkmeister_WMDE moved this task from Ready for Tech Verification to Parent Task on the Wikidata Dev Team (Wikidata.org Slice) board.

Seems to work (tested at https://test.wikidata.org/wiki/Special:MergeLexemes?from-id=L2843&to-id=L338), thanks!

Moving back to Parent Tasks on our board – I think this is now just stalled until the next security release publishes the patch, at which point we’ll want to clean it up a bit more with a follow-up change on the master branch. (On the release branches, IMHO it’s enough to keep the patch as it is.)

sbassett added a comment.Feb 13 2024, 4:16 PM
In T357101#9537753, @Lucas_Werkmeister_WMDE wrote:

Moving back to Parent Tasks on our board – I think this is now just stalled until the next security release publishes the patch, at which point we’ll want to clean it up a bit more with a follow-up change on the master branch. (On the release branches, IMHO it’s enough to keep the patch as it is.)

Yes, it should go out with the next supplemental release (T353904), due out by the end of March 2024. And since ext:WikibaseLexeme isn't bundled, it could theoretically be made public, and the patch backported, sooner. Which is handy if there are concerns about future conflicts, etc.

jnuche raised the priority of this task from Medium to Unbreak Now!.EditedMar 4 2024, 9:53 AM
jnuche added a parent task: T354439: 1.42.0-wmf.21 deployment blockers.
jnuche subscribed.

Hi there, patch 01-T357101.patch is currently failing to apply. We will need a rebased version at the current release dir /srv/patches/1.42.0-wmf.20/extensions/WikibaseLexeme in the deployment server. Otherwise the upcoming 1.42.0-wmf.21 will be blocked.

EDIT: Sry, the rebased patch should of course go in a new dir for 1.42.0-wmf.21 will all other patches copied over. I can take care of that once a new patch is available

Lucas_Werkmeister_WMDE claimed this task.Mar 4 2024, 10:13 AM
Lucas_Werkmeister_WMDE moved this task from Parent Task to In Development on the Wikidata Dev Team (Wikidata.org Slice) board.
Lucas_Werkmeister_WMDE added a comment.Mar 4 2024, 10:20 AM

Rebased version:

0001-SECURITY-Check-edit-token-in-Special-MergeLexemes.patch1 KBDownload

But I don’t understand why you need this in the wmf.20 dir when it’s only conflicting against the upcoming wmf.21 branch?

jnuche added a comment.Mar 4 2024, 10:42 AM

But I don’t understand why you need this in the wmf.20 dir when it’s only conflicting against the upcoming wmf.21 branch?

@Lucas_Werkmeister_WMDE Sorry for the confusion. Please see my updated comment.

Thanks for the rebased patch, I'll add it to the deployment server.

Lucas_Werkmeister_WMDE removed Lucas_Werkmeister_WMDE as the assignee of this task.Mar 4 2024, 10:43 AM
Lucas_Werkmeister_WMDE moved this task from In Development to Parent Task on the Wikidata Dev Team (Wikidata.org Slice) board.

Alright!

Lucas_Werkmeister_WMDE lowered the priority of this task from Unbreak Now! to Medium.Mar 4 2024, 10:43 AM
jnuche removed a parent task: T354439: 1.42.0-wmf.21 deployment blockers.Mar 5 2024, 8:31 AM
Mstyles added a subscriber: gerritbot.Mar 22 2024, 6:42 PM

pushing the rebased patch to gerrit for the supplemental release: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseLexeme/+/1013359

gerritbot added a comment.Mar 23 2024, 12:20 PM

Change #1013359 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] SECURITY: Check edit token in Special:MergeLexemes

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

Jdforrester-WMF added a project: MW-1.42-notes (1.42.0-wmf.23; 2024-03-19).Mar 25 2024, 12:12 PM
Lydia_Pintscher subscribed.Apr 2 2024, 9:08 AM
gerritbot added a comment.Apr 4 2024, 11:52 AM

Change #1016872 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_41] SECURITY: Check edit token in Special:MergeLexemes

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

gerritbot added a project: Patch-For-Review.Apr 4 2024, 11:52 AM
gerritbot added a comment.Apr 4 2024, 1:08 PM

Change #1016872 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_41] SECURITY: Check edit token in Special:MergeLexemes

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

gerritbot added a comment.Apr 4 2024, 1:15 PM

Change #1016876 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_40] SECURITY: Check edit token in Special:MergeLexemes

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

Lucas_Werkmeister_WMDE changed the task status from Stalled to Open.Apr 4 2024, 1:17 PM

No longer stalled; I’m backporting the fix to release branches now, and we can also clean up the code.

gerritbot added a comment.Apr 4 2024, 1:39 PM

Change #1016876 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_40] SECURITY: Check edit token in Special:MergeLexemes

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

gerritbot added a comment.Apr 4 2024, 1:40 PM

Change #1016877 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@REL1_39] SECURITY: Check edit token in Special:MergeLexemes

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

gerritbot added a comment.Apr 4 2024, 2:05 PM

Change #1016877 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_39] SECURITY: Check edit token in Special:MergeLexemes

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

Lucas_Werkmeister_WMDE claimed this task.Apr 4 2024, 2:31 PM
Lucas_Werkmeister_WMDE moved this task from Parent Task to In Development on the Wikidata Dev Team (Wikidata.org Slice) board.
gerritbot added a comment.Apr 4 2024, 2:50 PM

Change #1017092 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Clean up edit token check in SpecialMergeLexemes

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

Lucas_Werkmeister_WMDE removed Lucas_Werkmeister_WMDE as the assignee of this task.Apr 4 2024, 2:55 PM
Lucas_Werkmeister_WMDE moved this task from In Development to Ready for Peer Review on the Wikidata Dev Team (Wikidata.org Slice) board.
ArthurTaylor moved this task from Ready for Peer Review to In Peer Review on the Wikidata Dev Team (Wikidata.org Slice) board.Apr 5 2024, 7:01 AM
ArthurTaylor moved this task from In Peer Review to Ready for Tech Verification on the Wikidata Dev Team (Wikidata.org Slice) board.Apr 5 2024, 7:11 AM
gerritbot added a comment.Apr 5 2024, 7:28 AM

Change #1017092 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Clean up edit token check in SpecialMergeLexemes

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Apr 5 2024, 9:30 AM
Lucas_Werkmeister_WMDE claimed this task.
Lucas_Werkmeister_WMDE moved this task from Ready for Tech Verification to Done on the Wikidata Dev Team (Wikidata.org Slice) board.

I think this is done; can someone™ make the task public? :)

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 5 2024, 4:46 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Low.
sbassett removed a project: Patch-For-Review.
Mstyles renamed this task from Special:MergeLexemes makes edits on GET requests without edit tokens to CVE-2024-34502: Special:MergeLexemes makes edits on GET requests without edit tokens.May 6 2024, 9:02 AM
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