status-im / status-keycard

Our Javacard Implementation for making secure transactions within Status and Ethereum
Apache License 2.0
213 stars 65 forks source link

Signatures sometimes contain 33-byte components #47

Closed alex-miller-0 closed 5 years ago

alex-miller-0 commented 5 years ago

In running the tests, I have noticed that sometimes I get signatures with 33-byte X-components (as opposed to 32-byte ones, which I would expect every time).

Here's an example:

My comments in verifySignResp in KeycardTest.java:

private void verifySignResp(byte[] data, APDUResponse response) throws Exception {
Signature signature = Signature.getInstance("SHA256withECDSA", "BC");
assertEquals(0x9000, response.getSw());
byte[] sig = response.getData();
System.out.println("verify data");
System.out.println(Arrays.toString(sig));
byte[] keyData = extractPublicKeyFromSignature(sig);
sig = extractSignature(sig);
System.out.println("verify sig");
System.out.println(Arrays.toString(sig));

ECParameterSpec ecSpec = ECNamedCurveTable.getParameterSpec("secp256k1");
ECPublicKeySpec cardKeySpec = new ECPublicKeySpec(ecSpec.getCurve().decodePoint(keyData), ecSpec);
ECPublicKey cardKey = (ECPublicKey) KeyFactory.getInstance("ECDSA", "BC").generatePublic(cardKeySpec);

signature.initVerify(cardKey);
assertEquals((SecureChannel.SC_KEY_LENGTH * 2 / 8) + 1, keyData.length);
signature.update(data);
assertTrue(signature.verify(sig));
assertFalse(isMalleable(sig));
}

Produces the following output:

verify data
[-96, -127, -118, -128, 65, 4, 75, -98, 34, 38, 36, -11, 92, 24, -115, 22, -69, -57, 30, -76, -91, 124, -41, -18, -14, -33, 91, -33, 78, 63, 45, -27, -78, -92, 94, 92, 68, -90, -110, -46, -70, 101, -109, 60, 94, -1, 118, 38, -55, 95, -95, -50, 90, 110, 116, 79, -19, 16, -69, 31, -47, -124, 103, -18, 105, 49, -86, 14, -58, -4, 48, 69, 2, 33, 0, -25, 6, -126, -104, -39, 81, 106, -79, -119, -27, 61, 15, -1, 114, -25, 106, -107, -11, -52, -53, 73, 19, 28, 43, 108, -18, 55, 18, 125, 87, 11, 92, 2, 32, 83, 90, -26, -14, -50, 18, -46, -82, 103, 46, -10, 71, -76, 71, -19, 53, 23, -122, -109, -90, -21, -99, 76, 4, 3, -55, 35, 56, -114, -64, 40, 7]
verify sig
[48, 69, 2, 33, 0, -25, 6, -126, -104, -39, 81, 106, -79, -119, -27, 61, 15, -1, 114, -25, 106, -107, -11, -52, -53, 73, 19, 28, 43, 108, -18, 55, 18, 125, 87, 11, 92, 2, 32, 83, 90, -26, -14, -50, 18, -46, -82, 103, 46, -10, 71, -76, 71, -19, 53, 23, -122, -109, -90, -21, -99, 76, 4, 3, -55, 35, 56, -114, -64, 40, 7]

A different output, with the 32-byte component I would expect:

verify data
[-96, -127, -119, -128, 65, 4, 75, -98, 34, 38, 36, -11, 92, 24, -115, 22, -69, -57, 30, -76, -91, 124, -41, -18, -14, -33, 91, -33, 78, 63, 45, -27, -78, -92, 94, 92, 68, -90, -110, -46, -70, 101, -109, 60, 94, -1, 118, 38, -55, 95, -95, -50, 90, 110, 116, 79, -19, 16, -69, 31, -47, -124, 103, -18, 105, 49, -86, 14, -58, -4, 48, 68, 2, 32, 63, 86, -119, 57, 51, -126, 106, 113, 88, 112, -113, -41, -113, -11, 117, -63, -123, -6, -48, -77, -14, 17, 44, -114, 7, 83, -34, 119, 91, 67, 27, 81, 2, 32, 97, -58, -120, 110, 105, 125, -48, -48, 3, -78, -112, 73, 2, -3, -110, -5, 63, 48, -116, -95, 36, -31, -30, 7, -8, 82, -29, -23, 90, 57, 72, -12]
verify sig
[48, 68, 2, 32, 63, 86, -119, 57, 51, -126, 106, 113, 88, 112, -113, -41, -113, -11, 117, -63, -123, -6, -48, -77, -14, 17, 44, -114, 7, 83, -34, 119, 91, 67, 27, 81, 2, 32, 97, -58, -120, 110, 105, 125, -48, -48, 3, -78, -112, 73, 2, -3, -110, -5, 63, 48, -116, -95, 36, -31, -30, 7, -8, 82, -29, -23, 90, 57, 72, -12]

Note:

bitgamma commented 5 years ago

This is expected, since X and Y are ASN.1 encoded signed integers (as per standard) so if the most significant bit is set (i.e: one) a 0 byte is prepended to indicate positive sign. Removing the leading 0 byte converts the number to an unsigned representation. This is done automatically in our Java SDK and is the right strategy if you need 32-byte output (for example in Ethereum transactions).