labstack / labstack/echo

middleware.BodyLimit: a single oversized Read can silently bypass the limit

Aperta
#3,071 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Lingua principale
Go
Stelle
32.7k
Fork
2.8k
Merge medio
9h 35m
PR unite (30g)
6

Descrizione

Tested against Echo v4.15.2, reproduces on Go 1.26 and Go 1.27 alike.

limitedReader.Read forwards the caller's buffer to the underlying reader unbounded, and only checks the cumulative count afterward — critically, it's not sticky: once tripped, it keeps handing back more real data on every subsequent call:

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

io.Reader's own documentation says:

Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.

Any consumer following that documented, sanctioned pattern — including encoding/json.Decoder, which scans for a complete value before checking the trailing read error — can read arbitrarily far past the configured limit, since limitedReader keeps handing back more real data on every subsequent call after the limit has already been crossed.

Minimal reproduction, custom reader

Demonstrates the core defect directly, independent of Go version, no JSON involved: https://go.dev/play/p/vJbhmQqvI0Y

package main

import (
	"bytes"
	"fmt"
	"net/http"
	"net/http/httptest"

	"github.com/labstack/echo/v4"
	"github.com/labstack/echo/v4/middleware"
)

func main() {
	const limit = 5 // bytes
	body := bytes.Repeat([]byte("x"), 10*limit)

	e := echo.New()
	e.POST("/", func(c echo.Context) error {
		buf := make([]byte, 64)
		var total int
		for {
			n, err := c.Request().Body.Read(buf)
			total += n
			if n == 0 {
				break
			}
			// Processing n>0 bytes before treating a non-nil err as fatal
			// is exactly what io.Reader's docs say callers should do.
			_ = err
		}
		fmt.Printf("total bytes read: %d (configured limit: %d)\n", total, limit)
		return c.NoContent(http.StatusOK)
	}, middleware.BodyLimit(fmt.Sprintf("%dB", limit)))

	req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body))
	// Force the content-read path instead of the Content-Length fast path,
	// as with a real request whose size isn't known up front (chunked).
	req.ContentLength = -1
	req.TransferEncoding = []string{"chunked"}
	rec := httptest.NewRecorder()
	e.ServeHTTP(rec, req)
}

Output:

total bytes read: 50 (configured limit: 5)

Every byte of the oversized body was delivered to the caller despite the 5-byte limit.

Encoding/json angle, for context

This is not just contrived — encoding/json.Decoder follows exactly this documented pattern, so BodyLimit + c.Bind() can silently accept oversized JSON bodies too. This is most reliably triggered on Go 1.27, where encoding/json.Decoder is backed by encoding/json/v2 by default, whose buffer-growth curve happens to align the crossing-the-limit read with completion of the value for common payload sizes — but as the reproduction above shows, the underlying issue is not Go-version-specific.

Minimal reproduction, JSON: https://go.dev/play/p/x9suEwdIO2x

Suggested fix

Mirror net/http.MaxBytesReader. Two changes are both required — bounding the read alone is not enough, since a caller that keeps calling Read after getting an error alongside n > 0 can still assemble the full body one small chunk at a time:

  1. Bound the request size. Never ask the underlying reader for more than remaining+1 bytes, so a single Read can never return enough data to both cross the limit and complete a value:

    remaining := r.limit - r.read
    if int64(len(b))-1 > remaining {
        b = b[:remaining+1]
    }
    
  2. Make it sticky. Once the limit is crossed, permanently return (0, err) on every subsequent call — never hand back more real data, no matter how many more times the caller retries:

    func (r *limitedReader) Read(b []byte) (n int, err error) {
        if r.err != nil {
            return 0, r.err
        }
        ... // bound as above, then on overflow: r.err = echo.ErrStatusRequestEntityTooLarge
    }
    

Happy to submit a PR with this change if it's welcome.

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Direzione di ricerca

Inizia dall’implementazione di limitedReader utilizzata dal punto di ingresso middleware.BodyLimit del middleware, quindi riproduci il problema con l’esempio di chunked-request e un reader che restituisce più del limite. Aggiungi una copertura di regressione per le letture di streaming che superano il limite e verifica che non vengano forniti dati dopo il superamento del limite e che l’errore configurato rimanga osservabile.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
go
Ambito
backend
Tipo di issue
Bug
Difficoltà
3/5
Tempo stimato
1-2 giorni
Stato di attività
Attiva
Chiarezza
Specificata chiaramente
Idoneità per principianti
74/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.