Fix integer overflow in parseString bounds check - #262
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
parseString read a uint32 length prefix and then checked len(in) against 4+length. That addition is done in uint32, so a length near the maximum wraps: with length 0xFFFFFFFF, 4+length becomes 3, the guard passes for any buffer of 4+ bytes, and the code slices in[4:4+length] = in[4:3], which panics with a slice-bounds error. This is reachable through a malformed pty-req (parsePtyRequest calls parseString on the term field). There is no recover() in the package and the session handler runs in its own goroutine, so the panic takes down the whole process. On servers with no PublicKeyHandler or PasswordHandler set, gliderlabs enables NoClientAuth, so this is unauthenticated there. Advance past the length prefix before the bounds check and compare len(in) against length directly, matching the shape used in golang.org/x/crypto/ssh. Valid parsing is unchanged. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I do supply-chain security research and found this while auditing SSH server parsers.
parseString reads a uint32 length prefix and then guards with
uint32(len(in)) < 4+length. The4+lengthis evaluated in uint32, so a length close to the max wraps around. Withlength = 0xFFFFFFFF,4+lengthbecomes 3, the check passes for any buffer of four or more bytes, and execution reachesin[4 : 4+length]which isin[4:3]and panics with a slice-bounds error.You can hit this with a malformed pty-req: parsePtyRequest feeds the term field straight into parseString. There is no recover() in the package and DefaultSessionHandler runs in its own goroutine, so the panic brings the whole process down rather than just failing the one connection. When a server sets neither a PublicKeyHandler nor a PasswordHandler, NoClientAuth is enabled, so on those setups an unauthenticated client can trigger it.
The fix advances past the length prefix first and then compares
len(in)againstlengthdirectly, which is the same shape golang.org/x/crypto/ssh uses and avoids the addition entirely. Valid inputs parse exactly as before.Added util_test.go: a normal round-trip case, the 0xFFFFFFFF length case, and a malformed pty-req. The oversized cases panic on master and pass with this change;
go test ./...is green after the fix.