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

Handle leading zeros in (r|s)-parameters of ECDSA signature #17

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 81 additions & 39 deletions org/xbill/DNS/DNSSEC.java
Original file line number Diff line number Diff line change
Expand Up @@ -775,42 +775,86 @@ private static class ECKeyInfo {
return signature;
}

private static byte []
ECDSASignaturefromDNS(byte [] signature, ECKeyInfo keyinfo)
throws DNSSECException, IOException
{
if (signature.length != keyinfo.length * 2)
throw new SignatureVerificationException();

DNSInput in = new DNSInput(signature);
DNSOutput out = new DNSOutput();

byte [] r = in.readByteArray(keyinfo.length);
int rlen = keyinfo.length;
if (r[0] < 0)
rlen++;

byte [] s = in.readByteArray(keyinfo.length);
int slen = keyinfo.length;
if (s[0] < 0)
slen++;

out.writeU8(ASN1_SEQ);
out.writeU8(rlen + slen + 4);

out.writeU8(ASN1_INT);
out.writeU8(rlen);
if (rlen > keyinfo.length)
out.writeU8(0);
out.writeByteArray(r);

out.writeU8(ASN1_INT);
out.writeU8(slen);
if (slen > keyinfo.length)
out.writeU8(0);
out.writeByteArray(s);
/**
*
* Convert a DNS standard ECDSA signature (defined in RFC 6605) into a JCE
* standard ECDSA signature, which is encoded in ASN.1.
*
* The format of the ASN.1 signature is
*
* ASN1_SEQ . seq_length . ASN1_INT . r_length . R . ANS1_INT . s_length . S
*
* where R and S may have a leading zero byte if without it the values would be
* negative.
*
* The format of the DNSSEC signature is just R . S where R and S are both
* exactly "length" bytes.
*
* @param signature
* The binary signature data from an RRSIG record.
* @return signature data that may be used in a JCE Signature object for
* verification purposes.
*/
public static byte[] convertECDSASignature(byte[] signature) {
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Dangling javadoc. Perhaps just use the regular block comment?

* This method is a patch for the problem of leading zeros that keep the the
* public key from being verified even if it is valid
* See: https://github.com/ibauersachs/dnssecjava/issues/14
*
* This patch was obtained from:
* https://github.com/dblacka/jdnssec-tools/pull/4/commits/ca2a9324858b6b5a87fde8ce8a5fb2365c85db35
* and
* https://github.com/gryphius/dnsjava/commit/b4216b35a8386cf47e2c59a875c655d584624d1d
*/
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded extra braces {} ?

byte r_src_pos, r_src_len, r_pad, s_src_pos, s_src_len, s_pad, len;

r_src_len = s_src_len = (byte) (signature.length / 2);
r_src_pos = 0;
r_pad = 0;
s_src_pos = (byte) (r_src_pos + r_src_len);
s_pad = 0;
len = (byte) (6 + r_src_len + s_src_len);

// multiple leading zeroes are forbidden
while (signature[r_src_pos] == 0 && r_src_len > 0) {
r_src_pos++;
r_src_len--;
len--;
}
while (signature[s_src_pos] == 0 && s_src_len > 0) {
s_src_pos++;
s_src_len--;
len--;
}

return out.toByteArray();
// except when they are mandatory
if (r_src_len > 0 && signature[r_src_pos] < 0) {
r_pad = 1;
len++;
}
if (s_src_len > 0 && signature[s_src_pos] < 0) {
s_pad = 1;
len++;
}
byte[] sig = new byte[len];
byte pos = 0;

sig[pos++] = ASN1_SEQ;
sig[pos++] = (byte) (len - 2);
sig[pos++] = ASN1_INT;
sig[pos++] = (byte) (r_src_len + r_pad);
pos += r_pad;
System.arraycopy(signature, r_src_pos, sig, pos, r_src_len);
pos += r_src_len;

sig[pos++] = ASN1_INT;
sig[pos++] = (byte) (s_src_len + s_pad);
pos += s_pad;
System.arraycopy(signature, s_src_pos, sig, pos, s_src_len);

return sig;
}
}

private static byte []
Expand Down Expand Up @@ -869,12 +913,10 @@ private static class ECKeyInfo {
GOST);
break;
case Algorithm.ECDSAP256SHA256:
signature = ECDSASignaturefromDNS(signature,
ECDSA_P256);
signature = convertECDSASignature(signature);
break;
case Algorithm.ECDSAP384SHA384:
signature = ECDSASignaturefromDNS(signature,
ECDSA_P384);
signature = convertECDSASignature(signature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate branches in switch. If both this case and the one above it are to call the same exact method, would it would be cleaner to:

  case Algorithm.ECDSAP256SHA256:
  case Algorithm.ECDSAP384SHA384:
    signature = convertECDSASignature(signature);
    break;

break;
default:
throw new UnsupportedAlgorithmException(alg);
Expand Down