Repository: CTFd/CTFd
Pull request #534 (2017-12-20 04:03:16)

Sandbox themes
Themes are now inside of the Jinja [SandboxEnvironment](http://jinja.pocoo.org/docs/2.10/sandbox/)

`get_config()` can no longer access app.config values.
`get_app_config()` is now used for that purpose if you don't want to just access them directly with `app.config`

codecov[bot]: # [Codecov](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=h1) Report
> Merging [#534](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=desc) into [master](https://codecov.io/gh/CTFd/CTFd/commit/4772c63a8e19f4a8be0d9a22d18e1cb313d61270?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/CTFd/CTFd/pull/534/graphs/tree.svg?src=pr&width=650&token=IfPqkWkt9A&height=150)](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   72.21%   72.28%   +0.07%     
==========================================
  Files          18       18              
  Lines        2663     2659       -4     
==========================================
- Hits         1923     1922       -1     
+ Misses        740      737       -3
```


| [Impacted Files](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [CTFd/utils.py](https://codecov.io/gh/CTFd/CTFd/pull/534/diff?src=pr&el=tree#diff-Q1RGZC91dGlscy5weQ==) | `83.02% <100%> (+0.29%)` | :arrow_up: |
| [CTFd/\_\_init\_\_.py](https://codecov.io/gh/CTFd/CTFd/pull/534/diff?src=pr&el=tree#diff-Q1RGZC9fX2luaXRfXy5weQ==) | `84.9% <100%> (+0.43%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=footer). Last update [4772c63...2224000](https://codecov.io/gh/CTFd/CTFd/pull/534?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CTFd/__init__.py b/CTFd/__init__.py
index 65f31dbe..43665607 100644
--- a/CTFd/__init__.py
+++ b/CTFd/__init__.py
@@ -3,7 +3,8 @@ import os
 
 from distutils.version import StrictVersion
 from flask import Flask
-from jinja2 import FileSystemLoader
+from jinja2 import FileSystemLoader, select_autoescape
+from jinja2.sandbox import SandboxedEnvironment
 from sqlalchemy.engine.url import make_url
 from sqlalchemy.exc import OperationalError, ProgrammingError
 from sqlalchemy_utils import database_exists, create_database
@@ -63,7 +64,12 @@ def create_app(config='CTFd.config.Config'):
     app = Flask(__name__)
     with app.app_context():
         app.config.from_object(config)
-        app.jinja_loader = ThemeLoader(os.path.join(app.root_path, 'themes'), followlinks=True)
+        theme_loader = ThemeLoader(os.path.join(app.root_path, 'themes'), followlinks=True)
+        app.jinja_env = SandboxedEnvironment(
+            loader=theme_loader,
+            autoescape=select_autoescape(['html', 'xml'])
+        )
+        app.jinja_loader = theme_loader
 
         from CTFd.models import db, Teams, Solves, Challenges, WrongKeys, Keys, Tags, Files, Tracking
 
diff --git a/CTFd/utils.py b/CTFd/utils.py
index ca015c27..d08a5297 100644
--- a/CTFd/utils.py
+++ b/CTFd/utils.py
@@ -516,18 +516,13 @@ def delete_file(file_id):
 
 
 @cache.memoize()
-def get_config(key):
+def get_app_config(key):
     value = app.config.get(key)
-    if value:
-        if value and value.isdigit():
-            return int(value)
-        elif value and isinstance(value, six.string_types):
-            if value.lower() == 'true':
-                return True
-            elif value.lower() == 'false':
-                return False
-            else:
-                return value
+    return value
+
+
+@cache.memoize()
+def get_config(key):
     config = Config.query.filter_by(key=key).first()
     if config and config.value:
         value = config.value
@@ -774,7 +769,7 @@ def update_check(force=False):
 
 
 def export_ctf(segments=None):
-    db = dataset.connect(get_config('SQLALCHEMY_DATABASE_URI'))
+    db = dataset.connect(get_app_config('SQLALCHEMY_DATABASE_URI'))
     if segments is None:
         segments = ['challenges', 'teams', 'both', 'metadata']
 
@@ -826,7 +821,7 @@ def export_ctf(segments=None):
         backup_zip.writestr('db/alembic_version.json', result_file.read())
 
     # Backup uploads
-    upload_folder = os.path.join(os.path.normpath(app.root_path), get_config('UPLOAD_FOLDER'))
+    upload_folder = os.path.join(os.path.normpath(app.root_path), app.config.get('UPLOAD_FOLDER'))
     for root, dirs, files in os.walk(upload_folder):
         for file in files:
             parent_dir = os.path.basename(root)
@@ -838,7 +833,7 @@ def export_ctf(segments=None):
 
 
 def import_ctf(backup, segments=None, erase=False):
-    side_db = dataset.connect(get_config('SQLALCHEMY_DATABASE_URI'))
+    side_db = dataset.connect(get_app_config('SQLALCHEMY_DATABASE_URI'))
     if segments is None:
         segments = ['challenges', 'teams', 'both', 'metadata']
 
@@ -924,7 +919,7 @@ def import_ctf(backup, segments=None, erase=False):
                     entry_id = entry.pop('id', None)
                     # This is a hack to get SQlite to properly accept datetime values from dataset
                     # See Issue #246
-                    if get_config('SQLALCHEMY_DATABASE_URI').startswith('sqlite'):
+                    if get_app_config('SQLALCHEMY_DATABASE_URI').startswith('sqlite'):
                         for k, v in entry.items():
                             if isinstance(v, six.string_types):
                                 match = re.match(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d", v)


Test patch:
diff --git a/tests/test_themes.py b/tests/test_themes.py
new file mode 100644
index 00000000..e4133e3e
--- /dev/null
+++ b/tests/test_themes.py
@@ -0,0 +1,43 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+from tests.helpers import *
+from jinja2.sandbox import SecurityError
+
+
+def test_themes_run_in_sandbox():
+    """Does get_config and set_config work properly"""
+    app = create_ctfd()
+    with app.app_context():
+        try:
+            app.jinja_env.from_string("{{ ().__class__.__bases__[0].__subclasses__()[40]('./test_utils.py').read() }}").render()
+        except SecurityError:
+            pass
+        except Exception as e:
+            raise e
+    destroy_ctfd(app)
+
+
+def test_themes_cant_access_configpy_attributes():
+    """Themes should not be able to access config.py attributes"""
+    app = create_ctfd()
+    with app.app_context():
+        assert app.config['SECRET_KEY'] == 'AAAAAAAAAAAAAAAAAAAA'
+        assert app.jinja_env.from_string("{{ get_config('SECRET_KEY') }}").render() != app.config['SECRET_KEY']
+    destroy_ctfd(app)
+
+
+def test_themes_escape_html():
+    """Themes should escape XSS properly"""
+    app = create_ctfd()
+    with app.app_context():
+        team = gen_team(app.db, name="<script>alert(1)</script>")
+        team.affiliation = "<script>alert(1)</script>"
+        team.website = "<script>alert(1)</script>"
+        team.country = "<script>alert(1)</script>"
+
+        with app.test_client() as client:
+            r = client.get('/teams')
+            assert r.status_code == 200
+            assert "<script>alert(1)</script>" not in r.get_data(as_text=True)
+    destroy_ctfd(app)