Skip to content

fix: add pre-read guard to limitedReader to prevent state contamination#3033

Open
Kunall7890 wants to merge 1 commit into
labstack:masterfrom
Kunall7890:fix-body-limit-contamination
Open

fix: add pre-read guard to limitedReader to prevent state contamination#3033
Kunall7890 wants to merge 1 commit into
labstack:masterfrom
Kunall7890:fix-body-limit-contamination

Conversation

@Kunall7890

Copy link
Copy Markdown

What

Adds a strict pre-read guard to \limitedReader.Read\ that checks whether the read counter (
.read) already exceeds \LimitBytes\ before delegating to the underlying reader.

Why

The \limitedReader\ is reused via \sync.Pool\ in \BodyLimitWithConfig. While \Reset\ clears the counter, a contaminated \limitedReader\ retrieved from the pool could delegate reads to the underlying body without proper rejection if:

  1. \Reset\ is not called before \Read\ (defensive against future refactoring)
  2. A partial read sequence leaves
    .read\ at an intermediate value that still exceeds the limit

The post-read guard alone is insufficient because it only catches the overflow after it happens. The pre-read guard provides defense-in-depth: if
.read\ is already beyond the limit, fail immediately without touching the underlying stream.

Changes

\\diff
-func (r *limitedReader) Read(b []byte) (n int, err error) {

  • n, err = r.reader.Read(b)
    +func (r *limitedReader) Read(p []byte) (n int, err error) {
  • if r.read > r.LimitBytes {
  •   return 0, echo.ErrStatusRequestEntityTooLarge
    
  • }
  • n, err = r.reader.Read(p)
    r.read += int64(n)
    if r.read > r.LimitBytes {
    return n, echo.ErrStatusRequestEntityTooLarge
    }
    return
    }
    \\

Closes #1

When limitedReader is reused via sync.Pool, the read counter (r.read)
may retain a stale value if Reset is not called or is called after a
partial read. Add a strict pre-read guard that rejects immediately if
r.read already exceeds LimitBytes, preventing delegation to the
underlying reader with a contaminated counter.

Closes labstack#1
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.

Sync pool resetting context

1 participant