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

dns/server: return KEY record with sig0 key #289

Closed
wants to merge 3 commits into from

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Oct 28, 2019

As part of end to end testing the dns server in #265, I wanted to also test out the sig0 implementation.

From the rfc:

For all transaction SIG(0)s, the signer field MUST be a name of the
originating host and there MUST be a KEY RR at that name with the
public key corresponding to the private key used to calculate the

The root server does not handle the KEY record type and also does not return the KEY record for the ANY request, as the rfc specifies below.

The private keys used in transaction security belong to the host
composing the DNS response message, not to the zone involved.
Request authentication may also involve the private key of the host
or other entity composing the request or of a zone to be affected by
the request or other private keys depending on the request authority
it is sought to establish. The corresponding public key(s) are
normally stored in and retrieved from the DNS for verification as KEY
RRs with a protocol byte of 3 (DNSSEC) or 255 (ANY).

Using the helper function createKey built into bns/lib/hsig, which is a Handshake specific wrapper of sig0, to add the KEY record to the dns response, it causes an error with dig but not with dig.js. The error can be seen here:

;; Got bad packet: bad compression pointer
181 bytes
aa ad 81 a0 00 01 00 01 00 00 00 02 00 00 19 00          ................
01 00 00 19 00 ff 00 00 00 00 00 24 00 00 00 fd          ...........$....
10 49 32 18 1c fe d7 58 41 05 c7 28 cd c0 eb 9a          .I2....XA..(....
f1 e7 ff dc 4a 00 74 3f d4 5e 5d e6 6c ac 76 68          ....J.t?.^].l.vh
00 00 29 10 00 00 00 00 00 00 0c 00 0a 00 08 0a          ..).............
e5 7a e6 ac b0 7b ed 00 00 18 00 ff 00 00 00 00          .z...{..........
00 53 00 00 fd 00 00 00 00 00 5d b6 aa 2d 5d b6          .S........]..-].
01 6d 71 52 00 f7 6b 55 ba 40 14 f4 61 c1 4e 78          .mqR..kU.@..a.Nx
b7 ab 08 ff 3b 43 6b 92 d7 d2 7e 26 13 e5 a9 f7          ....;Ck....&....
b2 01 31 52 b1 1e 33 ee 82 f8 de c0 cb 95 09 c7          ..1R..3.........
02 6f 31 c2 2f 13 00 c4 6c d7 5e f0 f5 0b a1 e9          .o1./...l.^.....
95 10 e0 5d 4c                                           ...]L

This lead me to believe that there was an error in the serialization of the KEYRecord defined here: https://tools.ietf.org/html/rfc2535#section-3

This pull request also allows a node operator to pass along an identity-key when running in memory. This will allow me to rebase this PR on top of #265 and also test the sig0 implementation for each rr type.

TODO:

@tynes
Copy link
Contributor Author

tynes commented Oct 28, 2019

Note that the current implementation uses the same key for sig0 as for the p2p network identity key. Luckily its using secp256k1 which has rfc 6979 implemented in the native code (unlike other forms of ecdsa which rely on OpenSSL), so that could help to prevent the biased nonce attack that allows the attacker to calculate the key with many signatures. It is arbitrary to collect many signatures, since you get one with each dns request. We should really be certain that side channel attacks are not viable if we are going to use the same key for sig0 and for the p2p network. For example on the recently discovered Minerva side channel attack:

Our attack allows for practical recovery of the long-term private key.
We have found implementations which leak the bit-length of the scalar
during scalar multiplication on an elliptic curve. This leakage might
seem minuscule as the bit-length presents a very small amount of
information present in the scalar. However, in the case of ECDSA/EdDSA
signature generation, the leaked bit-length of the random nonce is enough
for full recovery of the private key used after observing a few hundreds
to a few thousands of signatures on known messages, due to the application
of lattice techniques.

@tynes
Copy link
Contributor Author

tynes commented Oct 31, 2019

@tynes
Copy link
Contributor Author

tynes commented Nov 11, 2019

Note that dig.js in bns does not cause this problem. Could this be a bug in dig? h/t @tuxcanfly

Maybe dig doesn't support the KEY rr type, or we could try serving KEY records from unbound or using a different tool than dig.

@@ -449,6 +454,10 @@ class RootServer extends DNSServer {
rd.typeBitmap = TYPE_MAP;
return rr;
}

