diff --git a/core/src/main/java/org/bitcoinj/core/AlertMessage.java b/core/src/main/java/org/bitcoinj/core/AlertMessage.java index 7c1ecc10..5020d5c2 100644 --- a/core/src/main/java/org/bitcoinj/core/AlertMessage.java +++ b/core/src/main/java/org/bitcoinj/core/AlertMessage.java @@ -115,7 +115,11 @@ public class AlertMessage extends Message { * doesn't verify, because that would allow arbitrary attackers to spam your users. */ public boolean isSignatureValid() { - return ECKey.verify(Sha256Hash.hashTwice(content), signature, params.getAlertSigningKey()); + try { + return ECKey.verify(Sha256Hash.hashTwice(content), signature, params.getAlertSigningKey()); + } catch (SignatureDecodeException e) { + return false; + } } ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/core/src/main/java/org/bitcoinj/core/ECKey.java b/core/src/main/java/org/bitcoinj/core/ECKey.java index 0e946514..237f5f6c 100644 --- a/core/src/main/java/org/bitcoinj/core/ECKey.java +++ b/core/src/main/java/org/bitcoinj/core/ECKey.java @@ -564,7 +564,10 @@ public class ECKey implements EncryptableItem { } } - public static ECDSASignature decodeFromDER(byte[] bytes) throws IllegalArgumentException { + /** + * @throws SignatureDecodeException if the signature is unparseable in some way. + */ + public static ECDSASignature decodeFromDER(byte[] bytes) throws SignatureDecodeException { ASN1InputStream decoder = null; try { // BouncyCastle by default is strict about parsing ASN.1 integers. We relax this check, because some @@ -573,22 +576,22 @@ public class ECKey implements EncryptableItem { decoder = new ASN1InputStream(bytes); final ASN1Primitive seqObj = decoder.readObject(); if (seqObj == null) - throw new IllegalArgumentException("Reached past end of ASN.1 stream."); + throw new SignatureDecodeException("Reached past end of ASN.1 stream."); if (!(seqObj instanceof DLSequence)) - throw new IllegalArgumentException("Read unexpected class: " + seqObj.getClass().getName()); + throw new SignatureDecodeException("Read unexpected class: " + seqObj.getClass().getName()); final DLSequence seq = (DLSequence) seqObj; ASN1Integer r, s; try { r = (ASN1Integer) seq.getObjectAt(0); s = (ASN1Integer) seq.getObjectAt(1); } catch (ClassCastException e) { - throw new IllegalArgumentException(e); + throw new SignatureDecodeException(e); } // OpenSSL deviates from the DER spec by interpreting these values as unsigned, though they should not be // Thus, we always use the positive versions. See: http://r6.ca/blog/20111119T211504Z.html return new ECDSASignature(r.getPositiveValue(), s.getPositiveValue()); } catch (IOException e) { - throw new IllegalArgumentException(e); + throw new SignatureDecodeException(e); } finally { if (decoder != null) try { decoder.close(); } catch (IOException x) {} @@ -674,6 +677,8 @@ public class ECKey implements EncryptableItem { } catch (NativeSecp256k1Util.AssertFailException e) { log.error("Caught AssertFailException inside secp256k1", e); throw new RuntimeException(e); + } catch (SignatureDecodeException e) { + throw new RuntimeException(e); // cannot happen } } if (FAKE_SIGNATURES) @@ -728,8 +733,9 @@ public class ECKey implements EncryptableItem { * @param data Hash of the data to verify. * @param signature ASN.1 encoded signature. * @param pub The public key bytes to use. + * @throws SignatureDecodeException if the signature is unparseable in some way. */ - public static boolean verify(byte[] data, byte[] signature, byte[] pub) { + public static boolean verify(byte[] data, byte[] signature, byte[] pub) throws SignatureDecodeException { if (Secp256k1Context.isEnabled()) { try { return NativeSecp256k1.verify(data, signature, pub); @@ -746,8 +752,9 @@ public class ECKey implements EncryptableItem { * * @param hash Hash of the data to verify. * @param signature ASN.1 encoded signature. + * @throws SignatureDecodeException if the signature is unparseable in some way. */ - public boolean verify(byte[] hash, byte[] signature) { + public boolean verify(byte[] hash, byte[] signature) throws SignatureDecodeException { return ECKey.verify(hash, signature, getPubKey()); } @@ -761,9 +768,10 @@ public class ECKey implements EncryptableItem { /** * Verifies the given ASN.1 encoded ECDSA signature against a hash using the public key, and throws an exception * if the signature doesn't match + * @throws SignatureDecodeException if the signature is unparseable in some way. * @throws java.security.SignatureException if the signature does not match. */ - public void verifyOrThrow(byte[] hash, byte[] signature) throws SignatureException { + public void verifyOrThrow(byte[] hash, byte[] signature) throws SignatureDecodeException, SignatureException { if (!verify(hash, signature)) throw new SignatureException(); } diff --git a/core/src/main/java/org/bitcoinj/core/SignatureDecodeException.java b/core/src/main/java/org/bitcoinj/core/SignatureDecodeException.java new file mode 100644 index 00000000..130d5aa1 --- /dev/null +++ b/core/src/main/java/org/bitcoinj/core/SignatureDecodeException.java @@ -0,0 +1,35 @@ +/* + * Copyright 2018 Andreas Schildbach + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.bitcoinj.core; + +public class SignatureDecodeException extends Exception { + public SignatureDecodeException() { + super(); + } + + public SignatureDecodeException(String message) { + super(message); + } + + public SignatureDecodeException(Throwable cause) { + super(cause); + } + + public SignatureDecodeException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/core/src/main/java/org/bitcoinj/crypto/TransactionSignature.java b/core/src/main/java/org/bitcoinj/crypto/TransactionSignature.java index ef14b767..dfae9057 100644 --- a/core/src/main/java/org/bitcoinj/crypto/TransactionSignature.java +++ b/core/src/main/java/org/bitcoinj/crypto/TransactionSignature.java @@ -16,6 +16,7 @@ package org.bitcoinj.crypto; +import org.bitcoinj.core.SignatureDecodeException; import org.bitcoinj.core.ECKey; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.VerificationException; @@ -163,20 +164,15 @@ public class TransactionSignature extends ECKey.ECDSASignature { * be canonical. * @param requireCanonicalSValue if the S-value must be canonical (below half * the order of the curve). - * @throws RuntimeException if the signature is invalid or unparseable in some way. + * @throws SignatureDecodeException if the signature is unparseable in some way. + * @throws VerificationException if the signature is invalid. */ - public static TransactionSignature decodeFromBitcoin(byte[] bytes, - boolean requireCanonicalEncoding, - boolean requireCanonicalSValue) throws VerificationException { + public static TransactionSignature decodeFromBitcoin(byte[] bytes, boolean requireCanonicalEncoding, + boolean requireCanonicalSValue) throws SignatureDecodeException, VerificationException { // Bitcoin encoding is DER signature + sighash byte. if (requireCanonicalEncoding && !isEncodingCanonical(bytes)) throw new VerificationException("Signature encoding is not canonical."); - ECKey.ECDSASignature sig; - try { - sig = ECKey.ECDSASignature.decodeFromDER(bytes); - } catch (IllegalArgumentException e) { - throw new VerificationException("Could not decode DER", e); - } + ECKey.ECDSASignature sig = ECKey.ECDSASignature.decodeFromDER(bytes); if (requireCanonicalSValue && !sig.isCanonical()) throw new VerificationException("S-value is not canonical."); diff --git a/core/src/main/java/org/bitcoinj/net/discovery/HttpDiscovery.java b/core/src/main/java/org/bitcoinj/net/discovery/HttpDiscovery.java index 2897a4b7..6498a2c0 100644 --- a/core/src/main/java/org/bitcoinj/net/discovery/HttpDiscovery.java +++ b/core/src/main/java/org/bitcoinj/net/discovery/HttpDiscovery.java @@ -114,7 +114,8 @@ public class HttpDiscovery implements PeerDiscovery { } @VisibleForTesting - public InetSocketAddress[] protoToAddrs(PeerSeedProtos.SignedPeerSeeds proto) throws PeerDiscoveryException, InvalidProtocolBufferException, SignatureException { + public InetSocketAddress[] protoToAddrs(PeerSeedProtos.SignedPeerSeeds proto) throws PeerDiscoveryException, + InvalidProtocolBufferException, SignatureDecodeException, SignatureException { if (details.pubkey != null) { if (!Arrays.equals(proto.getPubkey().toByteArray(), details.pubkey.getPubKey())) throw new PeerDiscoveryException("Public key mismatch"); diff --git a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelClient.java b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelClient.java index 26657127..cce5a269 100644 --- a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelClient.java +++ b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelClient.java @@ -349,7 +349,8 @@ public class PaymentChannelClient implements IPaymentChannelClient { } @GuardedBy("lock") - private void receiveRefund(Protos.TwoWayChannelMessage refundMsg, @Nullable KeyParameter userKey) throws VerificationException { + private void receiveRefund(Protos.TwoWayChannelMessage refundMsg, @Nullable KeyParameter userKey) + throws SignatureDecodeException, VerificationException { checkState(majorVersion == 1); checkState(step == InitStep.WAITING_FOR_REFUND_RETURN && refundMsg.hasReturnRefund()); log.info("Got RETURN_REFUND message, providing signed contract"); @@ -469,7 +470,7 @@ public class PaymentChannelClient implements IPaymentChannelClient { closeReason = CloseReason.REMOTE_SENT_INVALID_MESSAGE; break; } - } catch (VerificationException e) { + } catch (SignatureDecodeException | VerificationException e) { log.error("Caught verification exception handling message from server", e); errorBuilder = Protos.Error.newBuilder() .setCode(Protos.Error.ErrorCode.BAD_TRANSACTION); diff --git a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServer.java b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServer.java index 09909632..8cfad8ac 100644 --- a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServer.java +++ b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServer.java @@ -373,7 +373,7 @@ public class PaymentChannelServer { state.storeChannelInWallet(PaymentChannelServer.this); try { receiveUpdatePaymentMessage(providedContract.getInitialPayment(), false /* no ack msg */); - } catch (VerificationException e) { + } catch (SignatureDecodeException | VerificationException e) { log.error("Initial payment failed to verify", e); error(e.getMessage(), Protos.Error.ErrorCode.BAD_TRANSACTION, CloseReason.REMOTE_SENT_INVALID_MESSAGE); return; @@ -424,7 +424,8 @@ public class PaymentChannelServer { } @GuardedBy("lock") - private void receiveUpdatePaymentMessage(Protos.UpdatePayment msg, boolean sendAck) throws VerificationException, ValueOutOfRangeException, InsufficientMoneyException { + private void receiveUpdatePaymentMessage(Protos.UpdatePayment msg, boolean sendAck) throws SignatureDecodeException, + VerificationException, ValueOutOfRangeException, InsufficientMoneyException { log.info("Got a payment update"); Coin lastBestPayment = state.getBestValueToMe(); @@ -513,7 +514,7 @@ public class PaymentChannelServer { } catch (InsufficientMoneyException e) { log.error("Caught insufficient money exception handling message from client", e); error(e.getMessage(), Protos.Error.ErrorCode.BAD_TRANSACTION, CloseReason.REMOTE_SENT_INVALID_MESSAGE); - } catch (IllegalStateException e) { + } catch (SignatureDecodeException e) { log.error("Caught illegal state exception handling message from client", e); error(e.getMessage(), Protos.Error.ErrorCode.SYNTAX_ERROR, CloseReason.REMOTE_SENT_INVALID_MESSAGE); } diff --git a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServerState.java b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServerState.java index 8bbe2c70..a214aa67 100644 --- a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServerState.java +++ b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelServerState.java @@ -236,7 +236,9 @@ public abstract class PaymentChannelServerState { * @throws VerificationException If the signature does not verify or size is out of range (incl being rejected by the network as dust). * @return true if there is more value left on the channel, false if it is now fully used up. */ - public synchronized boolean incrementPayment(Coin refundSize, byte[] signatureBytes) throws VerificationException, ValueOutOfRangeException, InsufficientMoneyException { + public synchronized boolean incrementPayment(Coin refundSize, byte[] signatureBytes) + throws SignatureDecodeException, VerificationException, ValueOutOfRangeException, + InsufficientMoneyException { stateMachine.checkState(State.READY); checkNotNull(refundSize); checkNotNull(signatureBytes); diff --git a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelV1ClientState.java b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelV1ClientState.java index d7d8c55e..c83dbf7b 100644 --- a/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelV1ClientState.java +++ b/core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelV1ClientState.java @@ -227,7 +227,7 @@ public class PaymentChannelV1ClientState extends PaymentChannelClientState { * the appropriate time if necessary.

*/ public synchronized void provideRefundSignature(byte[] theirSignature, @Nullable KeyParameter userKey) - throws VerificationException { + throws SignatureDecodeException, VerificationException { checkNotNull(theirSignature); stateMachine.checkState(State.WAITING_FOR_SIGNED_REFUND); TransactionSignature theirSig = TransactionSignature.decodeFromBitcoin(theirSignature, true, false); diff --git a/core/src/main/java/org/bitcoinj/script/Script.java b/core/src/main/java/org/bitcoinj/script/Script.java index 0691058b..d70e6631 100644 --- a/core/src/main/java/org/bitcoinj/script/Script.java +++ b/core/src/main/java/org/bitcoinj/script/Script.java @@ -431,8 +431,12 @@ public class Script { // OP_0, skip } else { checkNotNull(chunk.data); - if (myIndex < redeemScript.findSigInRedeem(chunk.data, hash)) - return sigCount; + try { + if (myIndex < redeemScript.findSigInRedeem(chunk.data, hash)) + return sigCount; + } catch (SignatureDecodeException e) { + // ignore + } sigCount++; } } @@ -467,7 +471,7 @@ public class Script { return result; } - private int findSigInRedeem(byte[] signatureBytes, Sha256Hash hash) { + private int findSigInRedeem(byte[] signatureBytes, Sha256Hash hash) throws SignatureDecodeException { checkArgument(chunks.get(0).isOpCode()); // P2SH scriptSig int numKeys = Script.decodeFromOpN(chunks.get(chunks.size() - 2).opcode); TransactionSignature signature = TransactionSignature.decodeFromBitcoin(signatureBytes, true, false); @@ -1410,17 +1414,16 @@ public class Script { // TODO: Should check hash type is known Sha256Hash hash = txContainingThis.hashForSignature(index, connectedScript, (byte) sig.sighashFlags); sigValid = ECKey.verify(hash.getBytes(), sig, pubKey); - } catch (Exception e1) { - // There is (at least) one exception that could be hit here (EOFException, if the sig is too short) - // Because I can't verify there aren't more, we use a very generic Exception catch - - // This RuntimeException occurs when signing as we run partial/invalid scripts to see if they need more + } catch (SignatureDecodeException e) { + // This exception occurs when signing as we run partial/invalid scripts to see if they need more // signing work to be done inside LocalTransactionSigner.signInputs. // FIXME don't rely on exception message - if (e1.getMessage() != null && !e1.getMessage().contains("Reached past end of ASN.1 stream")) + if (e.getMessage() != null && !e.getMessage().contains("Reached past end of ASN.1 stream")) // Don't put critical code here; the above check is not reliable on HotSpot due to optimization: // http://jawspeak.com/2010/05/26/hotspot-caused-exceptions-to-lose-their-stack-traces-in-production-and-the-fix/ - log.warn("Signature checking failed!", e1); + log.warn("Signature parsing failed!", e); + } catch (Exception e) { + log.warn("Signature checking failed!", e); } if (opcode == OP_CHECKSIG) diff --git a/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java b/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java index 9795bc03..d57e27df 100644 --- a/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java +++ b/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java @@ -21,6 +21,7 @@ import org.bitcoinj.core.Coin; import org.bitcoinj.core.ECKey; import org.bitcoinj.core.ECKey.ECDSASignature; import org.bitcoinj.core.NetworkParameters; +import org.bitcoinj.core.SignatureDecodeException; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionConfidence; import org.bitcoinj.core.TransactionInput; @@ -190,7 +191,7 @@ public class DefaultRiskAnalysis implements RiskAnalysis { ECDSASignature signature; try { signature = ECKey.ECDSASignature.decodeFromDER(chunk.data); - } catch (IllegalArgumentException x) { + } catch (SignatureDecodeException x) { // Doesn't look like a signature. signature = null; } diff --git a/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java b/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java index fdce3826..ca530166 100644 --- a/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java +++ b/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java @@ -176,7 +176,7 @@ public class DefaultRiskAnalysisTest { } @Test - public void canonicalSignatureLowS() { + public void canonicalSignatureLowS() throws Exception { // First, a synthetic test. TransactionSignature sig = TransactionSignature.dummy(); Script scriptHighS = ScriptBuilder diff --git a/examples/src/main/java/org/bitcoinj/examples/GenerateLowSTests.java b/examples/src/main/java/org/bitcoinj/examples/GenerateLowSTests.java index a384d974..e7493da5 100644 --- a/examples/src/main/java/org/bitcoinj/examples/GenerateLowSTests.java +++ b/examples/src/main/java/org/bitcoinj/examples/GenerateLowSTests.java @@ -28,10 +28,12 @@ import org.bitcoinj.core.LegacyAddress; import org.bitcoinj.core.Coin; import org.bitcoinj.core.ECKey; import org.bitcoinj.core.NetworkParameters; +import org.bitcoinj.core.SignatureDecodeException; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionInput; import org.bitcoinj.core.TransactionOutput; import org.bitcoinj.core.Utils; +import org.bitcoinj.core.VerificationException; import org.bitcoinj.crypto.TransactionSignature; import org.bitcoinj.params.MainNetParams; import org.bitcoinj.script.Script; @@ -54,7 +56,8 @@ import org.bitcoinj.wallet.RedeemData; public class GenerateLowSTests { public static final BigInteger HIGH_S_DIFFERENCE = new BigInteger("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141", 16); - public static void main(final String[] argv) throws NoSuchAlgorithmException, IOException { + public static void main(final String[] argv) + throws NoSuchAlgorithmException, IOException, VerificationException, SignatureDecodeException { final NetworkParameters params = new MainNetParams(); final LocalTransactionSigner signer = new LocalTransactionSigner(); final SecureRandom secureRandom = SecureRandom.getInstanceStrong();