ECKey: If DER-encoded signatures cannot be parsed, throw SignatureDecodeException rather than RuntimeException.

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() {
+        try {
             return ECKey.verify(Sha256Hash.hashTwice(content), signature, params.getAlertSigningKey());
+        } catch (SignatureDecodeException e) {
+            return false;
+        }
     }
 
     /////////////////////////////////////////////////////////////////////////////////////////////////////////////////

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();
     }

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);
+    }
+}

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,
+    public static TransactionSignature decodeFromBitcoin(byte[] bytes, boolean requireCanonicalEncoding,
-                                                         boolean requireCanonicalEncoding,
+            boolean requireCanonicalSValue) throws SignatureDecodeException, VerificationException {
-                                                         boolean requireCanonicalSValue) throws VerificationException {
         // Bitcoin encoding is DER signature + sighash byte.
         if (requireCanonicalEncoding && !isEncodingCanonical(bytes))
             throw new VerificationException("Signature encoding is not canonical.");
-        ECKey.ECDSASignature sig;
+        ECKey.ECDSASignature sig = ECKey.ECDSASignature.decodeFromDER(bytes);
-        try {
-            sig = ECKey.ECDSASignature.decodeFromDER(bytes);
-        } catch (IllegalArgumentException e) {
-            throw new VerificationException("Could not decode DER", e);
-        }
         if (requireCanonicalSValue && !sig.isCanonical())
             throw new VerificationException("S-value is not canonical.");
 

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");

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);

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);
             }

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);

core/src/main/java/org/bitcoinj/protocols/channels/PaymentChannelV1ClientState.java

@@ -227,7 +227,7 @@ public class PaymentChannelV1ClientState extends PaymentChannelClientState {
      * the appropriate time if necessary.</p>
      */
     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);

core/src/main/java/org/bitcoinj/script/Script.java

@@ -431,8 +431,12 @@ public class Script {
                 // OP_0, skip
             } else {
                 checkNotNull(chunk.data);
+                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) {
+        } catch (SignatureDecodeException e) {
-            // There is (at least) one exception that could be hit here (EOFException, if the sig is too short)
+            // This exception occurs when signing as we run partial/invalid scripts to see if they need more
-            // 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
             // 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)

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;
                 }

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

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();