From 05746b7068827247fce84b8fa0f19b3f254d20d2 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 27 May 2019 10:59:43 +0000
Subject: [PATCH] [IMP] *: script-safe JSON serialisation

* add a json_scriptsafe module, which re-exports the stdlib's json but
provides a script-safe `dumps` function, because of the way it works
the script-safe version is fine / safe to use outside script
contexts, so there's no need to provide different functions to qweb
contexts
* make the json.dumps in qweb contexts <script>-safe (should be
rendered using t-raw)
* update callsites to properly dump json:
- dump in template, not in the python code
- dump the entire structure at once, not bits and bobs inside a JSON
literal

Task 1999158

closes odoo/odoo#33682

Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>
---
 addons/point_of_sale/controllers/main.py     |  2 +-
 addons/point_of_sale/views/pos_templates.xml | 10 ++--
 addons/web/models/ir_http.py                 |  2 +-
 addons/web/views/webclient_templates.xml     |  2 +-
 odoo/addons/base/models/ir_ui_view.py        |  5 +-
 odoo/addons/base/tests/test_qweb.py          | 17 +++++++
 odoo/tools/json/__init__.py                  |  1 +
 odoo/tools/json/scriptsafe.py                | 50 ++++++++++++++++++++
 8 files changed, 78 insertions(+), 11 deletions(-)
 create mode 100644 odoo/tools/json/__init__.py
 create mode 100644 odoo/tools/json/scriptsafe.py

diff --git a/addons/point_of_sale/controllers/main.py b/addons/point_of_sale/controllers/main.py
index 0fca1369384..1dcebd32ef1 100644
--- a/addons/point_of_sale/controllers/main.py
+++ b/addons/point_of_sale/controllers/main.py
@@ -22,7 +22,7 @@ class PosController(http.Controller):
             return werkzeug.utils.redirect('/web#action=point_of_sale.action_client_pos_menu')
         pos_sessions.login()
         context = {
-            'session_info': json.dumps(request.env['ir.http'].session_info())
+            'session_info': request.env['ir.http'].session_info()
         }
         return request.render('point_of_sale.index', qcontext=context)
 
diff --git a/addons/point_of_sale/views/pos_templates.xml b/addons/point_of_sale/views/pos_templates.xml
index cb1a8a81a01..05804c58711 100644
--- a/addons/point_of_sale/views/pos_templates.xml
+++ b/addons/point_of_sale/views/pos_templates.xml
@@ -28,11 +28,11 @@
         <link rel="shortcut icon" href="/web/static/src/img/favicon.ico" type="image/x-icon"/>
 
         <script type="text/javascript">
-            var odoo = {
-                csrf_token: "<t t-esc="request.csrf_token(None)"/>",
-                session_info: <t t-raw="session_info"/>,
-                debug: "<t t-esc="debug"/>",
-            };
+            var odoo = <t t-raw="json.dumps({
+                'csrf_token': request.csrf_token(None),
+                'session_info': session_info,
+                'debug': debug,
+            })"/>;
         </script>
 
         <t t-call-assets="web.assets_common" t-css="false"/>
diff --git a/addons/web/models/ir_http.py b/addons/web/models/ir_http.py
index f2bfc70f9b4..6c3dac17193 100644
--- a/addons/web/models/ir_http.py
+++ b/addons/web/models/ir_http.py
@@ -18,7 +18,7 @@ class Http(models.AbstractModel):
     def webclient_rendering_context(self):
         return {
             'menu_data': request.env['ir.ui.menu'].load_menus(request.session.debug),
-            'session_info': json.dumps(self.session_info()),
+            'session_info': self.session_info(),
         }
 
     def session_info(self):
diff --git a/addons/web/views/webclient_templates.xml b/addons/web/views/webclient_templates.xml
index aa3f67af07d..e67005f0eaf 100644
--- a/addons/web/views/webclient_templates.xml
+++ b/addons/web/views/webclient_templates.xml
@@ -833,7 +833,7 @@
         <t t-call="web.layout">
             <t t-set="head_web">
                 <script type="text/javascript">
