fix leading zero padding in ECDSA sig conversion #4
Conversation
| @@ -526,6 +526,15 @@ private static int ecdsaLength(int algorithm) throws SignatureException | |||
| s_src_pos = (byte) (r_src_pos + r_src_len); s_pad = 0; | |||
| len = (byte) (6 + r_src_len + s_src_len); | |||
|
|
|||
| // leading zeroes are forbidden | |||
| if (signature[r_src_pos] == 0) { | |||
There was a problem hiding this comment.
This should actually be a while, in case there's more than one leading zero. Will try to update soon.
|
Hi! I haven't found time to work on this, but even as-is this PR strongly improves the situation. Want to merge it? |
|
I have pasted two zone files at https://gist.github.com/Habbie/972ffa2eddfd14efa7b060049f0073bd. Zone file #1 is rejected by jdnssec-tools master, but accepted by this PR. Zone file #2 is rejected by both master and the current state (the first commit) of this PR. Commits fixing this are coming up. |
|
Two more commits pushed, both from @mind04. The first of these fixes validation of signatures with multiple leading zeroes in their numbers, the second updates the changelog. As far as I'm concerned this really is ready for merge now! |
|
Thanks for this. I think near 100% of the issues I had with the ECDSA implementation were around padding or not when converting to and from the DNSSEC and ASN.1 encoded. Any help on this code is appreciated ;) |
Yes, it's terrible. PowerDNS has had bugs in this area as well - generating signatures that were a byte too short etc. |
'Recently', it appears the ECDSA signature verifier (or some related component) that Java provides has gotten stricter. This patch fixes leading zero padding so it complies fully with the BER/DER spec, thus avoiding spurious validation failures when using a newer Java.
I notice that
convertDSASignaturedoes basically the same but looks entirely different. I did not check that one for correct behaviour.