Skip to content

Inspect Flask bodies read from the raw stream - #783

Open
iacobdaniel wants to merge 4 commits into
mainfrom
fix/record-raw-body
Open

iacobdaniel wants to merge 4 commits into
mainfrom
fix/record-raw-body

Conversation

@iacobdaniel

Copy link
Copy Markdown
Contributor

Bodies the app reads straight from request.stream or wsgi.input were not inspected. They are now read in full on the app's first read and stored for inspection, and later reads come from that copy.

Comment on lines +93 to +95
body = read_up_to(self._stream, MAX_BUFFERED_BODY_SIZE + 1)
if len(body) > MAX_BUFFERED_BODY_SIZE:
return io.BufferedReader(PrefixedStream(body, self._stream))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium - Chunked raw reads allocate 20 MiB before returning data

For a terminated or chunked request without a declared length (or with a declared length at or below the threshold), the first application read consumes up to 20 MiB plus one byte before returning even one requested byte. An unauthenticated client can keep many streaming requests incomplete, forcing each worker to retain a large prefix and delaying streaming handlers, causing substantial memory and request-availability exhaustion.

Show fix

Do not read a threshold-sized prefix synchronously on the application's first read when the declared length is absent or unreliable. Either leave such streams unbuffered and explicitly mark them unscanned, or spool them with a bounded, configurable mechanism that applies backpressure without retaining one large in-memory prefix per request.

More info - Reply on this comment to give feedback or ignore the issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially fixed in commit 8c0deb3.

This issue remains because BufferedWsgiInput._open_source still calls read_up_to(self._stream, MAX_BUFFERED_BODY_SIZE + 1) at line 97 for terminated streams, and read_up_to merely splits that synchronous read into 64 KiB chunks; it still retains the prefix and delays the first application byte until EOF or the threshold.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is intended: Zen needs the whole body before the app acts on it, otherwise an attack early in a body the app parses while reading would get through. The wait is bounded by the 20 MB limit, memory only grows with the bytes the client sends, and get_json() already waits for and holds the full body the same way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially fixed in commit 524ce13.

This issue remains because BufferedWsgiInput._open_source still calls read_up_to(self._stream, MAX_BUFFERED_BODY_SIZE + 1) at aikido_zen/sources/flask/buffered_wsgi_input.py:109 for terminated/chunked requests, and reads the entire declared length at line 113 when it is at or below the threshold, before returning the first application read; the 64 KiB chunks only reduce per-read allocation, not synchronous prefetch or retained buffering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended, as explained above.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant