From 26f875958053c2756149a53c3e3547d3d673ec09 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 17 Apr 2024 15:04:55 +0000 Subject: [PATCH] fix(iast): duplicated Flask headers [backport 2.8] (#9021) Backport ad6ac08e86e56978a832b09e439664cf24ba50f0 from #9014 to 2.8. Ensure that when tainting the headers of a Flask application, iterating over the headers (i.e., with `headers.items()`) does not duplicate them. ``` >> list(request.headers.items()) ``` Now: ``` [['Host', '0.0.0.0:8000'], ['User-Agent', 'python-requests/2.31.0'], ['Accept-Encoding', 'gzip, deflate, br'], ['Accept', '*/*'], ['Connection', 'keep-alive'], ['Host', '0.0.0.0:8000'], ['User-Agent', 'python-requests/2.31.0'], ['Accept-Encoding', 'gzip, deflate, br'], ['Accept', '*/*'], ['Connection', 'keep-alive']] != [['Host', '0.0.0.0:8000'], ['User-Agent', 'python-requests/2.31.0'], ['Accept-Encoding', 'gzip, deflate, br'], ['Accept', '*/*'], ['Connection', 'keep-alive'] ] ``` With this fix: ``` [ ["Host", "0.0.0.0:8000"], ["User-Agent", "python-requests/2.31.0"], ["Accept-Encoding", "gzip, deflate, br"], ["Accept", "*/*"], ["Connection", "keep-alive"], ] ``` Introduced in: https://github.com/DataDog/dd-trace-py/pull/8452 ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Alberto Vara --- ddtrace/appsec/_iast/_patch.py | 12 +++-- ...k-duplicated-headers-565e6d8ebb75a883.yaml | 5 ++ .../flask_propagation_app.py | 9 ++++ .../flask_propagation_views.py | 47 +++++++++++++++++++ .../flask_taint_sinks_app.py | 4 -- .../appsec/iast_tdd_propagation/test_flask.py | 25 ++++++++++ 6 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/iast-fix-flask-duplicated-headers-565e6d8ebb75a883.yaml create mode 100644 tests/appsec/iast_tdd_propagation/flask_propagation_app.py create mode 100644 tests/appsec/iast_tdd_propagation/flask_propagation_views.py diff --git a/ddtrace/appsec/_iast/_patch.py b/ddtrace/appsec/_iast/_patch.py index 23b295470c1..6f197023202 100644 --- a/ddtrace/appsec/_iast/_patch.py +++ b/ddtrace/appsec/_iast/_patch.py @@ -74,11 +74,13 @@ def if_iast_taint_yield_tuple_for(origins, wrapped, instance, args, kwargs): if not AppSecIastSpanProcessor.is_span_analyzed(): for key, value in wrapped(*args, **kwargs): yield key, value - - for key, value in wrapped(*args, **kwargs): - new_key = taint_pyobject(pyobject=key, source_name=key, source_value=key, source_origin=origins[0]) - new_value = taint_pyobject(pyobject=value, source_name=key, source_value=value, source_origin=origins[1]) - yield new_key, new_value + else: + for key, value in wrapped(*args, **kwargs): + new_key = taint_pyobject(pyobject=key, source_name=key, source_value=key, source_origin=origins[0]) + new_value = taint_pyobject( + pyobject=value, source_name=key, source_value=value, source_origin=origins[1] + ) + yield new_key, new_value else: for key, value in wrapped(*args, **kwargs): diff --git a/releasenotes/notes/iast-fix-flask-duplicated-headers-565e6d8ebb75a883.yaml b/releasenotes/notes/iast-fix-flask-duplicated-headers-565e6d8ebb75a883.yaml new file mode 100644 index 00000000000..4ecc76e6504 --- /dev/null +++ b/releasenotes/notes/iast-fix-flask-duplicated-headers-565e6d8ebb75a883.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Code Security: Ensure that when tainting the headers of a Flask application, iterating over the headers + (i.e., with `headers.items()`) does not duplicate them. \ No newline at end of file diff --git a/tests/appsec/iast_tdd_propagation/flask_propagation_app.py b/tests/appsec/iast_tdd_propagation/flask_propagation_app.py new file mode 100644 index 00000000000..1c1b23f9fbb --- /dev/null +++ b/tests/appsec/iast_tdd_propagation/flask_propagation_app.py @@ -0,0 +1,9 @@ +from flask_propagation_views import create_app + +from ddtrace import auto # noqa: F401 + + +app = create_app() + +if __name__ == "__main__": + app.run(debug=False, port=8000) diff --git a/tests/appsec/iast_tdd_propagation/flask_propagation_views.py b/tests/appsec/iast_tdd_propagation/flask_propagation_views.py new file mode 100644 index 00000000000..0cf9f201d7f --- /dev/null +++ b/tests/appsec/iast_tdd_propagation/flask_propagation_views.py @@ -0,0 +1,47 @@ +import sys + +from flask import Flask +from flask import request + +from ddtrace import tracer +from ddtrace.appsec._iast._taint_tracking import is_pyobject_tainted + + +class ResultResponse: + param = "" + sources = "" + vulnerabilities = "" + + def __init__(self, param): + self.param = param + + def json(self): + return { + "param": self.param, + "sources": self.sources, + "vulnerabilities": self.vulnerabilities, + "params_are_tainted": is_pyobject_tainted(self.param), + } + + +def create_app(): + app = Flask(__name__) + + @app.route("/shutdown") + def shutdown(): + tracer.shutdown() + sys.exit(0) + + @app.route("/") + def index(): + return "OK" + + @app.route("/check-headers") + def check_headers(): + headers = list(request.headers.items()) + + response = ResultResponse(headers) + + return response.json() + + return app diff --git a/tests/appsec/iast_tdd_propagation/flask_taint_sinks_app.py b/tests/appsec/iast_tdd_propagation/flask_taint_sinks_app.py index be5773a1490..b140a953812 100644 --- a/tests/appsec/iast_tdd_propagation/flask_taint_sinks_app.py +++ b/tests/appsec/iast_tdd_propagation/flask_taint_sinks_app.py @@ -1,7 +1,3 @@ -#!/usr/bin/env python3 - -""" This Flask application is imported on tests.appsec.appsec_utils.gunicorn_server -""" from flask_taint_sinks_views import create_app from ddtrace import auto # noqa: F401 diff --git a/tests/appsec/iast_tdd_propagation/test_flask.py b/tests/appsec/iast_tdd_propagation/test_flask.py index 6618f7f684f..78f9db48add 100644 --- a/tests/appsec/iast_tdd_propagation/test_flask.py +++ b/tests/appsec/iast_tdd_propagation/test_flask.py @@ -52,6 +52,7 @@ def test_iast_flask_orm(orm, xfail): def test_iast_flask_weak_cipher(): + """Verify a segmentation fault on pycriptodome and AES""" with flask_server( iast_enabled="true", tracer_enabled="true", @@ -83,3 +84,27 @@ def test_iast_flask_weak_cipher(): assert content["sources"] == "" assert content["vulnerabilities"] == "WEAK_CIPHER" assert content["params_are_tainted"] is True + + +def test_iast_flask_headers(): + """Verify duplicated headers in the request""" + with flask_server( + iast_enabled="true", + tracer_enabled="true", + remote_configuration_enabled="false", + token=None, + # app="tests/appsec/iast_tdd_propagation/flask_propagation_app.py", + app="flask_propagation_app.py", + ) as context: + server_process, client, pid = context + tainted_response = client.get("/check-headers", headers={"Accept-Encoding": "gzip, deflate, br"}) + + assert tainted_response.status_code == 200 + content = json.loads(tainted_response.content) + assert content["param"] == [ + ["Host", "0.0.0.0:8000"], + ["User-Agent", "python-requests/2.31.0"], + ["Accept-Encoding", "gzip, deflate, br"], + ["Accept", "*/*"], + ["Connection", "keep-alive"], + ]