-                    odoo.session_info = <t t-raw="session_info"/>;
+                    odoo.session_info = <t t-raw="json.dumps(session_info)"/>;
                     odoo.reloadMenus = function () {
                         return new Promise(function(resolve, reject) {
                             function onLoaded() {
diff --git a/odoo/addons/base/models/ir_ui_view.py b/odoo/addons/base/models/ir_ui_view.py
index 5ee3d98b8aa..40f953978ee 100644
--- a/odoo/addons/base/models/ir_ui_view.py
+++ b/odoo/addons/base/models/ir_ui_view.py
@@ -17,7 +17,6 @@ from functools import partial
 from difflib import HtmlDiff
 from operator import itemgetter
 
-import json
 import werkzeug
 from lxml import etree
 from lxml.etree import LxmlError
@@ -30,7 +29,7 @@ from odoo.modules.module import get_resource_from_path, get_resource_path
 from odoo.osv import orm
 from odoo.tools import config, graph, ConstantMapping, SKIPPED_ELEMENT_TYPES, pycompat
 from odoo.tools.convert import _fix_multiple_roots
-from odoo.tools.parse_version import parse_version
+from odoo.tools.json import scriptsafe as json_scriptsafe
 from odoo.tools.safe_eval import safe_eval
 from odoo.tools.view_validation import valid_view
 from odoo.tools.translate import xml_translate, TRANSLATED_ATTRS
@@ -1358,7 +1357,7 @@ actual arch.
             request=request,  # might be unbound if we're not in an httprequest context
             debug=request.session.debug if request else '',
             test_mode_enabled=bool(config['test_enable'] or config['test_file']),
-            json=json,
+            json=json_scriptsafe,
             quote_plus=werkzeug.url_quote_plus,
             time=time,
             datetime=datetime,
diff --git a/odoo/addons/base/tests/test_qweb.py b/odoo/addons/base/tests/test_qweb.py
index d8ce5e48f68..ee9b39770a2 100644
--- a/odoo/addons/base/tests/test_qweb.py
+++ b/odoo/addons/base/tests/test_qweb.py
@@ -75,6 +75,23 @@ class TestQWebTField(TransactionCase):
         text = etree.fromstring(view1.render()).find('span').text
         self.assertEqual(text, u'5.0000')
 
+    def test_xss_breakout(self):
+        view = self.env['ir.ui.view'].create({
+            'name': 'dummy', 'type': 'qweb',
+            'arch': u"""
+                <t t-name="base.dummy">
+                    <root>
+                        <script type="application/javascript">
+                            var s = <t t-raw="json.dumps({'key': malicious})"/>;
+                        </script>
+                    </root>
+                </t>
+            """
+        })
+        rendered = view.render({'malicious': '1</script><script>alert("pwned")</script><script>'}).decode()
+        self.assertIn('alert', rendered, "%r doesn't seem to be rendered" % rendered)
+        doc = etree.fromstring(rendered)
+        self.assertEqual(len(doc.xpath('//script')), 1)
 
 class TestQWebNS(TransactionCase):
     def test_render_static_xml_with_namespace(self):
diff --git a/odoo/tools/json/__init__.py b/odoo/tools/json/__init__.py
new file mode 100644
index 00000000000..40a96afc6ff
--- /dev/null
+++ b/odoo/tools/json/__init__.py
@@ -0,0 +1 @@
+# -*- coding: utf-8 -*-
diff --git a/odoo/tools/json/scriptsafe.py b/odoo/tools/json/scriptsafe.py
new file mode 100644
index 00000000000..f1147b02883
--- /dev/null
+++ b/odoo/tools/json/scriptsafe.py
@@ -0,0 +1,50 @@
+# -*- coding: utf-8 -*-
+import json as json_
+import re
+from json import *
+
+__all__ = ['dumps', 'loads']
+
+JSON_SCRIPTSAFE_MAPPER = {
+    '&': r'\u0026',
+    '<': r'\u003c',
+    '>': r'\u003e',
+    '\u2028': r'\u2028',
+    '\u2029': r'\u2029'
+}
+
+def dumps(*args, **kwargs):
+    """ JSON used as JS in HTML (script tags) is problematic: <script>
+    tags are a special context which only waits for </script> but doesn't
+    interpret anything else, this means standard htmlescaping does not
+    work (it breaks double quotes, and e.g. `<` will become `&lt;` *in
+    the resulting JSON/JS* not just inside the page).
+
+    However, failing to escape embedded json means the json strings could
+    contains `</script>` and thus become XSS vector.
+
+    The solution turns out to be very simple: use JSON-level unicode
+    escapes for HTML-unsafe characters (e.g. "<" -> "\u003C". This removes
+    the XSS issue without breaking the json, and there is no difference to
+    the end result once it's been parsed back from JSON. So it will work
+    properly even for HTML attributes or raw text.
+
+    Also handle U+2028 and U+2029 the same way just in case as these are
+    interpreted as newlines in javascript but not in JSON, which could
+    lead to oddities and issues.
+
+    .. warning::
+
+        except inside <script> elements, this should be escaped following
+        the normal rules of the containing format
+
+    Cf https://code.djangoproject.com/ticket/17419#comment:27
+    """
+    # replacement can be done straight in the serialised JSON as the
+    # problematic characters are not JSON metacharacters (and can thus
+    # only occur in strings)
+    return re.sub(
+        r'[<>&\u2028\u2029]',
+        lambda m: JSON_SCRIPTSAFE_MAPPER[m[0]],
+        json_.dumps(*args, **kwargs),
+    )