Skip to content

Fix bug with allowing duplicate signature in java SubzeroUtils #709

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.

Sign up for GitHub

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

Merged
andozw merged 1 commit into from
Nov 19, 2024
Merged

Fix bug with allowing duplicate signature in java SubzeroUtils #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 16 additions & 4 deletions java/shared/src/main/java/com/squareup/subzero/shared/SubzeroUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.google.common.collect.Lists;
import com.google.common.io.BaseEncoding;
import com.google.protobuf.ByteString;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.zxing.BarcodeFormat;
import com.google.zxing.common.BitMatrix;
Expand All @@ -18,7 +19,9 @@
import java.security.MessageDigest;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.bitcoinj.core.Address;
import org.bitcoinj.core.Coin;
import org.bitcoinj.core.ECKey;
Expand Down Expand Up @@ -130,16 +133,19 @@ protected static List<byte[]> validateAndSort(List<ECKey> pubkeys, byte[] hash,
// This needs to be the same sort that ScriptBuilder.createRedeemScript does.
pubkeys.sort(ECKey.PUBKEY_COMPARATOR);
List<byte[]> sortedSigs = new ArrayList<>();
Set<ByteString> seenSigs = new HashSet<>();

// If this check fails, we've probably got invalid signatures that would fail below, or when
// broadcast. However, this lets us distinguish between invalid signatures and signatures over
// the wrong data. Useful primarily for debugging when making changes to either piece of code.
ByteString expectedHash = ByteString.copyFrom(hash);
for (Signature sig : signatures) {
if (sig.hasHash()) {
if (!Arrays.equals(sig.getHash().toByteArray(), hash)) {
ByteString sigHash = sig.getHash();
if (!sigHash.equals(expectedHash)) {
throw new RuntimeException(format(
"Our calculated hash does not match the HSM provided sig: %s != %s",
Hex.toHexString(sig.getHash().toByteArray()), Hex.toHexString(hash)));
sigHash.toStringUtf8(), expectedHash.toStringUtf8()));
}
}
}
Expand All @@ -149,8 +155,14 @@ protected static List<byte[]> validateAndSort(List<ECKey> pubkeys, byte[] hash,
for(ECKey pubkey: pubkeys) {
for (Signature sig : signatures) {
try {
if (pubkey.verify(hash, sig.getDer().toByteArray())) {
sortedSigs.add(sig.getDer().toByteArray());
byte[] sigDerBytes = sig.getDer().toByteArray();
if (pubkey.verify(hash, sigDerBytes)) {
ByteString sigDerByteString = ByteString.copyFrom(sigDerBytes);
if (!seenSigs.contains(sigDerByteString)) {
// Only add the signature if it is unique
sortedSigs.add(sigDerBytes);
seenSigs.add(sigDerByteString);
}
}
} catch (SignatureDecodeException e) {
// keep going, we'll throw a RuntimeException later if we don't find the right number of
Expand Down
162 changes: 162 additions & 0 deletions java/shared/src/test/java/com/squareup/subzero/shared/SubzeroUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,22 @@
import com.squareup.subzero.proto.service.Common.EncryptedMasterSeed;
import com.squareup.subzero.proto.service.Common.EncryptedPubKey;
import com.squareup.subzero.proto.service.Common.Path;
import com.squareup.subzero.proto.service.Common.Signature;
import com.squareup.subzero.proto.service.Common.TxInput;
import com.squareup.subzero.proto.service.Common.TxOutput;
import com.squareup.subzero.proto.service.Internal.InternalCommandRequest;
import com.squareup.subzero.proto.service.Service.CommandRequest;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import org.bitcoinj.core.Address;
import org.bitcoinj.core.ECKey;
import org.bitcoinj.core.Sha256Hash;
import org.bitcoinj.core.VerificationException;
import org.bitcoinj.crypto.DeterministicKey;
import org.bitcoinj.params.TestNet3Params;
Expand Down Expand Up @@ -494,6 +501,135 @@ public void testValidateSignTxCommandRequest() {
assertTrue(e3.getMessage().contains(ERROR_OUTPUTS_COUNT));
}

@Test
public void testValidateAndSortValidSignatures() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey(), new ECKey());
keys.sort(ECKey.PUBKEY_COMPARATOR);

byte[] hash = sha256Hash("mock input");
List<Signature> signatures = Arrays.asList(
createValidSignature(hash, keys.get(0)),
createValidSignature(hash, keys.get(1))
);

List<byte[]> sortedSigs = SubzeroUtils.validateAndSort(keys, hash, signatures);

assertEquals(Constants.MULTISIG_THRESHOLD, sortedSigs.size());
assertArrayEquals(signatures.get(0).getDer().toByteArray(), sortedSigs.get(0));
assertArrayEquals(signatures.get(1).getDer().toByteArray(), sortedSigs.get(1));
}

@Test
public void testValidateAndSortInvalidSignatures() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey());
byte[] hash = sha256Hash("mock input");
List<Signature> signatures = Arrays.asList(
createInvalidEmptySignature(),
createInvalidEmptySignature()
);

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, signatures)
);

