Merge pull request from GHSA-qv6c-367r-3w6q

feat: sanitize HTML tags to prevent XSS vulnerabilities

Author: Feanil Patel

Changelog.md

@@ -1,6 +1,12 @@
 Drag and Drop XBlock changelog
 ==============================
 
+Version 3.0.0 (2022-11-18)
+---------------------------
+
+* Sanitize HTML tags to prevent XSS vulnerabilities.  
+  BREAKING CHANGE: Disallowed HTML tags (e.g. `<script>`) will no longer be rendered in LMS and Studio. 
+
 Version 2.7.0 (2022-11-15)
 ---------------------------
 

drag_and_drop_v2/drag_and_drop_v2.py

@@ -31,7 +31,7 @@
 from .default_data import DEFAULT_DATA
 from .utils import (
     Constants, SHOWANSWER, DummyTranslationService, FeedbackMessage,
-    FeedbackMessages, ItemStats, StateMigration, _clean_data, _
+    FeedbackMessages, ItemStats, StateMigration, _clean_data, _, sanitize_html
 )
 
 # Globals ###########################################################
@@ -388,11 +388,12 @@ def items_without_answers():
                     item['expandedImageURL'] = self._expand_static_url(image_url)
                 else:
                     item['expandedImageURL'] = ''
+                item['displayName'] = sanitize_html(item.get('displayName', ''))
             return items
 
         return {
             "block_id": six.text_type(self.scope_ids.usage_id),
-            "display_name": self.display_name,
+            "display_name": sanitize_html(self.display_name),
             "type": self.CATEGORY,
             "weight": self.weight,
             "mode": self.mode,
@@ -407,9 +408,9 @@ def items_without_answers():
             "display_zone_borders": self.data.get('displayBorders', False),
             "display_zone_borders_dragging": self.data.get('displayBordersDragging', False),
             "items": items_without_answers(),
-            "title": self.display_name,
+            "title": sanitize_html(self.display_name),
             "show_title": self.show_title,
-            "problem_text": self.question_text,
+            "problem_text": sanitize_html(self.question_text),
             "show_problem_header": self.show_question_header,
             "target_img_expanded_url": self.target_img_expanded_url,
             "target_img_description": self.target_img_description,
@@ -667,7 +668,7 @@ def show_answer(self, data, suffix=''):
                 except (TypeError, AttributeError):
                     logger.debug('Unable to perform URL substitution on the explanation: %s', explanation)
 
-            answer['explanation'] = explanation
+            answer['explanation'] = sanitize_html(explanation)
         return answer
 
     @XBlock.json_handler
@@ -885,7 +886,7 @@ def _present_feedback(feedback_messages):
         Transforms feedback messages into format expected by frontend code
         """
         return [
-            {"message": msg.message, "message_class": msg.message_class}
+            {"message": sanitize_html(msg.message), "message_class": msg.message_class}
             for msg in feedback_messages
             if msg.message
         ]
@@ -1195,7 +1196,14 @@ def zones(self):
         """
         # Convert zone data from old to new format if necessary
         migrator = StateMigration(self)
-        return [migrator.apply_zone_migrations(zone) for zone in self.data.get('zones', [])]
+        migrated_zones = []
+
+        for zone in self.data.get('zones', []):
+            result = migrator.apply_zone_migrations(zone)
+            result['title'] = sanitize_html(result.get('title', ''))
+            migrated_zones.append(result)
+
+        return migrated_zones
 
     def _get_zone_by_uid(self, uid):
         """

drag_and_drop_v2/public/js/drag_and_drop_edit.js

@@ -448,8 +448,8 @@ function DragAndDropEditBlock(runtime, element, params) {
                                 _fn.build.$el.zonesPreview.append(
                                     _fn.tpl.zoneElement({
                                         uid: zoneObj.uid,
-                                        title: gettext(zoneObj.title),
-                                        description: gettext(zoneObj.description),
+                                        title: Handlebars.Utils.escapeExpression(gettext(zoneObj.title)),
+                                        description: Handlebars.Utils.escapeExpression(gettext(zoneObj.description)),
                                         x_percent: (+zoneObj.x) / imgWidth * 100,
                                         y_percent: (+zoneObj.y) / imgHeight * 100,
                                         width_percent: (+zoneObj.width) / imgWidth * 100,

drag_and_drop_v2/utils.py

@@ -6,7 +6,15 @@
 import re
 from collections import namedtuple
 
-from bleach.sanitizer import Cleaner
+import bleach
+
+try:
+    from bleach.css_sanitizer import CSSSanitizer
+except (ImportError, ModuleNotFoundError):
+    # pylint: disable=fixme
+    # TODO: The bleach library changes the way CSS Styles are cleaned in version 5.0.0. The edx-platform uses version
+    #  4.1.0 in Maple, so this import is handled within a try block. This can be removed for the Nutmeg release.
+    CSSSanitizer = None
 
 
 def _(text):
@@ -24,11 +32,124 @@ def ngettext_fallback(text_singular, text_plural, number):
 
 def _clean_data(data):
     """ Remove html tags and extra white spaces e.g newline, tabs etc from provided data """
-    cleaner = Cleaner(tags=[], strip=True)
+    cleaner = bleach.Cleaner(tags=[], strip=True)
     cleaned_text = " ".join(re.split(r"\s+", cleaner.clean(data), flags=re.UNICODE)).strip()
     return cleaned_text
 
 
+ALLOWED_TAGS = bleach.ALLOWED_TAGS + [
+    'br',
+    'caption',
+    'dd',
+    'del',
+    'div',
+    'dl',
+    'dt',
+    'h1',
+    'h2',
+    'h3',
+    'h4',
+    'h5',
+    'h6',
+    'hr',
+    'img',
+    'p',
+    'pre',
+    's',
+    'strike',
+    'span',
+    'sub',
+    'sup',
+    'table',
+    'tbody',
+    'td',
+    'tfoot',
+    'th',
+    'thead',
+    'tr',
+    'u',
+]
+ALLOWED_ATTRIBUTES = {
+    '*': ['class', 'style', 'id'],
+    'a': ['href', 'title', 'target', 'rel'],
+    'abbr': ['title'],
+    'acronym': ['title'],
+    'audio': ['controls', 'autobuffer', 'autoplay', 'src'],
+    'img': ['src', 'alt', 'title', 'width', 'height'],
+    'table': ['border', 'cellspacing', 'cellpadding'],
+    'td': ['style', 'scope'],
+}
+ALLOWED_STYLES = [
+    "azimuth",
+    "background-color",
+    "border-bottom-color",
+    "border-collapse",
+    "border-color",
+    "border-left-color",
+    "border-right-color",
+    "border-top-color",
+    "clear",
+    "color",
+    "cursor",
+    "direction",
+    "display",
+    "elevation",
+    "float",
+    "font",
+    "font-family",
+    "font-size",
+    "font-style",
+    "font-variant",
+    "font-weight",
+    "height",
+    "letter-spacing",
+    "line-height",
+    "overflow",
+    "pause",
+    "pause-after",
+    "pause-before",
+    "pitch",
+    "pitch-range",
+    "richness",
+    "speak",
+    "speak-header",
+    "speak-numeral",
+    "speak-punctuation",
+    "speech-rate",
+    "stress",
+    "text-align",
+    "text-decoration",
+    "text-indent",
+    "unicode-bidi",
+    "vertical-align",
+    "voice-family",
+    "volume",
+    "white-space",
+    "width",
+]
+
+
+def sanitize_html(raw_body: str) -> str:
+    """
+    Remove not allowed HTML tags to mitigate XSS vulnerabilities.
+    """
+    bleach_options = dict(
+        tags=ALLOWED_TAGS,
+        protocols=bleach.ALLOWED_PROTOCOLS,
+        strip=True,
+        attributes=ALLOWED_ATTRIBUTES,
+    )
+    if CSSSanitizer:
+        bleach_options['css_sanitizer'] = CSSSanitizer()
+    else:
+        # pylint: disable=fixme
+        # TODO: This is maintaining backward compatibility with bleach 4.1.0 used in Maple release of edx-platform.
+        #  This can be removed for the Nutmeg release which uses bleach 5.0.0
+        bleach_options['styles'] = ALLOWED_STYLES
+
+    return bleach.clean(raw_body, **bleach_options)
+
+
 class DummyTranslationService:
     """
     Dummy drop-in replacement for i18n XBlock service

setup.py

@@ -23,7 +23,7 @@ def package_data(pkg, root_list):
 
 setup(
     name='xblock-drag-and-drop-v2',
-    version='2.7.0',
+    version='3.0.0',
     description='XBlock - Drag-and-Drop v2',
     packages=['drag_and_drop_v2'],
     install_requires=[

tests/integration/test_custom_data_render.py

@@ -22,7 +22,7 @@ def test_items_rendering(self):
         self.assertEqual(len(items), 3)
         self.assertIn('<b>1</b>', self.get_element_html(items[0]))
         self.assertIn('<i>2</i>', self.get_element_html(items[1]))
-        self.assertIn('<span style="color:red">X</span>', self.get_element_html(items[2]))
+        self.assertIn('<span style="color: red;">X</span>', self.get_element_html(items[2]))
 
     def test_html_title_renders_properly(self):
         """

tests/integration/test_title_and_question.py

@@ -40,7 +40,7 @@ def test_problem_parameters(self, problem_text, show_problem_header):
         ('plain shown', 'title1', True),
         ('plain hidden', 'title2', False),
         ('html shown', 'title with <i>HTML</i>', True),
-        ('html hidden', '<span style="color:red">Title: HTML?</span>', False)
+        ('html hidden', '<span style="color: red;">Title: HTML?</span>', False)
     )
     def test_title_parameters(self, _, display_name, show_title):
         const_page_name = 'Test show title parameter'