Repository navigation
fix: request limits and input validation - #5651
martinconic wants to merge 12 commits into
Conversation
Stamp.Valid now rejects chunk addresses that are not HashSize long before toBucket reads their first four bytes. Proximity and ExtendedProximity compare operand lengths as int, so lengths that are a multiple of 256 no longer wrap to zero.
Accept values from 0 to 4 MiB, 8x the largest default buffer, and document the range in the OpenAPI spec.
transform advances by the key length, so it now returns ErrInvalidKey for an empty key, and access keys read from an ACT must be encryption.KeyLength long.
Stop the branch size search before the multiplication overflows int64, which left the loop without a reachable exit.
Reject negative values and values above 1000 gwei in the gas middleware and the transaction cancel handler.
Map methods other than the standard HTTP methods to OTHER, so the number of series does not depend on request input.
Set the read limit of /chunks/stream to a stamp plus a maximum-size single owner chunk, the largest valid message.
…CORS Requests other than GET, HEAD and OPTIONS whose Origin is not allowed, or whose Sec-Fetch-Site reports another site that is not in the CORS allow list, get 403. Requests without these headers are unaffected. Origins allowed only by a wildcard no longer get Access-Control-Allow-Credentials.
Serve API responses with X-Content-Type-Options: nosniff, so browsers handle downloaded content as its declared type.
A check of all pins reads every pinned chunk. Respond with 429 while one is running; checks of a single pin are unaffected.
Log each change at info level before it takes effect, so a change that turns logging off is still recorded.
The cross-origin check compared the Origin, scheme included, with http:// plus the request host before looking at Sec-Fetch-Site. Behind a proxy that terminates TLS, same-origin pages send an https origin, so their writes got 403. Let Sec-Fetch-Site decide when present: same-origin and none are allowed, same-site and cross-site only when the origin is allowed by the CORS configuration. Without it, compare the Origin host with the request host, ignoring the scheme.
| // subtrieSize comes from chunk data. Stop before the multiplication | ||
| // overflows: a wrapped branchSize, or a non-positive refs, made the | ||
| // exit condition unsatisfiable. | ||
| if branchSize > math.MaxInt64/branching { |
There was a problem hiding this comment.
This stops the loop, but the function can still return nonsense for the same hostile sizes. branchSize * (refs - 1) on the line above can overflow too, and the last-branch return is computed from it.
If we call SubtrieSection with the sizes from the new test: for subtrieSize = math.MaxInt64 with a full chunk of refs, startIdx=0 returns 2305843009213693952 and the last branch (startIdx=4064) returns -6917529027641081857.
readAtOffset and processChunkAddresses then use sec as a size (cur += sec, currentReadSize := subtrieSpan - (off - cur)), so a negative value gets into the offset arithmetic. I haven't traced whether that leads to a wrong read or just an error further down.
A cheap guard would be to return an error, or ErrMalformedTrie from the callers, when subtrieSize is above what refs branches of branchSize can hold, or when the result is negative. A case in TestSubtrieSectionTerminatesOnLargeSize that also checks the return value would pin it down.
| // one the uploader declared, so content is handled as the declared type. | ||
| func noSniffHandler(h http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("X-Content-Type-Options", "nosniff") |
There was a problem hiding this comment.
Worth checking one interaction: files uploaded in a /bzz collection get their content type from mime.TypeByExtension, so a file without a known extension is stored with an empty type and served with no Content-Type. I checked: a tar with index and app.mjs2 downloads both with Content-Type: \"\", and they now also carry nosniff.\n\nWithout a declared type the browser used to sniff these. With nosniff it shouldn't guess, so extensionless HTML pages or scripts on existing sites may stop rendering or loading. I haven't confirmed exact browser behaviour for an empty type plus nosniff, so treat this as a question. Options: only set nosniff when a Content-Type is present, or fall back to http.DetectContentType on download when the metadata has no type (bzz.go already does that on single-file upload).
| // it decides when present. Otherwise, as with older browsers, the Origin host | ||
| // is compared with the request host. The scheme is ignored in both cases: the | ||
| // API serves plain HTTP, and a TLS-terminating proxy in front of it makes | ||
| // same-origin pages send an https origin. |
There was a problem hiding this comment.
nit: the scheme is ignored here, but checkOrigin below still compares scheme://host. As noted in the review body, same-origin pages behind a TLS proxy pass this write check but still get no CORS headers and fail websocket CheckOrigin. Maybe a TODO, or a follow-up issue, so the two stay in sync.
3 participants
Checklist
Description
A set of fixes for input validation and request limits. Each change is a separate commit with its own tests.
Stamp.Validrejects chunk addresses that are notHashSizelong.ProximityandExtendedProximitycompare operand lengths asint, so lengths that are a multiple of 256 no longer wrap to zero.Swarm-Lookahead-Buffer-Sizeaccepts 0 to 4 MiB (8x the largest default); other values get 400.ErrInvalidKey, and ACT access keys must beencryption.KeyLengthlong.subtrieSectionstops its branch size search before the multiplication overflows.Gas-Pricemust be between 0 and 1000 gwei, in the gas middleware and the transaction cancel handler; other values get 400.response_code_countis one of the standard HTTP methods orOTHER./chunks/streamlimits websocket messages to a stamp plus a maximum-size single owner chunk.Originis not allowed, or theirSec-Fetch-Sitereports another site that is not incors-allowed-origins. Requests without these headers, as sent by non-browser clients, are unaffected. Origins allowed only through*no longer getAccess-Control-Allow-Credentials.X-Content-Type-Options: nosniff.GET /pins/checkwithoutref) runs at a time; others get 429. Checks of a single pin are unaffected.Open API Spec Version Changes (if applicable)
SwarmCommon.yaml:minimum: 0andmaximum: 4194304forswarm-lookahead-buffer-size. Version unchanged.Motivation and Context (Optional)
Behavior changes to be aware of:
cors-allowed-origins.Gas-Priceabove 1000 gwei, andSwarm-Lookahead-Buffer-Sizeoutside 0 to 4 MiB, are rejected.AI Disclosure