-
Notifications
You must be signed in to change notification settings - Fork 278
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
Conversation
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:
|
Relevant |
Note that Maybe |
@@ -449,6 +454,10 @@ class RootServer extends DNSServer { | |||
rd.typeBitmap = TYPE_MAP; | |||
return rr; | |||
} | |||
|
|||
toKEY() { | |||
return hsig.makeKey(this.key); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
A solution is WIP here: chjj/bns#8 |
This PR fixes the problem: chjj/bns#8 I can run The problem now is the signature hash, as |
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
It is possible to reproduce this with a simple change to the 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", |
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. |
chjj/bns@1c49f71 has been merged and published so its possible to rebase this and make progress on it again |
As part of end to end testing the dns server in #265, I wanted to also test out the sig0 implementation.
From the rfc:
The root server does not handle the
KEY
record type and also does not return theKEY
record for theANY
request, as the rfc specifies below.Using the helper function
createKey
built intobns/lib/hsig
, which is a Handshake specific wrapper of sig0, to add theKEY
record to the dns response, it causes an error withdig
but not withdig.js
. The error can be seen here: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-3This 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: