Skip to content

Commit

Permalink
Fix bug with allowing duplicate signature in java SubzeroUtils
Browse files Browse the repository at this point in the history 
A duplicate signature would pass the validation test. This fixes that
bug and adds more test coverage for the validateAndSort method.
  • Loading branch information
@andozw
andozw committed Nov 15, 2024
1 parent bbe6e05 commit 401ad6d
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 1 deletion.
10 changes: 9 additions & 1 deletion java/shared/src/main/java/com/squareup/subzero/shared/SubzeroUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,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,6 +132,7 @@ 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<String> 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
Expand All @@ -150,7 +153,12 @@ protected static List<byte[]> validateAndSort(List<ECKey> pubkeys, byte[] hash,
for (Signature sig : signatures) {
try {
if (pubkey.verify(hash, sig.getDer().toByteArray())) {
sortedSigs.add(sig.getDer().toByteArray());
String derHex = Hex.toHexString(sig.getDer().toByteArray());
if (!seenSigs.contains(derHex)) {
// Only add the signature if it is unique
sortedSigs.add(sig.getDer().toByteArray());
seenSigs.add(derHex);
}
}
} catch (SignatureDecodeException e) {
// keep going, we'll throw a RuntimeException later if we don't find the right number of
Expand Down
114 changes: 114 additions & 0 deletions java/shared/src/test/java/com/squareup/subzero/shared/SubzeroUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,28 @@

import com.google.common.collect.ImmutableList;
import com.google.protobuf.ByteString;
import com.squareup.subzero.proto.service.Common;
import com.squareup.subzero.proto.service.Common.Destination;
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.math.BigInteger;
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 +503,81 @@ public void testValidateSignTxCommandRequest() {
assertTrue(e3.getMessage().contains(ERROR_OUTPUTS_COUNT));
}

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

byte[] hash = generateMockHash("mock input");
List<Signature> signatures = Arrays.asList(
createValidMockSignature(hash, pubkeys.get(0)),
createValidMockSignature(hash, pubkeys.get(1))
);

List<byte[]> sortedSigs = SubzeroUtils.validateAndSort(pubkeys, 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> pubkeys = Arrays.asList(newMockPubKey(), newMockPubKey());
byte[] hash = generateMockHash("mock input");
List<Signature> signatures = Arrays.asList(
createInvalidMockSignature(),
createInvalidMockSignature()
);

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

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

@Test
public void testValidateAndSortSignatureWithWrongHash() {
List<ECKey> pubkeys = Arrays.asList(newMockPubKey(), newMockPubKey());
byte[] hash = generateMockHash("input1");
byte[] wrongHash = generateMockHash("input2");
List<Signature> signatures = Arrays.asList(createValidMockSignature(wrongHash, pubkeys.get(0)));

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

@Test
public void testValidateAndSortDuplicateSignatures() {
List<ECKey> pubkeys = Arrays.asList(newMockPubKey(), newMockPubKey());
byte[] hash = generateMockHash("input");
Signature validSig = createValidMockSignature(hash, pubkeys.get(0));
List<Signature> signatures = Arrays.asList(validSig, validSig);

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

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

RuntimeException exception = assertThrows(RuntimeException.class, () ->
SubzeroUtils.validateAndSort(pubkeys, 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 +610,34 @@ private TxOutput testOutput(long amount, Destination destination) {
.setIndex(2))
.build();
}

private ECKey newMockPubKey() {
return new ECKey();
}

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

private byte[] generateMockHash(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 createInvalidMockSignature() {
return Signature.newBuilder()
.setHash(ByteString.copyFrom(new byte[]{0}))
.setDer(ByteString.copyFrom(new byte[]{0}))
.build();
}
}

0 comments on commit 401ad6d

Please sign in to comment.