Page MenuHomePhabricator

Data fields in Commons tabular datasets allow running arbitrary JS (CVE-2022-28210)
Closed, ResolvedPublicSecurity

Description

In tabular datasets on Commons, the values' contents are placed as raw HTML for the displayed values on the table, allowing any user to insert arbitrary JS onto any .tab page in the Data namespace. Tested with "<script>alert(1)</script>", runs on fields of all data types. (I've only tested this in previews so far, as I am very hesitant to save anything to Commons with this kind of exploit visible.)

Event Timeline

Confirmed on localhost that this indeed results in stored XSS; LocalSettings snippet to configure a Data namespace locally:

$wgJsonConfigModels['Tabular.JsonConfig'] = 'JsonConfig\JCTabularContent';
$wgJsonConfigs['Tabular.JsonConfig'] = [
    'namespace' => 486,
    'nsName' => 'Data',
    // page name must end in ".tab", and contain at least one symbol                                                                                                                                                                                                                                                                                       
    'pattern' => '/.\.tab$/',
    'license' => 'CC0-1.0',
    'isLocal' => false,
];
$wgJsonConfigs['Tabular.JsonConfig']['store'] = true;

Minimal data JSON:

{
    "license": "CC0-1.0",
    "schema": {
        "fields": [
            {
                "name": "xss",
                "type": "string",
                "title": {
                    "en": "script"
                }
            }
        ]
    },
    "data": [
        [
            "<script>alert('hi')</script>"
        ]
    ]
}

The alert is shown when the table is viewed.

Patch that seems to work locally:

Here’s the diff with a bit more context before:

diff --git a/includes/JCTabularContentView.php b/includes/JCTabularContentView.php
index b5aa8fd..87c32ce 100644
--- a/includes/JCTabularContentView.php
+++ b/includes/JCTabularContentView.php
@@ -110,50 +110,52 @@ public function valueToHtml(
                 foreach ( $row as $column ) {
                     $colIsValid = $column && $column instanceof JCValue && !$column->error();
                     $column =
                         ( $column && $column instanceof JCValue ) ? $column->getValue() : $column;
 
                     if ( count( $vals ) >= count( $headerAttributes ) ) {
                         $header = [];
                     } else {
                         $header = $headerAttributes[ count( $vals ) ];
                     }
 
                     if ( !$colIsValid ) {
                         $header['class'] = 'mw-tabular-error';
                     }
 
                     if ( is_object( $column ) ) {
                         $valueSize = count( (array)$column );
                         $column =
                             htmlspecialchars( JCUtils::pickLocalizedString( $column, $lang ) ) .
                             Html::element( 'span', $infoClass, "($valueSize)" );
                     } elseif ( is_bool( $column ) ) {
                         $column = $column ? '☑' : '☐';
                     } elseif ( $column === null ) {
                         $header['class'] = 'mw-tabular-value-null';
                         $column = '';
+                    } else {
+                        $column = htmlspecialchars( $column );
                     }
                     $vals[] = Html::rawElement( 'td', $header, $column );
                 }
                 $rows[] = $makeRow( $vals, $rowIsValid ? [] : [ 'class' => 'mw-tabular-error' ] );
             }
         }

Slightly updated patch to avoid a type error if the value is an array:

Third revision following discussion with @Krinkle:

Lucas_Werkmeister_WMDE lowered the priority of this task from Unbreak Now! to High.

Deployed, I can no longer reproduce the alert in a preview for Data:Data.tab.

Looks fixed. The change might have to be fiddled with, though, since I think it may have caused all the "type":"number" cells to disappear?

Ah – I tried setting the value to a number and got errors, so I assumed that numbers weren’t supported, but I guess that’s because I didn’t adjust the "type". I will probably not have time to fix that before 12:00 UTC today, I’m afraid, so if anyone else wants to take it over in the meantime, be my guest.

Alright, this patch should fix it:


I’ll try to deploy that later (review welcome).

Alright, should be fixed now (tested on this random tab file with numbers).

Thanks. SAL 1. SAL 2. Also tracking at T276237 and T297839.

One additional note: It looks like the patch under /srv/patches/1.38.0-wmf.22 might be the first version without the is_numeric check? What's actually applied under /srv/mediawiki-staging seems fine, so I just want to confirm that we still need to update the patch under /srv/patches.

Sorry, I forgot to add the second commit to /srv/patches yesterday. I added it to wmf.23 now (SAL 3), as well as to /srv/patches (for wmf.22 and wmf.23).

For public release, it probably makes sense to squash both commits into one, by the way.

Sorry, I forgot to add the second commit to /srv/patches yesterday. I added it to wmf.23 now (SAL 3), as well as to /srv/patches (for wmf.22 and wmf.23).

No problem and thanks for the update and deploy.

For public release, it probably makes sense to squash both commits into one, by the way.

Yes, thanks.

Mstyles renamed this task from Data fields in Commons tabular datasets allow running arbitrary JS to Data fields in Commons tabular datasets allow running arbitrary JS (CVE-2022-28210).Mar 31 2022, 5:41 PM
Mstyles closed this task as Resolved.
Mstyles moved this task from Watching to Our Part Is Done on the Security-Team board.
Mstyles changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 31 2022, 5:47 PM
Mstyles changed the edit policy from "Custom Policy" to "All Users".

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

[mediawiki/extensions/JsonConfig@master] SECURITY: HTML-escape strings and don’t display arrays

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

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

[mediawiki/extensions/JsonConfig@REL1_35] SECURITY: HTML-escape strings and don’t display arrays

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

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

[mediawiki/extensions/JsonConfig@REL1_36] SECURITY: HTML-escape strings and don’t display arrays

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

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

[mediawiki/extensions/JsonConfig@REL1_37] SECURITY: HTML-escape strings and don’t display arrays

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

Change 776026 merged by jenkins-bot:

[mediawiki/extensions/JsonConfig@REL1_37] SECURITY: HTML-escape strings and don’t display arrays

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

Change 775945 merged by jenkins-bot:

[mediawiki/extensions/JsonConfig@REL1_36] SECURITY: HTML-escape strings and don’t display arrays

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

Change 775944 merged by jenkins-bot:

[mediawiki/extensions/JsonConfig@REL1_35] SECURITY: HTML-escape strings and don’t display arrays

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

Change 776015 merged by jenkins-bot:

[mediawiki/extensions/JsonConfig@master] SECURITY: HTML-escape strings and don’t display arrays

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

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

[mediawiki/extensions/JsonConfig@REL1_38] SECURITY: HTML-escape strings and don’t display arrays

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

Change 776033 merged by jenkins-bot:

[mediawiki/extensions/JsonConfig@REL1_38] SECURITY: HTML-escape strings and don’t display arrays

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