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

SIG(0) signature should include SIG's RDATA in hash #120

Closed
jannic opened this issue Apr 24, 2017 · 3 comments
Closed

SIG(0) signature should include SIG's RDATA in hash #120

jannic opened this issue Apr 24, 2017 · 3 comments
Labels

Comments

@jannic
Copy link
Contributor

jannic commented Apr 24, 2017

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.

@bluejekyll
Copy link
Member

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 bluejekyll added the bug label Apr 24, 2017
@jannic
Copy link
Contributor Author

jannic commented 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)

@bluejekyll
Copy link
Member

fixed in #122 and #119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants