VOOZH about

URL: https://phabricator.wikimedia.org/T256171

⇱ ⚓ T256171 Unescaped message used in HTML within LogEventsList (CVE-2020-25815)

Maniphest T256171

Unescaped message used in HTML within LogEventsList (CVE-2020-25815)
Closed, ResolvedPublicSecurity

Description

Per T255918#6247910, is insecurely using message text to build options names for an HTML multi-select field. The relevant code should use instead of .

Related Objects

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

Proposed patch:
[...]

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

Proposed patch:
[...]

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,

FTR, I've just tested this and it works as expected.

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

Patch revision 2, special page unit tests seem fine:

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 should be replaced with .

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

Third time's the charm :)

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

It always is :)

:)

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

Deployed. Holding for next security release.

sbassett moved this task from In Progress to Done on the user-sbassett board.
Comment Actions

Resolving for ease of tracking in parent

Comment Actions

I did dig through some of the scap release/train scripts, and it did look like they used so it probably just applied fine

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 24 2020, 11:15 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".
Legoktm renamed this task from Unescaped message used in HTML within LogEventsList to Unescaped message used in HTML within LogEventsList (CVE-2020-25815).Sep 27 2020, 11:50 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