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

Make ParseLabels stricter #42

Merged
merged 6 commits into from
Feb 15, 2022
Merged

Make ParseLabels stricter #42

merged 6 commits into from
Feb 15, 2022

Conversation

jviide
Copy link
Contributor

@jviide jviide commented Feb 11, 2022

This pull request makes the following additions & changes:

  • Add a test case (TestDecodeDnsAnswer_PathologicalPacket) that demonstrates how the current non-strict version of ParseLabels can be (ab)used with a ~65 kilobyte packet that takes lots of CPU power and multiple gigabytes of memory to parse.
  • Make ParseLabels quite a bit stricter about the input it accepts:
    • Check that there's enough packet data to read before each step, including pointer bytes.
    • Ensure that the maximum total name length is 253 (when counting the separator dots). Note that the code compares to 254 instead of 253, because allowing an imaginary dangling separator dot makes bookkeeping easier.
    • The two most significant bits of label headers bytes must either be both set or be both unset. Return error when encountering a label header byte that doesn't adhere to this rule. This also effectively limits the length of a single label to 63 bytes.
    • Require pointers to always point to a prior name (based on a reading of RFC 1035, section 4.1.4). We also make a point to ensure that the prior name can't overlap already processed labels. These rules turn pointer loops into parsing errors, so the the separate map for tracking such loops can be dropped.
  • Add test cases for the following strictness checks.
  • Fix a corner case where a valid compressed name got suffixed with an extra dot.

The comments in the pathological packet code try to explain the logic behind it, but in short the idea is to create a message with one question and N CNAME answers. The question contains a very long (too long) name composed of chunks of oversized labels that are interleaved to save space. Each chunk is then followed by a burst of pointers that try to duplicate the data as much as possible, the last pointer jumping forwards and continuing decoding. The name is neatly finished with a null byte when we can't fit any more data to the first 16383 bytes that pointers can point to. A quick check seems to indicate that a name that's encoded like this takes somewhere around 700 kilobytes of memory (don't quote me on this).

The question is then followed by as many CNAME answers as we can fit into the remaining message space. Each answer is kept as short as possible and points just to the name in the question, so that the already bloated name gets copied many times in the memory.

In my Docker-based dev environment the test takes multiple gigabytes of memory and times out after 240 seconds or so.

I made the pathological case the first commit in this pull request so that it can be easily tested against the non-strict version of ParseLabels:

git checkout 3811d0e088002883551786e270c4af7f4a2a0cee
go test ./dnsutils -v -run PathologicalPacket

Add errors for too long labels (max length should be 253 when accounting for separator dots) and invalid label length bytes. Rename the infinite loop error to "invalid pointer".
@jviide jviide changed the title Make ParseLabel stricter Make ParseLabels stricter Feb 11, 2022
Just in case, as offset being an int makes it possible.
 * Check that there's enough packet data to read before each step, including pointer bytes.
 * Ensure that the maximum total name length is 253 (when counting the separator dots). Note that the code compares to 254 instead of 253, because allowing an imaginary dangling separator dot makes bookkeeping easier.
 * The two most significant bits of label headers bytes must either be both set or be both unset. Return error when encountering a label header byte that doesn't adhere to this rule. This also effectively limits the length of a single label to 63 bytes.
 * Require pointers to always point to prior data (based on a reading of RFC 1035, section 4.1.4).
 * Fix a corner case where a valid compressed name got suffixed with an extra dot.

The map is not needed for bookkeeping anymore, as all pointers need to point backwards. The pointed-to data is not even allowed to even overlap data that we've already read, ensuring that no loops can occur.
@dmachard
Copy link
Owner

Thanks a lot for your contribution.
I will take a look to your improvement, it seems to be very instructive :)

@dmachard dmachard merged commit ee0427b into dmachard:main Feb 15, 2022
@jviide jviide deleted the strict-labels branch February 15, 2022 22:46
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

Successfully merging this pull request may close these issues.

2 participants