Skip to content

fix(execd): clamp the Range end before computing the length - #2089

Open
acodercat wants to merge 2 commits into
opensandbox-group:mainfrom
acodercat:fix/execd-range-overflow
Open

acodercat wants to merge 2 commits into
opensandbox-group:mainfrom
acodercat:fix/execd-range-overflow

Conversation

@acodercat

@acodercat acodercat commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ParseRange computes length = end - start + 1 from the end value the client sent, and only clamps it to the file size afterwards. A very large end overflows before the clamp runs:

  • bytes=0-9223372036854775807 gives a negative length. The clamp (start+length > size) never fires for a negative value, so the download responds 206 with a broken Content-Range and Content-Length, and
    sends no body.
  • bytes=1-9223372036854775807 gives length = MaxInt64. This time start+length is what overflows, and it gets past the clamp the same way.

Both the regular download and the isolated-session download go through this parser.

The fix clamps the end to size-1 before computing the length, which is also the order net/http uses: r.length = min(j, size-1) - i + 1. I used min instead of an if so ParseRange stays under the
gocognit limit. For any input that didn't overflow before, the result is the same as it used to be.

The parser had a second, identical copy in utils_windows.go that the first commit missed. The second commit moves httpRange and ParseRange into a platform-independent range.go and removes both copies, so Windows
builds get the fix too.

Testing

  • Not run (explain why)
  • Unit tests
  • Integration tests
  • e2e / manual verification

I added four cases to the TestParseRange table: end past size, end at MaxInt64, an end that makes start+length overflow, and a start past size. The two overflow cases fail on main.

I also checked that nothing else changed. I compared the old and new parser on 200k random headers covering all three range forms, multiple ranges, size 0, and values at and around the file size, and they
returned identical results. That comparison test isn't committed.

go test ./pkg/..., go vet ./... and golangci-lint are clean, and darwin/windows build.

Breaking Changes

  • None
  • Yes (describe impact and migration path)

Checklist

  • Linked Issue or clearly described motivation
  • Added/updated docs (if needed)
  • Added/updated tests (if needed)
  • Security impact considered
  • Backward compatibility considered

ParseRange computed length = end - start + 1 from the raw end and only
clamped it to the file size afterwards. A huge end overflows that:
bytes=0-9223372036854775807 gives a negative length, which the clamp
does not catch, so the download answers 206 with a broken
Content-Range and Content-Length and no body. bytes=1-9223372036854775807
overflows start+length instead.

Clamp the end to size-1 first, as net/http does.
@github-actions github-actions Bot added component/execd size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2026

@Pangjiping Pangjiping left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code review: 1 high-severity finding (verified against commit cd454ab).

Comment thread components/execd/pkg/web/controller/utils.go Outdated
utils.go and utils_windows.go each had their own copy of httpRange and
ParseRange, identical apart from the previous fix, which only went into
the non-Windows one. Windows builds still computed the length from the
raw end and could overflow.

The parser has nothing platform specific, so keep one copy in range.go
and drop both duplicates.
@github-actions github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2026

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

component/execd size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants