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 RootServer resolution of SYNTH and GLUE records #444

Merged
merged 4 commits into from
Jul 30, 2020

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented May 6, 2020

SYNTH4 and SYNTH6 records are very special: they act like NS records that contain their own IP addresses, encoded in base32. They can therefore be resolved without any lookup whatsoever. It's a neat trick to save blockchain space and avoid using useless NS+glue.

This PR fixes three bugs in the hsd RootServer resolution of these records:

1) binet.isIPv4(raw) expects raw to be 16 bytes

The server does not currently map IPv4 to IPv6 addresses before proceeding with response.

Without patch:

$  dig @127.0.0.1 -p 25349 _fs0000g._synth.

[error] (ns) Assertion failed.
    at Object.isMapped (/Users/matthewzipkin/Desktop/work/hsd/node_modules/binet/lib/ip.js:734:3)
    at Object.isIPv4 (/Users/matthewzipkin/Desktop/work/hsd/node_modules/binet/lib/ip.js:745:16)
    at RootServer.response (/Users/matthewzipkin/Desktop/work/hsd/lib/dns/server.js:303:14)
    at RootServer.resolve (/Users/matthewzipkin/Desktop/work/hsd/lib/dns/server.js:398:28)
    at RootServer.answer (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bns/lib/server/dns.js:249:28)
    at RootServer.handle (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bns/lib/server/dns.js:316:24)
    at Server.<anonymous> (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bns/lib/server/dns.js:72:20)
    at Server.emit (events.js:210:5)
    at Socket.<anonymous> (/Users/matthewzipkin/Desktop/work/hsd/node_modules/bns/lib/internal/net.js:73:12)
    at Socket.emit (events.js:210:5)

2) cache does not properly detect the _synth. pseudo-TLD

The root server caches the first _synth. response and returns the same answer for all further queries, even though the design of the synth record is to decode the "subdomain" every time the pseudo-TLD is resolved:

Without patch:

$ dig @127.0.0.1 -p 25349 _fs0000g._synth.
...
;; ANSWER SECTION:
_fs0000g._synth.        21600   IN      A       127.0.0.2
...

$ dig @127.0.0.1 -p 25349 _00000000000000000000000008._synth.
...
;; ANSWER SECTION:
_fs0000g._synth.        21600   IN      A       127.0.0.2
...

3) glue is missing from the "additional" section of the answer

Expected result:

$ dig @127.0.0.1 -p 25349 proofofconcept

; <<>> DiG 9.14.6 <<>> @127.0.0.1 -p 25349 proofofconcept
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 47707
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 3
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;proofofconcept.                        IN      A

;; AUTHORITY SECTION:
proofofconcept.         21600   IN      NS      _hpen718._synth.

;; ADDITIONAL SECTION:
_hpen718._synth.        21600   IN      A       142.93.115.133

;; SIG0 PSEUDOSECTION:
.                       0       ANY     SIG     0 253 0 0 20200507001656 20200506121656 64595 . culJQ/ASg95kPVG3uqYQpbUkU+uIcDlXRDb5mXImmjQCgG21C+CzRPtT J7KuWP/4k7XqSrQLZqh11cPR+coS7Q==

;; Query time: 15 msec
;; SERVER: 127.0.0.1#25349(127.0.0.1)
;; WHEN: Wed May 06 14:16:56 EDT 2020
;; MSG SIZE  rcvd: 182

@pinheadmz pinheadmz force-pushed the synthfix1 branch 3 times, most recently from cf0ac80 to d09354f Compare May 6, 2020 18:35
lib/dns/resource.js Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member Author

rebase to bb292d8

I realized the isSubdomain() logic that I removed for SYNTH records should be removed for GLUE records as well.

Consider the name test-glue4 with this resource:

$ hsd-rpc getnameresource test-glue4
{
  "records": [
    {
      "type": "GLUE4",
      "ns": "ns2.hns.",
      "address": "10.20.30.40"
    }
  ]
}

Without the (newly force-pushed) patch, we wouldn't get the additional section we need to resolve the name:

dig @127.0.0.1 -p 33333 test-glue4

; <<>> DiG 9.14.6 <<>> @127.0.0.1 -p 33333 test-glue4
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 46932
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 3
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;test-glue4.                    IN      A

;; AUTHORITY SECTION:
test-glue4.             21600   IN      NS      ns2.hns.

;; ADDITIONAL SECTION:
ns2.hns.                21600   IN      A       10.20.30.40

;; SIG0 PSEUDOSECTION:
.                       0       ANY     SIG     0 253 0 0 20200512224049 20200512104049 64595 . MqFdNYMPTib07LNVFP+IHsQGv0EcpwARCe5/dWJj4m0vajyNTWwNegF/ bNyBh2GPLmver0TWGGC3ZI3aof5uyA==

;; Query time: 14 msec
;; SERVER: 127.0.0.1#33333(127.0.0.1)
;; WHEN: Tue May 12 12:40:49 EDT 2020
;; MSG SIZE  rcvd: 170

This is because test-glue4 is NOT a subdomain of hns.
I'm not sure what the logic for SYNTH and GLUE records is supposed to be.

@pinheadmz
Copy link
Member Author

rebase to 837d4cc : fix lint

@codecov-io
Copy link

Codecov Report

Merging #444 into master will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   62.07%   62.21%   +0.13%     
==========================================
  Files         129      129              
  Lines       34851    34849       -2     
  Branches     5921     5920       -1     
