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

Fix root server proofs #76

Closed
wants to merge 21 commits into from
Closed

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Nov 20, 2021

Closes #21
Closes #42 (actually builds off these commits)
Requires (rebased on) #68 for tests

Port of handshake-org/hsd#593

This should cover everything in the hsd PR and a few extra bug fixes in hnsd:

  • Replaces empty zone proofs with minimally covering NSEC records
  • Fix ICANN insecure delegation proofs
  • No referrals for negative DS answers since root zone is authoritative (referral answers must only be for delegated sub-trees).
  • Fix ICANN DS answers since it used to send referrals for all questions
  • NSEC bitmap now shows correct types available for a name like TXT records (if no delegation)
  • Clean up SOA and AA bit handling for handshake and ICANN names
  • Remove DS from root zone it should only exist in parent zone

also:

  • send REFUSED if name contains special characters

Testing:

I launched hsd in regtest mode like so:

hsd --network=regtest  --rs-port=5350 --ns-port=5349 --port=10000 --brontide-port=20000 --listen=true --no-sig0

Then executed my "regtest names" script which runs auctions and adds combinations of DNS records to the root zone:

https://gist.github.com/pinheadmz/49e3fac7d797a99c3a78fb3ca0ddc012

These data are already included in the hnsd tests in test/data/resource_vectors.h but this also gives an easy way to compare side-by-side the answers from hsd vs hnsd.

Build hnsd and connect to hsd like this:

 ./hnsd -s 127.0.0.1:10000

e.g.

# hnsd
dig @127.0.0.1 -p 25349 test-ds DS +dnssec

# hsd
dig @127.0.0.1 -p 5349 test-ds DS +dnssec

@pinheadmz pinheadmz force-pushed the nsec branch 2 times, most recently from 015b29d to e2600b7 Compare November 23, 2021 16:38
@pinheadmz pinheadmz marked this pull request as ready for review November 23, 2021 16:57
@pinheadmz pinheadmz requested a review from buffrr November 23, 2021 16:57
buffrr and others added 3 commits November 26, 2021 10:18
Adds an NSEC record to indicate that we only have NS NSEC RRSIG. This proves that we don't have a DS rr and allows unbound to treat the zone as unsigned.
@pinheadmz pinheadmz force-pushed the nsec branch 2 times, most recently from 55859a7 to b28d783 Compare November 26, 2021 16:13
Copy link
Contributor

@buffrr buffrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work! Only minor issues so far otherwise mostly nits

Packet error when sending an NX label with length 63 +dnssec more details in review comments.

dig @127.0.0.1 -p 5349 dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddda NS +dnssec

;; Got bad packet: FORMERR
516 bytes
e4 16 85 03 00 01 00 00 00 06 00 01 3f 64 64 64          ............?ddd
64 64 64 64 64 64 64 64 64 64 64 64 64 64 64 64          dddddddddddddddd
64 64 64 64 64 64 64 64 64 64 64 64 64 64 64 64          dddddddddddddddd
64 64 64 64 64 64 64 64 64 64 64 64 64 64 64 64          dddddddddddddddd
64 64 64 64 64 64 64 64 64 64 64 61 00 00 02 00          ddddddddddda....
01 3f 64 64 64 64 64 64 64 64 64 64 64 64 64 64          .?dddddddddddddd

Also qname mismatch with dirty names:

dig @127.0.0.1 -p 5349  foobar\\255

;; ;; Question section mismatch: got foobar\000/A/IN

src/resource.c Show resolved Hide resolved
src/resource.c Outdated Show resolved Hide resolved
src/dns.c Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member Author

rebase to a2e546f

  • fixed nit, extra semicolon
  • unrolled byte-code-to-integer loop and added more test vectors

@pinheadmz
Copy link
Member Author

rebase to d5adee6

  • clean up unrolled isdigit() checks
  • revert escaped-byte-code parsing in "is dirty" check, it's not necessary there

@pinheadmz
Copy link
Member Author

Also qname mismatch with dirty names:

dig @127.0.0.1 -p 5349  foobar\\255

;; ;; Question section mismatch: got foobar\000/A/IN

Regarding this, it is actually only an issue if the byte code is \254 or \255 due to these being special internal codes:

hnsd/src/dns.c

Lines 2136 to 2144 in b3e1f6f

// Hack because we're
// using c-strings.
if (b == 0x00)
b = 0xff;
// This allows for double-dots
// too easily in our design.
if (b == 0x2e)
b = 0xfe;

hnsd/src/dns.c

Lines 2276 to 2282 in b3e1f6f

// 0xff -> NUL
if (ch == -1)
ch = '\0';
// 0xfe -> .
if (ch == -2)
ch = '.';

In both cases, hnsd will still send a REFUSED so at least we are protected the same way hsd is. As far as the qname mismatch, I don't know how important that is to get exactly right in this edge case.

I ran a somewhat annoying bash loop:

for i in $(seq -f "%03g" 0 255); do echo $i; dig @127.0.0.1 -p 25349  foobar\\$i +dnssec; done;

...and with this output I verified that /254 and /255 were the only anomalies. Byte codes in the ASCII range get the usual NX response with the new NSEC proofs because our "dirty" test doesn't check for every special character but it does cover slashes, which is the most relevant for us. I'm still trying to figure out what that dirty test is specifically for. I moved it out of name-writing and name-serializing and just use it to test input now when a request is made, I hope that isnt a mistake.

test/data/name_serialization_vectors.h Outdated Show resolved Hide resolved
test/data/resource_vectors.h Outdated Show resolved Hide resolved
src/ns.c Show resolved Hide resolved
src/resource.c Show resolved Hide resolved
src/ns.c Outdated
Comment on lines 373 to 374
char next[HSK_DNS_MAX_NAME] = "\\000.";
memcpy(&next[5], req->name, strlen(req->name));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I be using strcat() for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hsk_rs_log(ns, " why_bogus: %s\n", result->why_bogus);
goto fail;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a check for the CD bit in the request here. If it's set then return the response as is even if bogus.

dig @hnsd-recursive dnssec-failed.org
SERVFAIL
dig @hnsd-recursive dnssec-failed.org +cd
dnssec-failed.org.      7200    IN      A       69.252.80.75

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 5e01487 in PR #86

@pinheadmz pinheadmz force-pushed the nsec branch 9 times, most recently from 9acb1d8 to 430db04 Compare December 22, 2021 13:17
@pinheadmz pinheadmz marked this pull request as draft December 22, 2021 13:39
@pinheadmz
Copy link
Member Author

Converting this to draft: I'm going to break it up into 3-4 smaller PRs to facilitate review ;-)

@pinheadmz
Copy link
Member Author

Closing this monster now, it has been sliced into 4 other PRs. I combined those 4 PRs back in to one branch to make sure all the tests still pass in the end: pinheadmz#4

@pinheadmz pinheadmz closed this Dec 23, 2021
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.

Malformed packet returned when requested type does not exist
3 participants