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.)
Description
Details
Related Objects
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.
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' ] ); } }
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.
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.
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.
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
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
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
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
Change 776026 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@REL1_37] SECURITY: HTML-escape strings and don’t display arrays
Change 775945 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@REL1_36] SECURITY: HTML-escape strings and don’t display arrays
Change 775944 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@REL1_35] SECURITY: HTML-escape strings and don’t display arrays
Change 776015 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@master] SECURITY: HTML-escape strings and don’t display arrays
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
Change 776033 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@REL1_38] SECURITY: HTML-escape strings and don’t display arrays