Page MenuHomePhabricator

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

Description

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().

Event Timeline

sbassett created this task.Jun 23 2020, 7:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 23 2020, 7:39 PM

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

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,

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.

sbassett added a comment.EditedJun 24 2020, 8:59 PM

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

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

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

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.

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.

sbassett moved this task from Incoming to In Progress on the Security-Team board.Jun 29 2020, 3:15 PM
sbassett claimed this task.Jun 29 2020, 3:38 PM
sbassett added a comment.EditedJun 29 2020, 10:01 PM

Deployed. Holding for next security release.

sbassett moved this task from In Progress to Waiting on the Security-Team board.Jun 29 2020, 10:57 PM
sbassett moved this task from In Progress to Done on the user-sbassett board.
Reedy closed this task as Resolved.Sep 21 2020, 11:36 PM

Resolving for ease of tracking in parent

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.

Reedy added a comment.Sep 22 2020, 9:20 PM

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

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