Check for malformed signature and schnorrKey fields in verifyTransac…

…tion (#46) * Check for malformed signature and schnorrKey fields in verifyTransaction We don't catch Json::Exceptions in verifyTransaction so we must check if those fields are not strings otherwise you can cause a crash * Write unit test for rejecting a malformed signature field
mit-dci · Jul 7, 2018 · 16b2d76 · 16b2d76
1 parent 3defa04
commit 16b2d76
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 17 deletions.
diff --git a/src/kernel/blockchain.cpp b/src/kernel/blockchain.cpp
@@ -272,25 +272,29 @@ std::tuple<bool, bool> CryptoKernel::Blockchain::verifyTransaction(Storage::Tran
 
         if(!outData["schnorrKey"].empty() && outData["contract"].empty()) {
             const Json::Value spendData = inp.getData();
-            if(spendData["signature"].empty()) {
+            if(spendData["signature"].empty() || !spendData["signature"].isString()) {
                 log->printf(LOG_LEVEL_INFO,
                             "blockchain::verifyTransaction(): Could not verify input signature");
                 return std::make_tuple(false, true);
             }
 
-            CryptoKernel::Schnorr schnorr;
-            schnorr.setPublicKey(outData["schnorrKey"].asString());
-            if(!schnorr.verify(out.getId().toString() + outputHash.toString(),
-                              spendData["signature"].asString())) {
-                log->printf(LOG_LEVEL_INFO,
-                            "blockchain::verifyTransaction(): Could not verify input signature");
-                return std::make_tuple(false, true);
+            if(!outData["schnorrKey"].isString()) {
+                log->printf(LOG_LEVEL_WARN, "blockchain::verifyTransaction(): Output has a malformed schnorr key, not checking its signature");
+            } else {
+                CryptoKernel::Schnorr schnorr;
+                schnorr.setPublicKey(outData["schnorrKey"].asString());
+                if(!schnorr.verify(out.getId().toString() + outputHash.toString(),
+                                spendData["signature"].asString())) {
+                    log->printf(LOG_LEVEL_INFO,
+                                "blockchain::verifyTransaction(): Could not verify input signature");
+                    return std::make_tuple(false, true);
+                }
             }
         }
 
         if(!outData["publicKey"].empty() && outData["contract"].empty()) {
             const Json::Value spendData = inp.getData();
-            if(spendData["signature"].empty()) {
+            if(spendData["signature"].empty() || !spendData["signature"].isString()) {
                 log->printf(LOG_LEVEL_INFO,
                             "blockchain::verifyTransaction(): Could not verify input signature");
                 return std::make_tuple(false, true);

diff --git a/src/kernel/blockchain.h b/src/kernel/blockchain.h
@@ -33,7 +33,7 @@ class Blockchain {
 public:
     Blockchain(CryptoKernel::Log* GlobalLog,
                const std::string& dbDir);
-    ~Blockchain();
+    virtual ~Blockchain();
 
     class InvalidElementException : public std::exception {
     public:

diff --git a/src/kernel/consensus/regtest.cpp b/src/kernel/consensus/regtest.cpp
@@ -17,7 +17,7 @@ bool CryptoKernel::Consensus::Regtest::isBlockBetter(Storage::Transaction* trans
 	return blockData.isBetter;
 }
 
-bool CryptoKernel::Consensus::Regtest::checkConsensusRules(Storage::Transaction* transaction, const CryptoKernel::Blockchain::block& block, const CryptoKernel::Blockchain::dbBlock& previousBlock)
+bool CryptoKernel::Consensus::Regtest::checkConsensusRules(Storage::Transaction* transaction, CryptoKernel::Blockchain::block& block, const CryptoKernel::Blockchain::dbBlock& previousBlock)
 {
 	return true;
 }
@@ -96,12 +96,11 @@ CryptoKernel::Consensus::Regtest::getConsensusData(const CryptoKernel::Blockchai
 	return data;
 }
 
-
 Json::Value CryptoKernel::Consensus::Regtest::consensusDataToJson(const CryptoKernel::Consensus::Regtest::consensusData& data) 
 {
 	Json::Value returning;
 	returning["isBetter"] = data.isBetter;
 	return returning;
 }
 
-
+void CryptoKernel::Consensus::Regtest::start() {}
diff --git a/src/kernel/consensus/regtest.h b/src/kernel/consensus/regtest.h
@@ -28,8 +28,8 @@ class Consensus::Regtest : public Consensus {
 	std::string serializeConsensusData(const CryptoKernel::Blockchain::block& block);
 
 	bool checkConsensusRules(Storage::Transaction* transaction,
-                const CryptoKernel::Blockchain::block& block,
-                const CryptoKernel::Blockchain::dbBlock& previousBlock);
+                        	 CryptoKernel::Blockchain::block& block,
+                             const CryptoKernel::Blockchain::dbBlock& previousBlock);
 
 	Json::Value generateConsensusData(Storage::Transaction* transaction,
 			const CryptoKernel::BigNum& previousBlockId, 
@@ -58,6 +58,8 @@ class Consensus::Regtest : public Consensus {
 	bool submitBlock(Storage::Transaction* transaction, const CryptoKernel::Blockchain::block& block);
 
 	void mineBlock(const bool isBetter, const std::string &pubKey);
+
+	void start();
 
 protected:
 	CryptoKernel::Blockchain* blockchain;

diff --git a/tests/BlockchainTests.cpp b/tests/BlockchainTests.cpp
@@ -0,0 +1,63 @@
+#include "BlockchainTests.h"
+
+#include "blockchain.h"
+#include "consensus/regtest.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(BlockchainTest);
+
+BlockchainTest::BlockchainTest() {
+    log.reset(new CryptoKernel::Log("tests.log"));
+}
+
+BlockchainTest::~BlockchainTest() {
+    CryptoKernel::Storage::destroy("./testblockdb");
+}
+
+void BlockchainTest::setUp() {
+    std::remove("genesistest.json");
+    CryptoKernel::Storage::destroy("./testblockdb");
+
+    blockchain.reset(new testChain(log.get()));
+    consensus.reset(new CryptoKernel::Consensus::Regtest(blockchain.get()));
+    blockchain->loadChain(consensus.get(), "genesistest.json");
+    consensus->start();
+}
+
+void BlockchainTest::tearDown() {}
+
+BlockchainTest::testChain::testChain(CryptoKernel::Log* GlobalLog) : CryptoKernel::Blockchain(GlobalLog, "./testblockdb") {}
+
+BlockchainTest::testChain::~testChain() {}
+
+std::string BlockchainTest::testChain::getCoinbaseOwner(const std::string& publicKey) {
+    return publicKey;
+}
+
+uint64_t BlockchainTest::testChain::getBlockReward(const uint64_t height) {
+    return 100000000;
+}
+
+void BlockchainTest::testVerifyMalformedSignature() {
+    consensus->mineBlock(true, "BL2AcSzFw2+rGgQwJ25r7v/misIvr3t4JzkH3U1CCknchfkncSneKLBo6tjnKDhDxZUSPXEKMDtTU/YsvkwxJR8=");
+
+    const auto block = blockchain->getBlockByHeight(2);
+
+    const auto output = *block.getCoinbaseTx().getOutputs().begin();
+
+    CryptoKernel::Blockchain::output outp(output.getValue() - 20000, 0, Json::nullValue);
+
+    Json::Value spendData;
+    Json::Value randomData;
+    randomData["this is"] = "malformed";
+
+    // Something malformed (not a string)
+    spendData["signature"] = randomData;
+
+    CryptoKernel::Blockchain::input inp(output.getId(), spendData);
+
+    CryptoKernel::Blockchain::transaction tx({inp}, {outp}, 1530888581);
+
+    const auto res = blockchain->submitTransaction(tx);
+
+    CPPUNIT_ASSERT(!std::get<0>(res));
+}
diff --git a/tests/BlockchainTests.h b/tests/BlockchainTests.h
@@ -0,0 +1,40 @@
+#ifndef BLOCKCHAINTEST_H
+#define BLOCKCHAINTEST_H
+
+#include <cppunit/extensions/HelperMacros.h>
+
+#include "blockchain.h"
+
+class BlockchainTest : public CPPUNIT_NS::TestFixture {
+    CPPUNIT_TEST_SUITE(BlockchainTest);
+
+    CPPUNIT_TEST(testVerifyMalformedSignature);
+
+    CPPUNIT_TEST_SUITE_END();
+
+public:
+    BlockchainTest();
+    virtual ~BlockchainTest();
+    void setUp();
+    void tearDown();
+
+private:
+    class testChain : public CryptoKernel::Blockchain {
+        public:
+            testChain(CryptoKernel::Log* GlobalLog);
+            virtual ~testChain();
+        private:
+            virtual std::string getCoinbaseOwner(const std::string& publicKey);
+            virtual uint64_t getBlockReward(const uint64_t height);
+    };
+
+
+    void testVerifyMalformedSignature();
+
+    std::unique_ptr<CryptoKernel::Blockchain> blockchain;
+    std::unique_ptr<CryptoKernel::Log> log;
+    std::unique_ptr<CryptoKernel::Consensus::Regtest> consensus;
+
+};
+
+#endif
diff --git a/tests/BlockchainTypesTests.h b/tests/BlockchainTypesTests.h
@@ -1,5 +1,5 @@
-#ifndef CRYPTOTEST_H
-#define CRYPTOTEST_H
+#ifndef BLOCKCHAINTYPESTEST_H
+#define BLOCKCHAINTYPESTEST_H
 
 #include <cppunit/extensions/HelperMacros.h>