Per T255918#6247910, LogEventList::getFiltersDesc is insecurely using message text to build options names for an HTML multi-select field. The relevant code should use escaped() instead of text().
Description
Description
Details
Details
- Author Affiliation
- WMF Technology Dept
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Reedy | T256334 Release MediaWiki 1.31.9/1.34.3/1.35.0 | |||
| Resolved | Reedy | T256335 Tracking bug for MediaWiki 1.31.9/1.34.3/1.35.0 | |||
| Resolved | Security | sbassett | T256171 Unescaped message used in HTML within LogEventsList (CVE-2020-25815) |
Event Timeline
Comment Actions
Proposed patch:
From c3ea35101de31f9ffc341c52137493926aa9aa33 Mon Sep 17 00:00:00 2001 From: sbassett <sbassett@wikimedia.org> Date: Tue, 23 Jun 2020 14:44:03 -0500 Subject: [PATCH] Unescaped message used in HTML within LogEventsList * Use escaped() instead of text() for messages used to build HTML multi-select field. * Clean up old FIXME conditional since T199657 has been resolved for over a year now. Bug: T256171 --- includes/logging/LogEventsList.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/includes/logging/LogEventsList.php b/includes/logging/LogEventsList.php index c8a3e73a35..90b648a6ec 100644 --- a/includes/logging/LogEventsList.php +++ b/includes/logging/LogEventsList.php @@ -207,11 +207,7 @@ class LogEventsList extends ContextSource { $default = []; foreach ( $filter as $type => $val ) { $message = $this->msg( "logeventslist-{$type}-log" ); - // FIXME: Remove this check once T199657 is fully resolved. - if ( !$message->exists() ) { - $message = $this->msg( "log-show-hide-{$type}" )->params( $this->msg( 'show' )->text() ); - } - $options[ $message->text() ] = $type; + $options[ $message->escaped() ] = $type; if ( $val === false ) { $default[] = $type; -- 2.22.0
Comment Actions
Hah, I didn't notice that T199657 was resolved and the conditional is now unnecessary. In this case, I think we should adopt an approach similar to T255918, i.e. replace $options with
$optionsMsg["logeventslist-{$type}-log"] = $type;
And then at line 224, replace with
'options-messages' => $optionsMsg,
Comment Actions
Patch revision 2, special page unit tests seem fine:
From be6629233b5efbb12b28097273c0b73d83c99e7c Mon Sep 17 00:00:00 2001 From: sbassett <sbassett@wikimedia.org> Date: Tue, 23 Jun 2020 14:44:03 -0500 Subject: [PATCH] Unescaped message used in HTML within LogEventsList * Use options-messages instead of text() for messages used to build HTML multi-select field. * Clean up old FIXME conditional since T199657 has been resolved for over a year now. Bug: T256171 --- includes/logging/LogEventsList.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/includes/logging/LogEventsList.php b/includes/logging/LogEventsList.php index c8a3e73a35..1f633cee3d 100644 --- a/includes/logging/LogEventsList.php +++ b/includes/logging/LogEventsList.php @@ -206,12 +206,7 @@ class LogEventsList extends ContextSource { $options = []; $default = []; foreach ( $filter as $type => $val ) { - $message = $this->msg( "logeventslist-{$type}-log" ); - // FIXME: Remove this check once T199657 is fully resolved. - if ( !$message->exists() ) { - $message = $this->msg( "log-show-hide-{$type}" )->params( $this->msg( 'show' )->text() ); - } - $options[ $message->text() ] = $type; + $optionsMsg["logeventslist-{$type}-log"] = $type; if ( $val === false ) { $default[] = $type; @@ -221,7 +216,7 @@ class LogEventsList extends ContextSource { 'class' => 'HTMLMultiSelectField', 'label-message' => 'logeventslist-more-filters', 'flatlist' => true, - 'options' => $options, + 'options-messages' => $optionsMsg, 'default' => $default, ]; } -- 2.22.0
Comment Actions
OK, we're almost there. One issue pointed out inline (it would also cause phan failures).
@@ -206,12 +206,7 @@ class LogEventsList extends ContextSource { $options = []; $default = [];
Here $options should be replaced with $optionsMsg.
Comment Actions
Third time's the charm :)
From 004947a11f936ba3934e70b26e583ad64e2a1c5e Mon Sep 17 00:00:00 2001 From: sbassett <sbassett@wikimedia.org> Date: Tue, 23 Jun 2020 14:44:03 -0500 Subject: [PATCH] Unescaped message used in HTML within LogEventsList * Use options-messages instead of text() for messages used to build HTML multi-select field. * Clean up old FIXME conditional since T199657 has been resolved for over a year now. Bug: T256171 --- includes/logging/LogEventsList.php | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/includes/logging/LogEventsList.php b/includes/logging/LogEventsList.php index c8a3e73a35..166116aade 100644 --- a/includes/logging/LogEventsList.php +++ b/includes/logging/LogEventsList.php @@ -203,15 +203,10 @@ class LogEventsList extends ContextSource { * @return array Form descriptor */ private function getFiltersDesc( $filter ) { - $options = []; + $optionsMsg = []; $default = []; foreach ( $filter as $type => $val ) { - $message = $this->msg( "logeventslist-{$type}-log" ); - // FIXME: Remove this check once T199657 is fully resolved. - if ( !$message->exists() ) { - $message = $this->msg( "log-show-hide-{$type}" )->params( $this->msg( 'show' )->text() ); - } - $options[ $message->text() ] = $type; + $optionsMsg["logeventslist-{$type}-log"] = $type; if ( $val === false ) { $default[] = $type; @@ -221,7 +216,7 @@ class LogEventsList extends ContextSource { 'class' => 'HTMLMultiSelectField', 'label-message' => 'logeventslist-more-filters', 'flatlist' => true, - 'options' => $options, + 'options-messages' => $optionsMsg, 'default' => $default, ]; } -- 2.22.0
Comment Actions
It always is :)
Looks good now. I was a bit scared by https://codesearch.wmflabs.org/search/?q=log-show-hide-&i=nope&files=%5C.json&repos= but I see that these were left for compatibility with older MW.
Comment Actions
:)
Looks good now.
Cool, I'll plan to deploy this and T255918 this coming Monday and then hold both for T256334.
I was a bit scared by https://codesearch.wmflabs.org/search/?q=log-show-hide-&i=nope&files=%5C.json&repos= but I see that these were left for compatibility with older MW.
Ok, thank goodness.
Comment Actions
rMW90f6829ce06e: Use ::class to refer HTML form field classes is going to cause a conflict for T257978: 1.36.0-wmf.10 deployment blockers
Trivial to fix... Or applied with -3
Comment Actions
This isn't needed in REL1_31...
Code added in rMWcab5b3afc725: LogEventsList: Add backwards-compatibility for log-show-hide messages and rMW38756eae46a8: Special:Log: Convert to HTMLForm which is REL1_32 and later
Comment Actions
Patch seems applied on wmf.9 and wmf.10. I assume it's working ok since it appears to gracefully override the change in rMW90f6829ce06e.
Comment Actions
I did dig through some of the scap release/train scripts, and it did look like they used -3 so it probably just applied fine