From 2fa5ba30e2942cbafe5fa7dea6eb7e818b3a42bf Mon Sep 17 00:00:00 2001
From: Andreas Schildbach <andreas@schildbach.de>
Date: Sat, 20 Dec 2014 01:15:47 +0100
Subject: [PATCH] DefaultRiskAnalysis.isStandard checks for signatures to use
 canonical DER encoding. Adds a test.

---
 .../bitcoinj/wallet/DefaultRiskAnalysis.java  | 19 ++++++++++++++++-
 .../wallet/DefaultRiskAnalysisTest.java       | 21 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java b/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java
index c36bb6c8..990efae6 100644
--- a/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java
+++ b/core/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.java
@@ -18,12 +18,15 @@
 package org.bitcoinj.wallet;
 
 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.Transaction;
 import org.bitcoinj.core.TransactionConfidence;
 import org.bitcoinj.core.TransactionInput;
 import org.bitcoinj.core.TransactionOutput;
 import org.bitcoinj.core.Wallet;
+import org.bitcoinj.crypto.TransactionSignature;
 import org.bitcoinj.script.ScriptChunk;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -111,7 +114,8 @@ public class DefaultRiskAnalysis implements RiskAnalysis {
         VERSION,
         DUST,
         SHORTEST_POSSIBLE_PUSHDATA,
-        NONEMPTY_STACK  // Not yet implemented (for post 0.12)
+        NONEMPTY_STACK, // Not yet implemented (for post 0.12)
+        SIGNATURE_CANONICAL_ENCODING
     }
 
     /**
@@ -168,6 +172,19 @@ public class DefaultRiskAnalysis implements RiskAnalysis {
         for (ScriptChunk chunk : input.getScriptSig().getChunks()) {
             if (chunk.data != null && !chunk.isShortestPossiblePushData())
                 return RuleViolation.SHORTEST_POSSIBLE_PUSHDATA;
+            if (chunk.isPushData()) {
+                ECDSASignature signature;
+                try {
+                    signature = ECKey.ECDSASignature.decodeFromDER(chunk.data);
+                } catch (RuntimeException x) {
+                    // Doesn't look like a signature.
+                    signature = null;
+                }
+                if (signature != null) {
+                    if (!TransactionSignature.isEncodingCanonical(chunk.data))
+                        return RuleViolation.SIGNATURE_CANONICAL_ENCODING;
+                }
+            }
         }
         return RuleViolation.NONE;
     }
diff --git a/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java b/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java
index e800d4ea..e7eeb29a 100644
--- a/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java
+++ b/core/src/test/java/org/bitcoinj/wallet/DefaultRiskAnalysisTest.java
@@ -17,14 +17,17 @@
 
 package org.bitcoinj.wallet;
 
-import com.google.common.collect.Lists;
+import java.util.Arrays;
 import org.bitcoinj.core.*;
+import org.bitcoinj.crypto.TransactionSignature;
 import org.bitcoinj.params.MainNetParams;
+import org.bitcoinj.script.Script;
 import org.bitcoinj.script.ScriptBuilder;
 import org.bitcoinj.script.ScriptChunk;
 import com.google.common.collect.ImmutableList;
 import org.bitcoinj.wallet.DefaultRiskAnalysis;
 import org.bitcoinj.wallet.RiskAnalysis;
+import org.bitcoinj.wallet.DefaultRiskAnalysis.RuleViolation;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -162,6 +165,22 @@ public class DefaultRiskAnalysisTest {
         assertEquals(DefaultRiskAnalysis.RuleViolation.SHORTEST_POSSIBLE_PUSHDATA, DefaultRiskAnalysis.isStandard(tx));
     }
 
+    @Test
+    public void canonicalSignature() {
+        TransactionSignature sig = TransactionSignature.dummy();
+        Script scriptOk = ScriptBuilder.createInputScript(sig);
+        assertEquals(RuleViolation.NONE,
+                DefaultRiskAnalysis.isInputStandard(new TransactionInput(params, null, scriptOk.getProgram())));
+
+        byte[] sigBytes = sig.encodeToBitcoin();
+        // Appending a zero byte makes the signature uncanonical without violating DER encoding.
+        Script scriptUncanonicalEncoding = new ScriptBuilder().data(Arrays.copyOf(sigBytes, sigBytes.length + 1))
+                .build();
+        assertEquals(RuleViolation.SIGNATURE_CANONICAL_ENCODING,
+                DefaultRiskAnalysis.isInputStandard(new TransactionInput(params, null, scriptUncanonicalEncoding
+                        .getProgram())));
+    }
+
     @Test
     public void standardOutputs() throws Exception {
         Transaction tx = new Transaction(params);