Bounds check the parser against two panics on malformed input - #773
Open
MoJony wants to merge 1 commit into
Open
Bounds check the parser against two panics on malformed input#773MoJony wants to merge 1 commit into
MoJony wants to merge 1 commit into
Conversation
atoi indexed d[0] without a length check, so any empty-valued int field the parser reads back panicked. parseUInt already handled the empty case; atoi did not. extractXMLDataField took its slice bound straight from XMLDataLen(212) with no validation. A large value slices past the buffer and a large enough one overflows int and slices negative. Both are reachable from a peer on the session read goroutine.
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.
Two shapes a peer can put on the wire panic the parser on the session read goroutine, so a single malformed message takes the session down instead of being rejected.
atoiin fix_int.go indexesd[0]with no length check, so any empty-valued int field the parser reads back panics with index out of range.parseUIntalready handled the empty case;atoidid not. Repro:8=FIX.4.2|9=|35=D|34=2|49=TW|56=ISLD|10=000|.extractXMLDataFieldin message.go takes its slice bound straight from XMLDataLen(212) and doesendIndex += dataLen + 1before slicing, with no validation. A large value slices past the buffer, and a large enough one overflows int and slices negative. Repro:8=FIX.4.2|9=20|35=n|34=2|49=CME|56=OAEAAAN|212=999999999|213=<x/>|10=000|, and the same with212=9223372036854775807.Both are now rejected with an error instead. Four table cases added in the existing test-file style, plus an empty-input case on TestFIXInt_Read; each fails with a panic before the patch.
make vet,make build-srcandmake test-ciare clean locally (log/mongo needs a live MongoDB and fails the same way on an unpatched tree).Fixes the parser half of #678. Found by fuzzing an application built on this library.