toKEY() {
return hsig.makeKey(this.key);
Copy link
Contributor

Choose a reason for hiding this comment

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

So this works with dig:

    const rr = hsig.makeKey(this.key);
    rr.data.algorithm = 4;
    return rr;

I suspect the PRIVATEDNS experimental algorithm code is not standardized, so dig parses it as a compression byte and fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, it appears even with this we see:

;; Warning: Message parser reports malformed message packet.

Also, PRIVATEDNS is defined in RFC 4034: https://tools.ietf.org/html/rfc4034 so it should be considered standard. I believe a combination of the warning above and a invalid case in parsing leads to the error described above.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so dig requires that the class in the answer section match the question section. So

rr.class = classes.IN;

solves the warning. The issue with parsing still exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, these are the offending lines:

if (algorithm == DNS_KEYALG_PRIVATEDNS) {
	dns_name_t name;
	dns_decompress_setmethods(dctx, DNS_COMPRESS_NONE);
	dns_name_init(&name, NULL);
	RETERR(dns_name_fromwire(&name, source, dctx, options, target));
}

Is there a spec for how to handle PRIVATEDNS? Not sure what's going on here...

But if we are planning to use a different key other than the brontide key anyway, why not just use a key of the type ECDSAP256SHA256 like KSK and ZSK... that would avoid the experimental PRIVATEDNS case altogether which seems to be under/unspecified.

Copy link
Contributor

@tuxcanfly tuxcanfly Nov 11, 2019

Choose a reason for hiding this comment

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

Alright, so RFC 4034 says:

Algorithm number 253 is reserved for private use and will never be assigned to
a specific algorithm. The public key area in the DNSKEY RR and the signature
area in the RRSIG RR begin with a wire encoded domain name, which MUST NOT be
compressed. The domain name indicates the private algorithm to use, and the
remainder of the public key area is determined by that algorithm. Entities
should only use domain names they control to designate their private
algorithms.

https://tools.ietf.org/html/rfc4034#appendix-A.1.1

So for PRIVATEDNS algorithm, dig actually tries to parse the pubkey part of the KEY record as a algo name string first, followed by the actual pubkey. However, the code that parses this is interleaved with the compression decoding code (even though RFC says name will never be compressed), causing a bad label error if any byte falls in the range 128 - 192.

As a very simple test case to verify this, if you prefix the pubkey with a null byte, you can see that dig is able to parse the result without issues.

@tynes
Copy link
Contributor Author

tynes commented Nov 11, 2019

A solution is WIP here: chjj/bns#8

@tynes
Copy link
Contributor Author

tynes commented Dec 3, 2019

@tuxcanfly

This PR fixes the problem: chjj/bns#8

I can run hsd locally and send KEY dns requests using dig without a problem.

The problem now is the signature hash, as SIG(0) commits to information that may be altered by downstream services such as caches changing the TTL. This is outside the scope of this PR, but we need to come up with a cache friendly signature hashing algorithm for SIG(0) to scale.

@tynes
Copy link
Contributor Author

tynes commented Dec 5, 2019

After closer inspection:

$ dig @127.0.0.1 -p 25350 +short KEY .
;; Warning: Message parser reports malformed message packet.
0 0 253 CXNlY3AyNTZrMQADoyuDmFgAKR0PwlRQ1yVR9pOfmHWm6A5+43JGQmi2 rHU=

A warning still appears. There is a problem trying to base64 --decode the public key when the space is included. When eliminating the space, it does properly decode the prefix.

$ dig @127.0.0.1 -p 25350 +short KEY . | grep -v ';;' | cut -d ' ' -f 4 | base64 --decode | tr -d '\t'
secp256k1+X)TP%Qu~rFBh

It is possible to reproduce this with a simple change to the package.json.

diff --git a/package.json b/package.json
index dd7109da..33209db5 100644
--- a/package.json
+++ b/package.json
@@ -34,7 +34,7 @@
     "blru": "~0.1.6",
     "blst": "~0.1.5",
     "bmutex": "~0.1.6",
-    "bns": "~0.7.0",
+    "bns": "tuxcanfly/bns#privatedns-pubkey",
     "bs32": "~0.1.5",
     "bsert": "~0.0.10",
     "bsip": "~0.1.8",

@tynes
Copy link
Contributor Author

tynes commented Jan 27, 2020

Now that chjj/bns@1c49f71 has been merged, we can make progress on this again once it is published. This PR will allow client validation of SIG(0). Currently it is not possible.

Short term we need to come up with a cache-friendly signature hashing algo for SIG(0) (not commit to the DNS cache malleable data on DNS messages) and long term we need to come up with a modern solution to authenticating DNS messages.

@tynes
Copy link
Contributor Author

tynes commented Aug 7, 2020

chjj/bns@1c49f71 has been merged and published so its possible to rebase this and make progress on it again

@tynes tynes closed this Aug 7, 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.

2 participants