Skip to content

Commit 8985062

Browse files
avara1986brettlangdon
avara1986
and
brettlangdon
authored
fix(appsec): transfer-encoding chunked requests are dropped [backport 1.16] (#7394)
Backport 4c065aa from #7378 to 1.16. #7377 reverts #7266. This change instead is an attempt at the proper fix needed to support `Transfer-Encoding: chunked` requests in appsec. According to the WSGI spec, `CONTENT_LENGTH` is allowed to be empty or missing. This can be the case when `Transfer-Encoding: chunked` is sent. In those cases we are removing the post data from the request causing errors in the user's application. Example reproduction: ```python from flask import Flask, request app = Flask(__name__) @app.post("/submit/file") def submit_file(): user_file = request.files.get("file") if not user_file: raise Exception("user_file is missing") return "" ``` ```shell # Run without ddtrace/asm $ gunicorn app:app # Send a chunked request without a content-length header $ curl -vL --request POST 'http://localhost:8000/submit/file' \ --header 'Content-Type: multipart/form-data' --header 'Content-Length:' --header 'Transfer-Encoding: chunked' \ -F [email protected] Note: Unnecessary use of -X or --request, POST is already inferred. * Trying 127.0.0.1:8000... * Connected to localhost (127.0.0.1) port 8000 (#0) > POST /submit/file HTTP/1.1 > Host: localhost:8000 > User-Agent: curl/7.88.1 > Accept: */* > Transfer-Encoding: chunked > Content-Type: multipart/form-data; boundary=------------------------d146b48a5158972c > * Signaling end of chunked upload via terminating chunk. < HTTP/1.1 200 OK < Server: gunicorn < Date: Thu, 26 Oct 2023 19:05:51 GMT < Connection: close < Content-Type: text/html; charset=utf-8 < Content-Length: 7 < * Closing connection 0 success% ``` ```shell # Run the same with ddtrace + ASM enabled $ DD_APPSEC_ENABLED=true ddtrace-run gunicorn app:app # Send a chunked request without a content-length header $ curl -vL --request POST 'http://localhost:8000/submit/file' \ --header 'Content-Type: multipart/form-data' --header 'Content-Length:' --header 'Transfer-Encoding: chunked' \ -F [email protected] Note: Unnecessary use of -X or --request, POST is already inferred. * Trying 127.0.0.1:8000... * Connected to localhost (127.0.0.1) port 8000 (#0) > POST /submit/file HTTP/1.1 > Host: localhost:8000 > User-Agent: curl/7.88.1 > Accept: */* > Transfer-Encoding: chunked > Content-Type: multipart/form-data; boundary=------------------------78465a1f7fa07e00 > * Signaling end of chunked upload via terminating chunk. < HTTP/1.1 500 INTERNAL SERVER ERROR < Server: gunicorn < Date: Thu, 26 Oct 2023 19:09:54 GMT < Connection: close < Content-Type: text/html; charset=utf-8 < Content-Length: 265 < <!doctype html> <html lang=en> <title>500 Internal Server Error</title> <h1>Internal Server Error</h1> <p>The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.</p> * Closing connection 0 ``` ```shell # Run with only tracing enabled $ ddtrace-run gunicorn app:app $ curl -vL --request POST 'http://localhost:8000/submit/file' \ --header 'Content-Type: multipart/form-data' --header 'Content-Length:' --header 'Transfer-Encoding: chunked' \ -F [email protected] Note: Unnecessary use of -X or --request, POST is already inferred. * Trying 127.0.0.1:8000... * Connected to localhost (127.0.0.1) port 8000 (#0) > POST /submit/file HTTP/1.1 > Host: localhost:8000 > User-Agent: curl/7.88.1 > Accept: */* > Transfer-Encoding: chunked > Content-Type: multipart/form-data; boundary=------------------------6137a5c31f27950e > * Signaling end of chunked upload via terminating chunk. < HTTP/1.1 200 OK < Server: gunicorn < Date: Thu, 26 Oct 2023 19:10:53 GMT < Connection: close < Content-Type: text/html; charset=utf-8 < Content-Length: 7 < * Closing connection 0 success% ``` Looking over the Werkzeug code for how they handle this, is something that we should replicate here. https://github.com/pallets/werkzeug/blob/f3c803b3ade485a45f12b6d6617595350c0f03e2/src/werkzeug/sansio/utils.py#L140-L159 ## 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] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [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)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly 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) - [x] If this PR 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`. - [x] This PR doesn't touch any of that. Co-authored-by: Brett Langdon <[email protected]>
1 parent f584e07 commit 8985062

25 files changed

+736
-258
lines changed

‎.riot/requirements/11c167a.txt‎

Lines changed: 0 additions & 43 deletions
This file was deleted.

‎.riot/requirements/13790d2.txt‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
#
2+
# This file is autogenerated by pip-compile with Python 3.7
3+
# by the following command:
4+
#
5+
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/13790d2.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
certifi==2023.7.22
10+
cffi==1.15.1
11+
charset-normalizer==3.3.1
12+
click==8.1.7
13+
coverage[toml]==7.2.7
14+
cryptography==41.0.5
15+
exceptiongroup==1.1.3
16+
flask==2.2.5
17+
gunicorn==21.2.0
18+
hypothesis==6.45.0
19+
idna==3.4
20+
importlib-metadata==6.7.0
21+
iniconfig==2.0.0
22+
itsdangerous==2.1.2
23+
jinja2==3.1.2
24+
markupsafe==2.1.3
25+
mock==5.1.0
26+
opentracing==2.4.0
27+
packaging==23.2
28+
pluggy==1.2.0
29+
pycparser==2.21
30+
pycryptodome==3.19.0
31+
pytest==7.4.3
32+
pytest-cov==4.1.0
33+
pytest-mock==3.11.1
34+
requests==2.31.0
35+
six==1.16.0
36+
sortedcontainers==2.4.0
37+
tomli==2.0.1
38+
typing-extensions==4.7.1
39+
urllib3==2.0.7
40+
werkzeug==2.2.3
41+
wheel==0.41.2
42+
zipp==3.15.0

‎.riot/requirements/1483961.txt‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
#
2+
# This file is autogenerated by pip-compile with python 3.9
3+
# To update, run:
4+
#
5+
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/1483961.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
blinker==1.6.3
10+
certifi==2023.7.22
11+
cffi==1.16.0
12+
charset-normalizer==3.3.1
13+
click==8.1.7
14+
coverage[toml]==7.3.2
15+
cryptography==41.0.5
16+
exceptiongroup==1.1.3
17+
flask==3.0.0
18+
gunicorn==21.2.0
19+
hypothesis==6.45.0
20+
idna==3.4
21+
importlib-metadata==6.8.0
22+
iniconfig==2.0.0
23+
itsdangerous==2.1.2
24+
jinja2==3.1.2
25+
markupsafe==2.1.3
26+
mock==5.1.0
27+
opentracing==2.4.0
28+
packaging==23.2
29+
pluggy==1.3.0
30+
pycparser==2.21
31+
pycryptodome==3.19.0
32+
pytest==7.4.3
33+
pytest-cov==4.1.0
34+
pytest-mock==3.12.0
35+
requests==2.31.0
36+
six==1.16.0
37+
sortedcontainers==2.4.0
38+
tomli==2.0.1
39+
urllib3==2.0.7
40+
werkzeug==3.0.1
41+
wheel==0.41.2
42+
zipp==3.17.0

‎.riot/requirements/15bb169.txt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
#
2+
# This file is autogenerated by pip-compile with Python 3.10
3+
# by the following command:
4+
#
5+
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/15bb169.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
blinker==1.6.3
10+
certifi==2023.7.22
11+
cffi==1.16.0
12+
charset-normalizer==3.3.1
13+
click==8.1.7
14+
coverage[toml]==7.3.2
15+
cryptography==41.0.5
16+
exceptiongroup==1.1.3
17+
flask==2.3.3
18+
gunicorn==21.2.0
19+
hypothesis==6.45.0
20+
idna==3.4
21+
iniconfig==2.0.0
22+
itsdangerous==2.1.2
23+
jinja2==3.1.2
24+
markupsafe==2.1.3
25+
mock==5.1.0
26+
opentracing==2.4.0
27+
packaging==23.2
28+
pluggy==1.3.0
29+
pycparser==2.21
30+
pycryptodome==3.19.0
31+
pytest==7.4.3
32+
pytest-cov==4.1.0
33+
pytest-mock==3.12.0
34+
requests==2.31.0
35+
six==1.16.0
36+
sortedcontainers==2.4.0
37+
tomli==2.0.1
38+
urllib3==2.0.7
39+
werkzeug==3.0.1
40+
wheel==0.41.2

‎.riot/requirements/162a088.txt‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#
2+
# This file is autogenerated by pip-compile with python 3.9
3+
# To update, run:
4+
#
5+
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/162a088.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
certifi==2023.7.22
10+
cffi==1.16.0
11+
charset-normalizer==3.3.1
12+
click==7.1.2
13+
coverage[toml]==7.3.2
14+
cryptography==41.0.5
15+
exceptiongroup==1.1.3
16+
flask==1.1.4
17+
gunicorn==21.2.0
18+
hypothesis==6.45.0
19+
idna==3.4
20+
iniconfig==2.0.0
21+
itsdangerous==1.1.0
22+
jinja2==2.11.3
23+
markupsafe==1.1.1
24+
mock==5.1.0
25+
opentracing==2.4.0
26+
packaging==23.2
27+
pluggy==1.3.0
28+
pycparser==2.21
29+
pycryptodome==3.19.0
30+
pytest==7.4.3
31+
pytest-cov==4.1.0
32+
pytest-mock==3.12.0
33+
requests==2.31.0
34+
six==1.16.0
35+
sortedcontainers==2.4.0
36+
tomli==2.0.1
37+
urllib3==2.0.7
38+
werkzeug==1.0.1
39+
wheel==0.41.2

‎.riot/requirements/194b700.txt‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
#
2+
# This file is autogenerated by pip-compile with Python 3.8
3+
# by the following command:
4+
#
5+
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/194b700.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
blinker==1.6.3
10+
certifi==2023.7.22
11+
cffi==1.16.0
12+
charset-normalizer==3.3.1
13+
click==8.1.7
14+
coverage[toml]==7.3.2
15+
cryptography==41.0.5
16+
exceptiongroup==1.1.3
17+
flask==2.3.3
18+
gunicorn==21.2.0
19+
hypothesis==6.45.0
20+
idna==3.4
21+
importlib-metadata==6.8.0
22+
iniconfig==2.0.0
23+
itsdangerous==2.1.2
24+
jinja2==3.1.2
25+
markupsafe==2.1.3
26+
mock==5.1.0
27+
opentracing==2.4.0
28+
packaging==23.2
29+
pluggy==1.3.0
30+
pycparser==2.21
31+
pycryptodome==3.19.0
32+
pytest==7.4.3
33+
pytest-cov==4.1.0
34+
pytest-mock==3.12.0
35+
requests==2.31.0
36+
six==1.16.0
37+
sortedcontainers==2.4.0
38+
tomli==2.0.1
39+
urllib3==2.0.7
40+
werkzeug==3.0.1
41+
wheel==0.41.2
42+
zipp==3.17.0

‎.riot/requirements/1961d96.txt‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#
2+
# This file is autogenerated by pip-compile with Python 3.11
3+
# by the following command:
4+
#
5+
# pip-compile --no-annotate .riot/requirements/1961d96.in
6+
#
7+
astunparse==1.6.3
8+
attrs==23.1.0
9+
blinker==1.6.3
10+
certifi==2023.7.22
11+
cffi==1.16.0
12+
charset-normalizer==3.3.1
13+
click==8.1.7
14+
coverage[toml]==7.3.2
15+
cryptography==41.0.5
16+
flask==2.3.3
17+
gunicorn==21.2.0
18+
hypothesis==6.45.0
19+
idna==3.4
20+
iniconfig==2.0.0
21+
itsdangerous==2.1.2
22+
jinja2==3.1.2
23+
markupsafe==2.1.3
24+
mock==5.1.0
25+
opentracing==2.4.0
26+
packaging==23.2
27+
pluggy==1.3.0
28+
pycparser==2.21
29+
pycryptodome==3.19.0
30+
pytest==7.4.3
31+
pytest-cov==4.1.0
32+
pytest-mock==3.12.0
33+
requests==2.31.0
34+
six==1.16.0
35+
sortedcontainers==2.4.0
36+
urllib3==2.0.7
37+
werkzeug==3.0.1
38+
wheel==0.41.2

‎.riot/requirements/1ba9802.txt‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎.riot/requirements/1f1bd7d.txt‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

0 commit comments

Comments
 (0)