New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-implement UDP checksum on input. #63

Merged
merged 2 commits into from Jul 7, 2017

Conversation

Projects
None yet
2 participants
@haesbaert
Member

haesbaert commented Jul 4, 2017

The unmarshal function doesn't verify checksum, so we have to do it ourselves.
There doesn't seem to be a simple API for verifying IPV4 header checksum in Tcpip.

Can anyone clarify why this is not done by the unmarshal function ?
This seems incredibly error prone, and we should not give an option for the user to keep a packet if the checksum is incorrect imho.

@yomimono

This comment has been minimized.

Show comment
Hide comment
@yomimono

yomimono Jul 5, 2017

Member

True about the IPV4 header checksum; there should be a convenience function for this like there is for TCP and UDP checksums.

In tcpip the original authors decided that it would be better to check the checksum 0 times than to check it more than once, given that it was not possible to know whether it had been checked already. I agree that checking it 0 times is not a good solution.

Member

yomimono commented Jul 5, 2017

True about the IPV4 header checksum; there should be a convenience function for this like there is for TCP and UDP checksums.

In tcpip the original authors decided that it would be better to check the checksum 0 times than to check it more than once, given that it was not possible to know whether it had been checked already. I agree that checking it 0 times is not a good solution.

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Jul 6, 2017

Member

Ack, I'll see if I can come up with something for Tcpip.
Meanwhile, I think this PR is pretty safe, can I get an ok ?

Member

haesbaert commented Jul 6, 2017

Ack, I'll see if I can come up with something for Tcpip.
Meanwhile, I think this PR is pretty safe, can I get an ok ?

@yomimono

It's probably possible to replace Util.guard with something from Rresult, and I think there's an Alcotest.check_raises but I don't remember whether Alcotest is already a dependency. It's not worth adding just for that.

Those are just nits though, I think this is good.

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Jul 6, 2017

Member

Ah thanks, I'll have a look since I'm using this guard idiom extensively in ssh, would be nice if Rresult has something

Member

haesbaert commented Jul 6, 2017

Ah thanks, I'll have a look since I'm using this guard idiom extensively in ssh, would be nice if Rresult has something

haesbaert added some commits Jul 4, 2017

Re-implement UDP checksum on input.
The unmarshal function doesn't verify checksum, so we have to do it ourselves.
There doesn't seem to be a simple API for verifying IPV4 header checksum in Tcpip.

For now we have UDP back at least.
Add a test for checksum.
The test corrupts one byte of the packet, asserts checksum is incorrect, then
restores the old byte and corrupts the next one, repeating until all packet has
been verified.
@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Jul 6, 2017

Member

So I haven't found but found a better assert_error(), meanwhile I converted the length check to use guard, at least it's not so alone anymoar.

Member

haesbaert commented Jul 6, 2017

So I haven't found but found a better assert_error(), meanwhile I converted the length check to use guard, at least it's not so alone anymoar.

@haesbaert haesbaert merged commit 50f0193 into master Jul 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment