Page MenuHomePhabricator

XSS in ISA tool
Closed, ResolvedPublicSecurity

Description

There are several places where the ISA tool interprets texts from Wikidata as HTML without escaping them. This is problematic, because Wikidata labels and descriptions are not necessarily HTML-safe.

Example: go to https://isa.toolforge.org/campaigns/77/participate, then search for the item ID Q43981055 in the “depicts” field. You will get one browser alert as soon as the search results load, and then another one if you select the search result.

Details

Risk Rating
Low
Author Affiliation
Wikimedia Deutschland

Event Timeline

Possible fix:

diff --git a/isa/src/participate.js b/isa/src/participate.js
index 764476e..17022c5 100644
--- a/isa/src/participate.js
+++ b/isa/src/participate.js
@@ -97,9 +97,13 @@ function searchResultsFormat(state) {
     if (!state.id) {
       return state.text;
     }
-    var $state = $(
-      '<span class="search-result-label">' + state.text + '</span> <br> <span class="search-result-description">' + state.description + '</span>'
-    );
+    var $state = $('<span class="search-result-label">')
+        .text(state.text)
+        .add('<br>')
+        .add(
+            $('<span class="search-result-description">')
+                .text(state.description)
+        );
     return $state;
   }
 
diff --git a/isa/src/participation-manager/populate.js b/isa/src/participation-manager/populate.js
index c88c269..7358976 100644
--- a/isa/src/participation-manager/populate.js
+++ b/isa/src/participation-manager/populate.js
@@ -139,7 +139,7 @@ ParticipationManager.prototype.populateStructuredData = function(filename, callb
             // now we have the labels, populate the statements area to show existing depicts items
             // we need to extract isProminent from results of previous API call to Commons
             // and add the labels and descriptions found to the existing stored data
-            var intialStatementsHtml = "";
+            var intialStatementsHtml = $();
             for (var qvalue in response.entities) {
                 var storedStatementData = getStoredStatementData(qvalue);
 
@@ -151,7 +151,7 @@ ParticipationManager.prototype.populateStructuredData = function(filename, callb
                     isProminent = storedStatementData.isProminent,
                     statementId = storedStatementData.statementId;
 
-                intialStatementsHtml += me.getStatementHtml(qvalue, label, description, isProminent, statementId);
+                intialStatementsHtml.add(me.getStatementHtml(qvalue, label, description, isProminent, statementId));
 
                 // save label and description
                 storedStatementData.label = label;
@@ -178,18 +178,39 @@ ParticipationManager.prototype.getStatementHtml = function(item, label, descript
     var prominentHoverText = this.i18nStrings['Mark this depicted item as prominent'];
     var notProminentHoverText = this.i18nStrings['Mark this depicted item as NOT prominent'];
 
-    var isProminentButtonHtml = isProminent ?
-        '<button class="btn btn-sm prominent-btn active" title=' + notProminentHoverText + '><i class="fas fa-flag"></i></button>' :
-        '<button class="btn btn-sm  prominent-btn" title=' + prominentHoverText  + '><i class="fas fa-flag"></i></button>';
-
-    return [
-        '<div class="depict-tag-item" ' + statementIdAttribute + ' title="' + description + '">',
-        '<div class="depict-tag-label">',
-        '<div class="label btn-sm"><span class="depict-tag-label-text"><a href="//www.wikidata.org/wiki/' + item + '" target="_blank">' + label + '</a></span> <span class="depict-tag-qvalue">' + item + '</span></div>',
-        isProminentButtonHtml,
-        '<div class="depict-tag-btn">',
-        '<button class="fas fa-trash btn-link btn" title="' + this.i18nStrings['Remove this depicted item']  + '"></button></div>',
-        '</div></div></div>'].join("");
+    var isProminentButtonHtml = $('<button class="btn btn-sm prominent-btn"><i class="fas fa-flag"></i></button>');
+    isProminentButtonHtml.attr('title', prominentHoverText);
+    if (isProminent) {
+        isProminentButtonHtml.addClass('active');
+    }
+
+    return $('<div class="depict-tag-item" ' + statementIdAttribute + '>')
+        .attr('title', description)
+        .append(
+            $('<div class="depict-tag-label">')
+                .append(
+                    $('<div class="label btn-sm">')
+                        .append(
+                            $('<span class="depict-tag-label-text">')
+                                .append(
+                                    $('<a href="//www.wikidata.org/wiki/' + item + '" target="_blank">')
+                                        .text(label))
+                        )
+                        .append(' ')
+                        .append(
+                            $('<span class="depict-tag-qvalue">')
+                                .text(item)
+                        )
+                )
+                .append(isProminentButtonHtml)
+                .append(
+                    $('<div class="depict-tag-btn">')
+                        .append(
+                            $('<button class="fas fa-trash btn-link btn"')
+                                .attr('title', this.i18nStrings['Remove this depicted item'])
+                        )
+                )
+        );
 }
 
 function populateCaption(language, text) {

I’ve tested the first part (participate.js – the search result), but I don’t think I can test the second part (populate.js – after selecting the search result) without a local dev setup, which I don’t have.

@LucasWerkmeister Thanks very much for pointing this out. @NavinoEvans and myself are going to have a look at this hopefully :)

sbassett changed Risk Rating from N/A to High.Jul 14 2021, 8:04 PM
Urbanecm added subscribers: Ladsgroup, Matanya, Quiddity and 3 others.

@Huji @Ladsgroup @Matanya @Quiddity @zhuyifei1999 Hello Standards Committee, unfortunately, it appears there is no follow-up from the tool maintainers within a few months. Could you please help to identify next steps per https://wikitech.wikimedia.org/wiki/Help:Toolforge/Toolforge_standards_committee#Security_issues? Thanks!

I think we could apply the patch above T286415#7203570. The other issue is looks like more complex fix however.

Sebastian Berlin <sebastian.berlin@wikimedia.se> will start to work on the tool very soon. He said he would start to look at this issue first thing.

He would like to be granted access to this task. Can someone involved here give him access ? His phabricator account is https://phabricator.wikimedia.org/p/Sebastian_Berlin-WMSE/

Thanks

@Anthere This was done (by @sbassett). Thanks for the update.

Sebastian_Berlin-WMSE changed the task status from Open to In Progress.Nov 12 2021, 4:09 PM

@Huji @Ladsgroup @Matanya @Quiddity @zhuyifei1999 Hello Standards Committee, unfortunately, it appears there is no follow-up from the tool maintainers within a few months. Could you please help to identify next steps per https://wikitech.wikimedia.org/wiki/Help:Toolforge/Toolforge_standards_committee#Security_issues? Thanks!

I've uploaded a patch. @NavinoEvans has started reviewing. Anyone else is welcome to also have a look.

The tool has been updated with the patch for this task. I don't know if that's enough to resolve it or if there are any remaining steps.

The tool has been updated with the patch for this task. I don't know if that's enough to resolve it or if there are any remaining steps.

The example within the task description appears to be fixed. And since the patch to fix the issue was public, we can make this task public and resolve it.

sbassett triaged this task as Medium priority.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from High to Low.