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

DOH: improve encoding/decoding and update related unit test #4598

Closed
wants to merge 4 commits into from

Conversation

@niallor
Copy link

niallor commented Nov 14, 2019

DOH: improve encoding/decoding and update related unit test

A number of bugs in functions doh_encode() and doh_decode() (see below) are addressed.
Unit test 1655 for doh_encode() is updated with more thorough tests
and proofs of detection.

Bugs in function doh_encode():

  • function can accept excessively long host names and empty labels
    for encoding in the QNAME, resulting in an invalid DNS query message;
  • function incorrectly estimates the expected query length when
    the host name has a trailing dot.

Bug in function doh_decode():

  • DNS RR type DNAME is treated as an error instead of being accepted and ignored.

Reported-by: @niallor

Niall added 3 commits Nov 14, 2019
increased strictness of QNAME-encoding, adding error detection
for empty labels and names longer than the overall limit;
avoided treating DNAME as unexpected;
Copy link
Member

bagder left a comment

I think you could've made the fixes for the encoder and the decoder in separate fixes, but I won't insist on it...

lib/doh.c Outdated
(12 /* header */
+ (hostlen ? (hostlen + ((host[hostlen-1] == '.') ? 0 : 1)) : 0)
+ 1 /* zero-length root label */
+ 4); /* QTYPE, QCLASS */

This comment has been minimized.

Copy link
@bagder

bagder Nov 14, 2019

Member

I don't think this needs to deal with a zero hostlen and I think maybe the handling of the trailing dot can be moved outside of the main formula to make this more readable. Maybe like:

size_t expected_len;
DEBUGASSERT(hostlen);
expected_len = 12 + 1 + hostlen + 4;
if(host[hostlen-1]!='.')
  hostlen++;

This comment has been minimized.

Copy link
@niallor

niallor Nov 15, 2019

Author
  1. If libcurl will never need to query for an RRset at the root, supporting zero hostlen is definitely unnecessary. As I don't know about this, I put it in there, so that this corner case, legitimate from a DNS point of view, would be covered.

    For avoidance of doubt, I'ld like to see either "No, it's really overkill" or "Oops, good catch" from you, if you wouldn't mind.

  2. Not doing all the estimation during initialization certainly improves readability. I had far too much fun with the brackets.

    I think you mean

if(host[hostlen-1]!='.')
  expected_len++;
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 16, 2019

Thanks!

@bagder bagder closed this in b6a53ff Nov 16, 2019
@niallor niallor deleted the niallor:doh-encode-strict branch Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.