==========================================
+ Hits        21635    21680      +45     
+ Misses      13216    13169      -47     
Impacted Files Coverage Δ
lib/dns/resource.js 85.53% <ø> (+0.76%) ⬆️
lib/dns/server.js 44.95% <100.00%> (+11.71%) ⬆️
lib/protocol/consensus.js 87.75% <0.00%> (-1.03%) ⬇️
lib/covenants/rules.js 73.18% <0.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89d3ab4...837d4cc. Read the comment docs.

@pinheadmz pinheadmz changed the title Fix RootServer resolution of SYNTH records Fix RootServer resolution of SYNTH and GLUE records May 13, 2020
@pinheadmz
Copy link
Member Author

pinheadmz commented May 13, 2020

I've looked a little bit more into this and I think again the isSubdomain() rule is unnecessary for our use case, probably for all DNS.

Consider the response below querying org from a root zone server. Notice how there are several NS's returned in the AUTHORITY section, some ARE SUBDOMAINS of the queried name org and some ARE NOT (they are in the info zone).

Regardless if the NS is a subdomain of the queried name or not, if the nameserver knows about glue, it will be returned in the ADDITIONAL section.

Source: this helpful IRC chat

$ dig ns org @k.root-servers.net

; <<>> DiG 9.14.6 <<>> ns org @k.root-servers.net
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 37006
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 6, ADDITIONAL: 13
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;org.				IN	NS

;; AUTHORITY SECTION:
org.			172800	IN	NS	d0.org.afilias-nst.org.
org.			172800	IN	NS	b0.org.afilias-nst.org.
org.			172800	IN	NS	b2.org.afilias-nst.org.
org.			172800	IN	NS	a0.org.afilias-nst.info.
org.			172800	IN	NS	a2.org.afilias-nst.info.
org.			172800	IN	NS	c0.org.afilias-nst.info.

;; ADDITIONAL SECTION:
d0.org.afilias-nst.org.	172800	IN	A	199.19.57.1
c0.org.afilias-nst.info. 172800	IN	A	199.19.53.1
b2.org.afilias-nst.org.	172800	IN	A	199.249.120.1
b0.org.afilias-nst.org.	172800	IN	A	199.19.54.1
a2.org.afilias-nst.info. 172800	IN	A	199.249.112.1
a0.org.afilias-nst.info. 172800	IN	A	199.19.56.1
d0.org.afilias-nst.org.	172800	IN	AAAA	2001:500:f::1
c0.org.afilias-nst.info. 172800	IN	AAAA	2001:500:b::1
b2.org.afilias-nst.org.	172800	IN	AAAA	2001:500:48::1
b0.org.afilias-nst.org.	172800	IN	AAAA	2001:500:c::1
a2.org.afilias-nst.info. 172800	IN	AAAA	2001:500:40::1
a0.org.afilias-nst.info. 172800	IN	AAAA	2001:500:e::1

;; Query time: 34 msec
;; SERVER: 193.0.14.129#53(193.0.14.129)
;; WHEN: Wed May 13 13:12:21 EDT 2020
;; MSG SIZE  rcvd: 437

@pinheadmz
Copy link
Member Author

TODO: do we need to remove isSubdomain() here as well?

hsd/lib/dns/resource.js

Lines 257 to 271 in 89d3ab4

// Add the glue last.
for (const record of this.records) {
switch (record.type) {
case hsTypes.GLUE4:
case hsTypes.GLUE6:
case hsTypes.SYNTH4:
case hsTypes.SYNTH6: {
if (!util.isSubdomain(name, record.ns))
continue;
zone.push(record.toGlue(record.ns, this.ttl));
break;
}
}
}

@chjj
Copy link
Contributor

chjj commented May 26, 2020

@pinheadmz, IIRC we put the isSubdomain behavior in there to protect against DNS cache poisoning. Otherwise if I own chjj., I can add glue for ns1.pinheadmz. and set it to my own malicious nameserver. Assuming the glue for that is cached, when you try to resolve whatever.pinheadmz., it will go through your ns1. nameserver, but your cache has been poisoned with my IP address.

@pinheadmz
Copy link
Member Author

This paper talks a bit about the glue-not-subdomain records:

http://www.nxnsattack.com/shafir2020-nxnsattack-paper.pdf

It's called the Bailiwick Rule:

If the glue record refers to a name server that resides within the domain for which it is a name server (in-bailiwick, for example, ns.example.com as a name server for the example.com zone) the recursive will honor the IP address provided in the glue record associated with ns.example.com and store it in its cache. Otherwise (out-of-bailiwick, for example, ns.example.net as a name server for the example.com zone), it will discard the glue record. Generally, without getting into different variations and implementation details, BIND [11] recursive implementation, which we analyze in this paper, as well as Unbound [15], PowerDNS [1] and Microsoft DNS all discard out-of-bailiwick glue records. Other solutions to eliminate cache poisoning attacks as a result of out-ofbailiwick glue records include DNSSEC - authenticating the authoritative responses by verifying their signature, but these have a low adoption rate.

@pinheadmz
Copy link
Member Author

rebase to 7365b1c:

rebase on master, update test and revert one of the isSubdomain() changes -- we don't add glue when the NS record is outside the queried domain UNLESS it is a SYNTH4/6 record (TLD is _synth) in which case, the glue is computed and added.

@pinheadmz pinheadmz merged commit 91411ea into handshake-org:master Jul 30, 2020
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.

4 participants