Skip to content

Commit

Permalink
[AIRFLOW-1007] Use Jinja sandbox for chart_data endpoint
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
saguziel committed Mar 29, 2017
1 parent b55f41f commit 04cacdd
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 3 deletions.
7 changes: 4 additions & 3 deletions airflow/www/views.py
Expand Up @@ -43,7 +43,7 @@
from flask_login import flash
from flask._compat import PY2

import jinja2
from jinja2.sandbox import ImmutableSandboxedEnvironment
import markdown
import nvd3

Expand Down Expand Up @@ -328,8 +328,9 @@ def chart_data(self):
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
Expand Down
38 changes: 38 additions & 0 deletions tests/core.py
Expand Up @@ -63,6 +63,8 @@
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

Expand Down Expand Up @@ -1469,6 +1471,42 @@ def test_xss(self):
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())
Expand Down

0 comments on commit 04cacdd

Please sign in to comment.