[GH-ISSUE #120] SIG(0) signature should include SIG's RDATA in hash #59

New issue

Closed

opened 2026-03-07 22:18:26 +03:00 by kerem · 3 comments

kerem commented

2026-03-07 22:18:26 +03:00

Owner

Originally created by @jannic on GitHub (Apr 24, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/120

According to RFC2931, the hash for the signature should be calculated over
data = RDATA | request - SIG(0)
where "RDATA is the RDATA of the SIG(0) being calculated less the signature itself".

When signing DNS requests, AFAICT, trust_dns currently calculates the hash over request - SIG(0), only.

In fn hash_rrset, used to sign signatures over record sets, there is a call to sig::emit_pre_sig to include the SIG(0) RDATA.
But in fn hash_messages, such code is missing.

Originally created by @jannic on GitHub (Apr 24, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/120 According to [RFC2931](https://tools.ietf.org/html/rfc2931#section-3.1), the hash for the signature should be calculated over `data = RDATA | request - SIG(0)` where "RDATA is the RDATA of the SIG(0) being calculated less the signature itself". When signing DNS requests, AFAICT, trust_dns currently calculates the hash over `request - SIG(0)`, only. In `fn hash_rrset`, used to sign signatures over record sets, there is a call to `sig::emit_pre_sig` to include the SIG(0) RDATA. But in `fn hash_messages`, such code is missing.

kerem

2026-03-07 22:18:26 +03:00

closed this issue
added the
bug
label

kerem commented

2026-03-07 22:18:27 +03:00

Author

Owner

@bluejekyll commented on GitHub (Apr 24, 2017):

Thanks for the report. I use the presig in RRSIG creation for DNSSec. I've validated that in real world systems. The SIG0 calculation has always annoyed me, and I haven't had real world systems to validate it. Thank you for this report!

Anyway, for various reasons, most of the code for SIG0 creation is in Message::sign(). That is where this should probably be fixed. The SIG0 rdata creation will need to be reordered to allow for this.

@bluejekyll commented on GitHub (Apr 24, 2017): Thanks for the report. I use the presig in RRSIG creation for DNSSec. I've validated that in real world systems. The SIG0 calculation has always annoyed me, and I haven't had real world systems to validate it. Thank you for this report! Anyway, for various reasons, most of the code for SIG0 creation is in Message::sign(). That is where this should probably be fixed. The SIG0 rdata creation will need to be reordered to allow for this.

kerem commented

2026-03-07 22:18:27 +03:00

Author

Owner

@jannic commented on GitHub (Apr 24, 2017):

Yes, I came up with:

        let pre_sig0 = SIG::new(
            // type covered in SIG(0) is 0 which is what makes this SIG0 vs a standard SIG
            RecordType::NULL,
            signer.algorithm(),
            num_labels,
            // see above, original_ttl is meaningless, The TTL fields SHOULD be zero
            0,
            // recommended time is +5 minutes from now, to prevent timing attacks, 2 is probably good
            expiration_time,
            // current time, this should be UTC
            // unsigned numbers of seconds since the start of 1 January 1970, GMT
            inception_time,
            key_tag,
            // can probably get rid of this clone if the owndership is correct
            signer.signer_name().clone(),
            Vec::new(),
        );
        let signature: Vec<u8> = try!(signer.sign_message(self, &pre_sig0));
        sig0.set_rdata(
            RData::SIG(SIG::new(
                // type covered in SIG(0) is 0 which is what makes this SIG0 vs a standard SIG
                RecordType::NULL,
                signer.algorithm(),
                num_labels,
                // see above, original_ttl is meaningless, The TTL fields SHOULD be zero
                0,
                // recommended time is +5 minutes from now, to prevent timing attacks, 2 is probably good
                expiration_time,
                // current time, this should be UTC
                // unsigned numbers of seconds since the start of 1 January 1970, GMT
                inception_time,
                key_tag,
                // can probably get rid of this clone if the owndership is correct
                signer.signer_name().clone(),
                signature,
            )

... and then forwarding pre_sig0 to fn hash_message where it's needed to calculate the hash.
Unfortunately, it still doesn't work (still getting request has invalid signature: RRSIG failed to verify (BADSIG) from bind)

@jannic commented on GitHub (Apr 24, 2017): Yes, I came up with: ``` let pre_sig0 = SIG::new( // type covered in SIG(0) is 0 which is what makes this SIG0 vs a standard SIG RecordType::NULL, signer.algorithm(), num_labels, // see above, original_ttl is meaningless, The TTL fields SHOULD be zero 0, // recommended time is +5 minutes from now, to prevent timing attacks, 2 is probably good expiration_time, // current time, this should be UTC // unsigned numbers of seconds since the start of 1 January 1970, GMT inception_time, key_tag, // can probably get rid of this clone if the owndership is correct signer.signer_name().clone(), Vec::new(), ); let signature: Vec<u8> = try!(signer.sign_message(self, &pre_sig0)); sig0.set_rdata( RData::SIG(SIG::new( // type covered in SIG(0) is 0 which is what makes this SIG0 vs a standard SIG RecordType::NULL, signer.algorithm(), num_labels, // see above, original_ttl is meaningless, The TTL fields SHOULD be zero 0, // recommended time is +5 minutes from now, to prevent timing attacks, 2 is probably good expiration_time, // current time, this should be UTC // unsigned numbers of seconds since the start of 1 January 1970, GMT inception_time, key_tag, // can probably get rid of this clone if the owndership is correct signer.signer_name().clone(), signature, ) ``` ... and then forwarding `pre_sig0` to `fn hash_message` where it's needed to calculate the hash. Unfortunately, it still doesn't work (still getting `request has invalid signature: RRSIG failed to verify (BADSIG)` from bind)

kerem commented

2026-03-07 22:18:27 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 7, 2017):

fixed in #122 and #119

@bluejekyll commented on GitHub (May 7, 2017): fixed in #122 and #119

kerem referenced this issue

2026-03-16 01:54:17 +03:00

[PR #59] [MERGED] fix for #56 and named startup tests #1213