Repository: incubator-airflow Updated Branches: refs/heads/master b55f41f2c -> daa281c03
[AIRFLOW-1007] Use Jinja sandbox for chart_data endpoint Right now, users can put in arbitrary strings into the chart_data endpoint, and execute arbitrary code using the chart_data endpoint. By using literal_eval and ImmutableSandboxedEnvironment, we can reduce RCE. Right now, users can put in arbitrary strings into the chart_data endpoint, and execute arbitrary code using the chart_data endpoint. By using literal_eval and ImmutableSandboxedEnvironment, we can prevent RCE. Dear Airflow maintainers, Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below! ### JIRA - [x] My PR addresses the following [Airflow JIRA] (https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR" - https://issues.apache.org/jira/browse/AIRFLOW-1007 ### Description - [x] I changed Jinja to use the ImmutableSandboxedEnvironment, and used literal_eval, to limit the amount of RCE. ### Tests - [x] My PR adds the following unit tests: SecurityTest chart_data tests ### Commits - [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git- commit/)": 1. Subject is separated from body by a blank line 2. Subject is limited to 50 characters 3. Subject does not end with a period 4. Subject uses the imperative mood ("add", not "adding") 5. Body wraps at 72 characters 6. Body explains "what" and "why", not "how" to: aoen plypaul artwr bolkedebruin Closes #2184 from saguziel/aguziel-jinja-2 Project: http://git-wip-us.apache.org/repos/asf/incubator-airflow/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-airflow/commit/daa281c0 Tree: http://git-wip-us.apache.org/repos/asf/incubator-airflow/tree/daa281c0 Diff: http://git-wip-us.apache.org/repos/asf/incubator-airflow/diff/daa281c0 Branch: refs/heads/master Commit: daa281c0364609d6812921123cf47e4118b40484 Parents: b55f41f Author: Alex Guziel <alex.guz...@airbnb.com> Authored: Mon Apr 3 12:16:00 2017 -0700 Committer: Arthur Wiedmer <arthur.wied...@gmail.com> Committed: Mon Apr 3 12:16:00 2017 -0700 ---------------------------------------------------------------------- airflow/www/views.py | 7 ++++--- tests/core.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-airflow/blob/daa281c0/airflow/www/views.py ---------------------------------------------------------------------- diff --git a/airflow/www/views.py b/airflow/www/views.py index 0def0a9..a9bab31 100644 --- a/airflow/www/views.py +++ b/airflow/www/views.py @@ -43,7 +43,7 @@ from flask_admin.tools import iterdecode from flask_login import flash from flask._compat import PY2 -import jinja2 +from jinja2.sandbox import ImmutableSandboxedEnvironment import markdown import nvd3 @@ -328,8 +328,9 @@ class Airflow(BaseView): request_dict = {k: request.args.get(k) for k in request.args} args.update(request_dict) args['macros'] = macros - sql = jinja2.Template(chart.sql).render(**args) - label = jinja2.Template(chart.label).render(**args) + sandbox = ImmutableSandboxedEnvironment() + sql = sandbox.from_string(chart.sql).render(**args) + label = sandbox.from_string(chart.label).render(**args) payload['sql_html'] = Markup(highlight( sql, lexers.SqlLexer(), # Lexer call http://git-wip-us.apache.org/repos/asf/incubator-airflow/blob/daa281c0/tests/core.py ---------------------------------------------------------------------- diff --git a/tests/core.py b/tests/core.py index bd52d19..997bb42 100644 --- a/tests/core.py +++ b/tests/core.py @@ -63,6 +63,8 @@ from airflow.utils.logging import LoggingMixin from lxml import html from airflow.exceptions import AirflowException from airflow.configuration import AirflowConfigException, run_command +from jinja2.sandbox import SecurityError +from jinja2 import UndefinedError import six @@ -1469,6 +1471,42 @@ class SecurityTests(unittest.TestCase): response = self.app.get("/admin/log", follow_redirects=True) self.assertIn(bleach.clean("<script>alert(123456)</script>"), response.data.decode('UTF-8')) + def test_chart_data_template(self): + """Protect chart_data from being able to do RCE.""" + session = settings.Session() + Chart = models.Chart + chart1 = Chart( + label='insecure_chart', + conn_id='airflow_db', + chart_type='bar', + sql="SELECT {{ ''.__class__.__mro__[1].__subclasses__() }}" + ) + chart2 = Chart( + label="{{ ''.__class__.__mro__[1].__subclasses__() }}", + conn_id='airflow_db', + chart_type='bar', + sql="SELECT 1" + ) + chart3 = Chart( + label="{{ subprocess.check_output('ls') }}", + conn_id='airflow_db', + chart_type='bar', + sql="SELECT 1" + ) + session.add(chart1) + session.add(chart2) + session.add(chart3) + session.commit() + chart1_id = session.query(Chart).filter(Chart.label=='insecure_chart').first().id + with self.assertRaises(SecurityError): + response = self.app.get("/admin/airflow/chart_data?chart_id={}".format(chart1_id)) + chart2_id = session.query(Chart).filter(Chart.label=="{{ ''.__class__.__mro__[1].__subclasses__() }}").first().id + with self.assertRaises(SecurityError): + response = self.app.get("/admin/airflow/chart_data?chart_id={}".format(chart2_id)) + chart3_id = session.query(Chart).filter(Chart.label=="{{ subprocess.check_output('ls') }}").first().id + with self.assertRaises(UndefinedError): + response = self.app.get("/admin/airflow/chart_data?chart_id={}".format(chart3_id)) + def tearDown(self): configuration.conf.set("webserver", "expose_config", "False") self.dag_bash.clear(start_date=DEFAULT_DATE, end_date=datetime.now())
