From 5ac181b952616a46ca2784e047ea6cacd7937b3c Mon Sep 17 00:00:00 2001 From: Ivan Ivanov Date: Wed, 11 Dec 2024 10:21:23 +0200 Subject: [PATCH] feat: Add validation for creating `PublicKey` from bytes. (#2107) Signed-off-by: Ivan Ivanov --- .../java/com/hedera/hashgraph/sdk/Key.java | 4 +-- .../hedera/hashgraph/sdk/PrivateKeyECDSA.java | 2 +- .../hashgraph/sdk/PrivateKeyED25519.java | 15 ++++---- .../com/hedera/hashgraph/sdk/PublicKey.java | 6 ++-- .../hedera/hashgraph/sdk/PublicKeyECDSA.java | 27 +++++++------- .../hashgraph/sdk/PublicKeyED25519.java | 8 ++++- .../hashgraph/sdk/ECDSAPublicKeyTest.java | 35 +++++++++++++++++++ .../hashgraph/sdk/Ed25519PublicKeyTest.java | 29 +++++++++++++++ .../TokenUpdateIntegrationTest.java | 18 +++++----- 9 files changed, 106 insertions(+), 38 deletions(-) diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/Key.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/Key.java index 234a95943..dbcabbffc 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/Key.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/Key.java @@ -55,13 +55,13 @@ public abstract class Key { static Key fromProtobufKey(com.hedera.hashgraph.sdk.proto.Key key) { switch (key.getKeyCase()) { case ED25519 -> { - return new PublicKeyED25519(key.getEd25519().toByteArray()); + return PublicKeyED25519.fromBytesInternal(key.getEd25519().toByteArray()); } case ECDSA_SECP256K1 -> { if (key.getECDSASecp256K1().size() == 20) { return new EvmAddress(key.getECDSASecp256K1().toByteArray()); } else { - return new PublicKeyECDSA(key.getECDSASecp256K1().toByteArray()); + return PublicKeyECDSA.fromBytesInternal(key.getECDSASecp256K1().toByteArray()); } } case KEYLIST -> { diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyECDSA.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyECDSA.java index 315bda757..aa90d593d 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyECDSA.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyECDSA.java @@ -218,7 +218,7 @@ public PublicKey getPublicKey() { var q = ECDSA_SECP256K1_DOMAIN.getG().multiply(keyData); var publicParams = new ECPublicKeyParameters(q, ECDSA_SECP256K1_DOMAIN); - publicKey = new PublicKeyECDSA(publicParams.getQ().getEncoded(true)); + publicKey = PublicKeyECDSA.fromBytesInternal(publicParams.getQ().getEncoded(true)); return publicKey; } diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyED25519.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyED25519.java index 5cbd61f0c..daa3feb20 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyED25519.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/PrivateKeyED25519.java @@ -20,6 +20,12 @@ package com.hedera.hashgraph.sdk; import com.hedera.hashgraph.sdk.utils.Bip32Utils; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import javax.annotation.Nullable; import org.bouncycastle.asn1.ASN1OctetString; import org.bouncycastle.asn1.DEROctetString; import org.bouncycastle.asn1.pkcs.PrivateKeyInfo; @@ -30,13 +36,6 @@ import org.bouncycastle.crypto.params.KeyParameter; import org.bouncycastle.math.ec.rfc8032.Ed25519; -import javax.annotation.Nullable; -import java.io.IOException; -import java.nio.ByteBuffer; -import java.nio.ByteOrder; -import java.nio.charset.StandardCharsets; -import java.util.Arrays; - /** * Encapsulate the ED25519 private key. */ @@ -221,7 +220,7 @@ public PublicKey getPublicKey() { byte[] publicKeyData = new byte[Ed25519.PUBLIC_KEY_SIZE]; Ed25519.generatePublicKey(keyData, 0, publicKeyData, 0); - publicKey = new PublicKeyED25519(publicKeyData); + publicKey = PublicKeyED25519.fromBytesInternal(publicKeyData); return publicKey; } diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKey.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKey.java index 0b338bcae..d541d5ae0 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKey.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKey.java @@ -43,13 +43,13 @@ public abstract class PublicKey extends Key { public static PublicKey fromBytes(byte[] publicKey) { if (publicKey.length == Ed25519.PUBLIC_KEY_SIZE) { // If this is a 32 byte string, assume an Ed25519 public key - return new PublicKeyED25519(publicKey); + return PublicKeyED25519.fromBytesInternal(publicKey); } else if (publicKey.length == 33) { // compressed 33 byte raw form - return new PublicKeyECDSA(publicKey); + return PublicKeyECDSA.fromBytesInternal(publicKey); } else if (publicKey.length == 65) { // compress the 65 byte form - return new PublicKeyECDSA( + return PublicKeyECDSA.fromBytesInternal( Key.ECDSA_SECP256K1_CURVE.getCurve().decodePoint(publicKey).getEncoded(true) ); } diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyECDSA.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyECDSA.java index 06a50bea4..986a2c142 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyECDSA.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyECDSA.java @@ -19,23 +19,22 @@ */ package com.hedera.hashgraph.sdk; +import static com.hedera.hashgraph.sdk.Crypto.calcKeccak256; + import com.google.protobuf.ByteString; import com.hedera.hashgraph.sdk.proto.SignaturePair; +import java.io.IOException; +import java.math.BigInteger; +import java.util.Arrays; import org.bouncycastle.asn1.x509.AlgorithmIdentifier; import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo; import org.bouncycastle.crypto.params.ECPublicKeyParameters; import org.bouncycastle.crypto.signers.ECDSASigner; -import java.io.IOException; -import java.math.BigInteger; -import java.util.Arrays; - -import static com.hedera.hashgraph.sdk.Crypto.calcKeccak256; - /** * Encapsulate the ECDSA public key. */ -public class PublicKeyECDSA extends PublicKey { +class PublicKeyECDSA extends PublicKey { // Compressed 33 byte form private byte[] keyData; @@ -44,7 +43,7 @@ public class PublicKeyECDSA extends PublicKey { * * @param keyData the byte array representing the key */ - PublicKeyECDSA(byte[] keyData) { + private PublicKeyECDSA(byte[] keyData) { this.keyData = keyData; } @@ -55,14 +54,14 @@ public class PublicKeyECDSA extends PublicKey { * @return the new key */ static PublicKeyECDSA fromBytesInternal(byte[] publicKey) { - if (publicKey.length == 33) { - // compressed 33 byte raw form + // Validate the key if it's not all zero public key, see HIP-540 + if (Arrays.equals(publicKey, new byte[33])) { return new PublicKeyECDSA(publicKey); - } else if (publicKey.length == 65) { - // compress the 65 byte form + } + if (publicKey.length == 33 || publicKey.length == 65) { return new PublicKeyECDSA( - Key.ECDSA_SECP256K1_CURVE.getCurve().decodePoint(publicKey).getEncoded(true) - ); + // compress and validate the key + Key.ECDSA_SECP256K1_CURVE.getCurve().decodePoint(publicKey).getEncoded(true)); } // Assume a DER-encoded public key descriptor diff --git a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyED25519.java b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyED25519.java index 08c8accbb..ca7928aa6 100644 --- a/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyED25519.java +++ b/sdk/src/main/java/com/hedera/hashgraph/sdk/PublicKeyED25519.java @@ -23,6 +23,7 @@ import com.hedera.hashgraph.sdk.proto.SignaturePair; import org.bouncycastle.asn1.x509.AlgorithmIdentifier; import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo; +import org.bouncycastle.crypto.params.Ed25519PublicKeyParameters; import org.bouncycastle.math.ec.rfc8032.Ed25519; import javax.annotation.Nullable; @@ -40,7 +41,7 @@ class PublicKeyED25519 extends PublicKey { * * @param keyData the byte array representing the key */ - PublicKeyED25519(byte[] keyData) { + private PublicKeyED25519(byte[] keyData) { this.keyData = keyData; } @@ -52,6 +53,11 @@ class PublicKeyED25519 extends PublicKey { */ static PublicKeyED25519 fromBytesInternal(byte[] publicKey) { if (publicKey.length == Ed25519.PUBLIC_KEY_SIZE) { + // Validate the key if it's not all zero public key, see HIP-540 + if (!Arrays.equals(publicKey, new byte[32])) { + // Will throw if the key is invalid + new Ed25519PublicKeyParameters(publicKey, 0); + } // If this is a 32 byte string, assume an Ed25519 public key return new PublicKeyED25519(publicKey); } diff --git a/sdk/src/test/java/com/hedera/hashgraph/sdk/ECDSAPublicKeyTest.java b/sdk/src/test/java/com/hedera/hashgraph/sdk/ECDSAPublicKeyTest.java index c7962df1b..3f8ba0fe9 100644 --- a/sdk/src/test/java/com/hedera/hashgraph/sdk/ECDSAPublicKeyTest.java +++ b/sdk/src/test/java/com/hedera/hashgraph/sdk/ECDSAPublicKeyTest.java @@ -27,6 +27,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; public class ECDSAPublicKeyTest { @Test @@ -66,6 +68,39 @@ void keyByteSerialization2() { assertThat(key2Bytes).containsExactly(key1Bytes); } + @Test + void keyByteValidation() { + byte[] invalidKeyECDSA = new byte[33]; + assertDoesNotThrow(() -> PublicKey.fromBytes(invalidKeyECDSA)); + assertDoesNotThrow(() -> PublicKey.fromBytesECDSA(invalidKeyECDSA)); + + byte[] invalidCompressedKey = new byte[]{ + 0x00, + (byte) 0xca, (byte) 0x35, 0x4b, 0x7c, (byte) 0xf4, (byte) 0x87, (byte) 0xd1, (byte) 0xbc, 0x43, + 0x5a, 0x25, 0x66, 0x77, 0x09, (byte) 0xc1, (byte) 0xab, (byte) 0x98, 0x0c, 0x11, 0x4d, + 0x35, (byte) 0x94, (byte) 0xe6, 0x25, (byte) 0x9e, (byte) 0x81, 0x2e, 0x6a, 0x70, 0x3d, + 0x4f, 0x51 + }; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> PublicKey.fromBytesECDSA(invalidCompressedKey)); + + byte[] malformedKey = new byte[]{0x00, 0x01, 0x02}; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> PublicKey.fromBytesECDSA(malformedKey)); + + byte[] validCompressedKey = new byte[]{ + 0x02, // Prefix for compressed keys + (byte) 0xca, (byte) 0x35, 0x4b, 0x7c, (byte) 0xf4, (byte) 0x87, (byte) 0xd1, (byte) 0xbc, 0x43, + 0x5a, 0x25, 0x66, 0x77, 0x09, (byte) 0xc1, (byte) 0xab, (byte) 0x98, 0x0c, 0x1f, 0x4d, + 0x35, (byte) 0x94, (byte) 0xe6, 0x25, (byte) 0x9e, (byte) 0x81, 0x2e, 0x6a, 0x75, 0x3d, + 0x4f, 0x59 + }; + assertDoesNotThrow(() -> PublicKey.fromBytesECDSA(validCompressedKey)); + + byte[] validDERKey = PrivateKey.generateECDSA().getPublicKey().toBytesDER(); + assertDoesNotThrow(() -> PublicKey.fromBytesECDSA(validDERKey)); + } + @Test @DisplayName("public key can be recovered from DER bytes") void keyByteSerialization3() { diff --git a/sdk/src/test/java/com/hedera/hashgraph/sdk/Ed25519PublicKeyTest.java b/sdk/src/test/java/com/hedera/hashgraph/sdk/Ed25519PublicKeyTest.java index ed7fa79e9..ee3a53530 100644 --- a/sdk/src/test/java/com/hedera/hashgraph/sdk/Ed25519PublicKeyTest.java +++ b/sdk/src/test/java/com/hedera/hashgraph/sdk/Ed25519PublicKeyTest.java @@ -29,6 +29,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; class Ed25519PublicKeyTest { private static final String TEST_KEY_STR = "302a300506032b6570032100e0c8ec2758a5879ffac226a13c0c516b799e72e35141a0dd828f94d37988a4b7"; @@ -47,6 +49,33 @@ void verifyTransaction() { assertThat(key.getPublicKey().verifyTransaction(transaction)).isTrue(); } + @Test + void keyByteValidation() { + byte[] invalidKeyED25519 = new byte[32]; + assertDoesNotThrow(() -> PublicKey.fromBytes(invalidKeyED25519)); + assertDoesNotThrow(() -> PublicKey.fromBytesED25519(invalidKeyED25519)); + + byte[] invalidKey = new byte[]{ + 0x00, + (byte) 0xca, (byte) 0x35, 0x4b, 0x7c, (byte) 0xf4, (byte) 0x87, (byte) 0xd1, (byte) 0xbc, 0x43, + 0x5a, 0x25, 0x66, 0x77, 0x09, (byte) 0xc1, (byte) 0xab, (byte) 0x98, 0x0c, 0x11, 0x4d, + 0x35, (byte) 0x94, (byte) 0xe6, 0x25, (byte) 0x9e, (byte) 0x81, 0x2e, 0x6a, 0x70, 0x3d, + 0x4f, 0x51 + }; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> PublicKey.fromBytesED25519(invalidKey)); + + byte[] malformedKey = new byte[]{0x00, 0x01, 0x02}; + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> PublicKey.fromBytesED25519(malformedKey)); + + byte[] validKey = PrivateKey.generateED25519().getPublicKey().toBytes(); + assertDoesNotThrow(() -> PublicKey.fromBytesED25519(validKey)); + + byte[] validDERKey = PrivateKey.generateED25519().getPublicKey().toBytesDER(); + assertDoesNotThrow(() -> PublicKey.fromBytesED25519(validDERKey)); + } + @Test @DisplayName("public key can be recovered from bytes") void keyByteSerialization() { diff --git a/sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/TokenUpdateIntegrationTest.java b/sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/TokenUpdateIntegrationTest.java index ba4dbc433..9e5799c3e 100644 --- a/sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/TokenUpdateIntegrationTest.java +++ b/sdk/src/testIntegration/java/com/hedera/hashgraph/sdk/test/integration/TokenUpdateIntegrationTest.java @@ -2438,8 +2438,8 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThat(tokenInfoBeforeUpdate.feeScheduleKey.toString()).isEqualTo(feeScheduleKey.getPublicKey().toString()); assertThat(tokenInfoBeforeUpdate.metadataKey.toString()).isEqualTo(metadataKey.getPublicKey().toString()); - // This key is truly invalid, as all Ed25519 public keys must be 32 bytes long - var structurallyInvalidKey = PublicKey.fromString("000000000000000000000000000000000000000000000000000000000000000000"); + // invalid ecdsa key + var ecdsaKey = PublicKey.fromBytesECDSA(new byte[33]); // update all of token’s lower-privilege keys // to a structurally invalid key (trying to update keys one by one to check all errors), @@ -2448,7 +2448,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setWipeKey(structurallyInvalidKey) + .setWipeKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(wipeKey) @@ -2459,7 +2459,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setKycKey(structurallyInvalidKey) + .setKycKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(kycKey) @@ -2470,7 +2470,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setFreezeKey(structurallyInvalidKey) + .setFreezeKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(freezeKey) @@ -2481,7 +2481,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setPauseKey(structurallyInvalidKey) + .setPauseKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(pauseKey) @@ -2492,7 +2492,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setSupplyKey(structurallyInvalidKey) + .setSupplyKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(supplyKey) @@ -2503,7 +2503,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setFeeScheduleKey(structurallyInvalidKey) + .setFeeScheduleKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(feeScheduleKey) @@ -2514,7 +2514,7 @@ void cannotUpdateAllLowerPrivilegeKeysWhenUpdatingKeysToStructurallyInvalidKeysS assertThatExceptionOfType(PrecheckStatusException.class).isThrownBy(() -> { new TokenUpdateTransaction() .setTokenId(tokenId) - .setMetadataKey(structurallyInvalidKey) + .setMetadataKey(ecdsaKey) .setKeyVerificationMode(TokenKeyValidation.NO_VALIDATION) .freezeWith(testEnv.client) .sign(metadataKey)