CVE-2012-1909: Configuration (CWE-16)
value: The Bitcoin protocol, as used in bitcoind before 0.4.4, wxBitcoin, Bitcoin-Qt, and other programs, does not properly handle multiple transactions with the same identifier, which allows remote attackers to cause a denial of service (unspendable transaction) by leveraging the ability to create a duplicate coinbase transaction.
CWE description: Weaknesses in this category are typically introduced during the configuration of the software.
Published: 2012-08-06T16:55Z
Repository: https://github.com/sipa/bitcoin
Language: C++
Files changed: src/main.cpp
Commit date: 2012-03-03 18:26:36 +0100
Commit message: Do not allow overwriting unspent transactions (BIP 30)

Introduce the following network rule:
 * a block is not valid if it contains a transaction whose hash
   already exists in the block chain, unless all that transaction's
   outputs were already spent before said block.

Warning: this is effectively a network rule change, with potential
risk for forking the block chain. Leaving this unfixed carries the
same risk however, for attackers that can cause a reorganisation
in part of the network.

Thanks to Russell O'Connor and Ben Reeves.

Diff: diff --git a/src/main.cpp b/src/main.cpp
index 9951952..20aa069 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -974,12 +974,14 @@ bool CTransaction::DisconnectInputs(CTxDB& txdb)
                 return error("DisconnectInputs() : UpdateTxIndex failed");
         }
     }
 
     // Remove transaction from index
-    if (!txdb.EraseTxIndex(*this))
-        return error("DisconnectInputs() : EraseTxPos failed");
+    // This can fail if a duplicate of this transaction was in a chain that got
+    // reorganized away. This is only possible if this transaction was completely
+    // spent, so erasing it would be a no-op anway.
+    txdb.EraseTxIndex(*this);
 
     return true;
 }
 
 
@@ -1254,10 +1256,30 @@ bool CBlock::ConnectBlock(CTxDB& txdb, CBlockIndex* pindex)
 {
     // Check it again in case a previous version let a bad block in
     if (!CheckBlock())
         return false;
 
+    // Do not allow blocks that contain transactions which 'overwrite' older transactions,
+    // unless those are already completely spent.
+    // If such overwrites are allowed, coinbases and transactions depending upon those
+    // can be duplicated to remove the ability to spend the first instance -- even after
+    // being sent to another address.
+    // See BIP30 and http://r6.ca/blog/20120206T005236Z.html for more information.
+    // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool
+    // already refuses previously-known transaction id's entirely.
+    // This rule applies to all blocks whose timestamp is after March 15, 2012, 0:00 UTC.
+    // On testnet it is enabled as of februari 20, 2012, 0:00 UTC.
+    if (pindex->nTime > 1331769600 || (fTestNet && pindex->nTime > 1329696000))
+        BOOST_FOREACH(CTransaction& tx, vtx)
+        {
+            CTxIndex txindexOld;
+            if (txdb.ReadTxIndex(tx.GetHash(), txindexOld))
+                BOOST_FOREACH(CDiskTxPos &pos, txindexOld.vSpent)
+                    if (pos.IsNull())
+                        return false;
+        }
+
     // To avoid being on the short end of a block-chain split,
     // don't do secondary validation of pay-to-script-hash transactions
     // until blocks with timestamps after paytoscripthashtime (see init.cpp for default).
     // This code can be removed once a super-majority of the network has upgraded.
     int64 nEvalSwitchTime = GetArg("-paytoscripthashtime", std::numeric_limits<int64_t>::max());

Vulnerable code:     if (!txdb.EraseTxIndex(*this))
        return error("DisconnectInputs() : EraseTxPos failed");
Fixed code:     // This can fail if a duplicate of this transaction was in a chain that got
    // reorganized away. This is only possible if this transaction was completely
    // spent, so erasing it would be a no-op anway.
    txdb.EraseTxIndex(*this);
    // Do not allow blocks that contain transactions which 'overwrite' older transactions,
    // unless those are already completely spent.
    // If such overwrites are allowed, coinbases and transactions depending upon those
    // can be duplicated to remove the ability to spend the first instance -- even after
    // being sent to another address.
    // See BIP30 and http://r6.ca/blog/20120206T005236Z.html for more information.
    // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool
    // already refuses previously-known transaction id's entirely.
    // This rule applies to all blocks whose timestamp is after March 15, 2012, 0:00 UTC.
    // On testnet it is enabled as of februari 20, 2012, 0:00 UTC.
    if (pindex->nTime > 1331769600 || (fTestNet && pindex->nTime > 1329696000))
        BOOST_FOREACH(CTransaction& tx, vtx)
        {
            CTxIndex txindexOld;
            if (txdb.ReadTxIndex(tx.GetHash(), txindexOld))
                BOOST_FOREACH(CDiskTxPos &pos, txindexOld.vSpent)
                    if (pos.IsNull())
                        return false;
        }