assertTrue(exception.getMessage().contains("Our calculated hash does not match the HSM provided sig"));
}

@Test
public void testValidateAndSortValidSignaturesFlippedBit() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey(), new ECKey());
keys.sort(ECKey.PUBKEY_COMPARATOR);

byte[] hash = sha256Hash("mock input");
List<Signature> signatures = Arrays.asList(
createValidSignature(hash, keys.get(0)),
createValidSignature(hash, keys.get(1))
);

List<byte[]> sortedSigs = SubzeroUtils.validateAndSort(keys, hash, signatures);

assertEquals(Constants.MULTISIG_THRESHOLD, sortedSigs.size());
assertArrayEquals(signatures.get(0).getDer().toByteArray(), sortedSigs.get(0));
assertArrayEquals(signatures.get(1).getDer().toByteArray(), sortedSigs.get(1));

// Flip a bit in one signature and check for failure
byte[] corruptedSigBytes = signatures.get(0).getDer().toByteArray();
corruptedSigBytes[0] ^= 0x01;

// Use the same hash but use the corrupted signature
Signature corruptedSignature = Signature.newBuilder()
.setHash(signatures.get(0).getHash())
.setDer(ByteString.copyFrom(corruptedSigBytes))
.build();

List<Signature> corruptedSignatures = Arrays.asList(corruptedSignature, signatures.get(1));

// Validate and expect a failure
RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, corruptedSignatures)
);

assertTrue(exception.getMessage().contains("Failed validating signatures"));
}

@Test
public void testValidateAndSortSignatureWithWrongHash() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey());
byte[] hash = sha256Hash("input1");
byte[] wrongHash = sha256Hash("input2");
List<Signature> signatures = Arrays.asList(createValidSignature(wrongHash, keys.get(0)));

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, signatures)
);
assertTrue(exception.getMessage().contains("Our calculated hash does not match the HSM provided sig"));
}

@Test
public void testValidateAndSortDuplicateSignatures() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey());
byte[] hash = sha256Hash("input");
Signature validSig = createValidSignature(hash, keys.get(0));
List<Signature> signatures = Arrays.asList(validSig, validSig);

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, signatures)
);
assertTrue(exception.getMessage().contains("Failed validating signatures"));
}

@Test
public void testValidateAndSortDuplicateKey() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey());
byte[] hash = sha256Hash("input");

// Sign twice using the same key rather than using a copied reference to the same signature
// as in testValidateAndSortDuplicateSignatures
Signature validSig1 = createValidSignature(hash, keys.get(0));
Signature validSig2 = createValidSignature(hash, keys.get(0));
List<Signature> signatures = Arrays.asList(validSig1, validSig2);

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, signatures)
);
assertTrue(exception.getMessage().contains("Failed validating signatures"));
}

@Test
public void testValidateAndSortSignaturesBelowThreshold() {
List<ECKey> keys = Arrays.asList(new ECKey(), new ECKey());
byte[] hash = sha256Hash("input");
// Only supply 1 sig
List<Signature> signatures = Arrays.asList(
createValidSignature(hash, keys.get(0))
);

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(keys, hash, signatures)
);
assertTrue(exception.getMessage().contains("Failed validating signatures"));
}

private TxInput testInput() {
return testInput(1000L, ByteString.copyFromUtf8("test prev hash"));
}
Expand Down Expand Up @@ -526,4 +662,30 @@ private TxOutput testOutput(long amount, Destination destination) {
.setIndex(2))
.build();
}

private Signature createValidSignature(byte[] hash, ECKey ecKey) {
Sha256Hash sha256Hash = Sha256Hash.wrap(hash);
ECKey.ECDSASignature ecdsaSignature = ecKey.sign(sha256Hash);
byte[] signatureBytes = ecdsaSignature.encodeToDER();
return Signature.newBuilder()
.setHash(ByteString.copyFrom(hash))
.setDer(ByteString.copyFrom(signatureBytes))
.build();
}

private byte[] sha256Hash(String inputData) {
try {
MessageDigest digest = MessageDigest.getInstance("SHA-256");
return digest.digest(inputData.getBytes(StandardCharsets.UTF_8));
} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("SHA-256 not found", e);
}
}

private Signature createInvalidEmptySignature() {
return Signature.newBuilder()
.setHash(ByteString.copyFrom(new byte[]{0}))
.setDer(ByteString.copyFrom(new byte[]{0}))
.build();
}
}
Loading