Fix various parsing bugs in tcpip #301

Merged
merged 7 commits into from Mar 6, 2017

Conversation

Projects
None yet
2 participants
@talex5
Contributor

talex5 commented Mar 5, 2017

They were checking that the buffer was big enough for the headers and
the payload, but they don't write the payload and there's no reason to
require it to be written to the same buffer.

Also, change the IPv4 marshal functions to take the payload length
rather than the payload itself, so that the caller can provide the
payload in multiple buffers. This also makes it clear that the function
does not actually write the payload.

@talex5

This comment has been minimized.

Show comment
Hide comment
@talex5

talex5 Mar 6, 2017

Contributor

I've found a new more bugs and pushed fixes and tests for them to this branch too:

  • The IP equal function didn't use Cstruct.equal.
  • The IP header length calculation was incorrect when IP options were used.
  • The IP parser didn't check that the buffer was big enough to contain the claimed payload length.
  • TCP make_cstruct didn't include the options in the TCP checksum.
Contributor

talex5 commented Mar 6, 2017

I've found a new more bugs and pushed fixes and tests for them to this branch too:

  • The IP equal function didn't use Cstruct.equal.
  • The IP header length calculation was incorrect when IP options were used.
  • The IP parser didn't check that the buffer was big enough to contain the claimed payload length.
  • TCP make_cstruct didn't include the options in the TCP checksum.

@talex5 talex5 changed the title from Fix length check in TCP and UDP into_cstruct functions to Fix various parsing bugs in tcpip Mar 6, 2017

@talex5

This comment has been minimized.

Show comment
Hide comment
@talex5

talex5 Mar 6, 2017

Contributor

Also, it tried to interpret the TCP payload as additional options, so pushed a fix for that.

Contributor

talex5 commented Mar 6, 2017

Also, it tried to interpret the TCP payload as additional options, so pushed a fix for that.

talex5 added some commits Mar 5, 2017

Fix length check in TCP and UDP into_cstruct functions
They were checking that the buffer was big enough for the headers and
the payload, but they don't write the payload and there's no reason to
require it to be written to the same buffer.

Also, change the IPv4 marshal functions to take the payload length
rather than the payload itself, so that the caller can provide the
payload in multiple buffers. This also makes it clear that the function
does not actually write the payload.
Fix IP header length output when IP options are used
The length is in bytes, not words.
Fix Tcpip_packet.Marshal.make_cstruct when TCP options are used
Options need to be included in the checksum.
@yomimono

This comment has been minimized.

Show comment
Hide comment
@yomimono

yomimono Mar 6, 2017

Member

Thanks for the fixes and especially the tests; this looks great.

Member

yomimono commented Mar 6, 2017

Thanks for the fixes and especially the tests; this looks great.

@yomimono yomimono merged commit 430b917 into mirage:master Mar 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talex5 talex5 deleted the talex5:fix-length-checks branch Mar 14, 2017

@talex5 talex5 restored the talex5:fix-length-checks branch Mar 15, 2017

samoht pushed a commit that referenced this pull request Apr 4, 2017

Merge pull request #301 from talex5/fix-length-checks
Fix various parsing bugs in tcpip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment