Skip to content
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

Operator Precedence bug in dns.c #7

Open
nigeltao opened this issue Nov 16, 2017 · 1 comment
Open

Operator Precedence bug in dns.c #7

nigeltao opened this issue Nov 16, 2017 · 1 comment

Comments

@nigeltao
Copy link

nigeltao commented Nov 16, 2017

In https://github.com/jbangert/nail/blob/master/examples/dns/dns.c there is this snippet:

pos = highbyte & 63 << 8 | current->data[pos];

or

pos = highbyte & 63 << 8 | etc;

In C, the operator precedence is <<, then &, then |, so that the equivalent parenthesized expression is:

pos = (highbyte & (63 << 8)) | etc;

Folding the 63<<8 constant gives:

pos = (highbyte & 0x3F00) | etc;

highbyte is a uint8_t, obviously in the range [0x00, 0xFF], so and'ing it with 0x3F00 will always be zero:

pos = 0 | etc;

which simplifies to:

pos = etc;

which is:

pos = current->data[pos];

which is clearly an error, as it ignores the highbyte value. Presumably the original statement should have been:

pos = (highbyte & 63) << 8 | current->data[pos];

although it's not obvious if that is still semantically correct, for compressed DNS records.

@pictyeye
Copy link

We just ran into the same problem (which led to CPU-intensive never-ending loops) and we believe adding the parenthesis as in the last snippet is the correct way to fix this.

pos = (highbyte & 63) << 8 | current->data[pos];

(line 54 should also be changed similarly)

We also have found other issues with the way DNS compression is handled in dns.c, so we may propose a PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants