From 26adf6894848807d4b75f1fdec57d9489442519d Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Tue, 8 May 2018 13:51:26 -0300 Subject: [PATCH] Limit initial size of some structures Limits initial size of these structures: - Inputs and Outputs in Transaction - Transactions in Block - Hashes in PartialMerkleeTree The fix prevents this DoS attack: - Somehow the attacker needs to get a p2p connection to the bitcoinj node. - The attacker sends a tx msg that says the tx contains a trillion inputs (or a similar msg attacking any other of the structures described above). - bitcoinj tries to instantiate an ArrayList with a size of a trillion. OutOfMemoryError and the bitcoinj node is down. --- .../main/java/org/bitcoinj/core/Block.java | 2 +- .../org/bitcoinj/core/PartialMerkleTree.java | 2 +- .../java/org/bitcoinj/core/Transaction.java | 9 +- .../org/bitcoinj/core/TransactionWitness.java | 2 +- .../main/java/org/bitcoinj/core/Utils.java | 7 ++ .../java/org/bitcoinj/core/BlockTest.java | 38 +++++++ ...ilteredBlockAndPartialMerkleTreeTests.java | 32 ++++++ .../org/bitcoinj/core/TransactionTest.java | 102 ++++++++++++++++++ 8 files changed, 184 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/Block.java b/core/src/main/java/org/bitcoinj/core/Block.java index b2b54893..0337c75f 100644 --- a/core/src/main/java/org/bitcoinj/core/Block.java +++ b/core/src/main/java/org/bitcoinj/core/Block.java @@ -237,7 +237,7 @@ public class Block extends Message { int numTransactions = (int) readVarInt(); optimalEncodingMessageSize += VarInt.sizeOf(numTransactions); - transactions = new ArrayList<>(numTransactions); + transactions = new ArrayList<>(Math.min(numTransactions, Utils.MAX_INITIAL_ARRAY_LENGTH)); for (int i = 0; i < numTransactions; i++) { Transaction tx = new Transaction(params, payload, cursor, this, serializer, UNKNOWN_LENGTH); // Label the transaction as coming from the P2P network, so code that cares where we first saw it knows. diff --git a/core/src/main/java/org/bitcoinj/core/PartialMerkleTree.java b/core/src/main/java/org/bitcoinj/core/PartialMerkleTree.java index ef726a4c..5073dbce 100644 --- a/core/src/main/java/org/bitcoinj/core/PartialMerkleTree.java +++ b/core/src/main/java/org/bitcoinj/core/PartialMerkleTree.java @@ -118,7 +118,7 @@ public class PartialMerkleTree extends Message { transactionCount = (int)readUint32(); int nHashes = (int) readVarInt(); - hashes = new ArrayList<>(nHashes); + hashes = new ArrayList<>(Math.min(nHashes, Utils.MAX_INITIAL_ARRAY_LENGTH)); for (int i = 0; i < nHashes; i++) hashes.add(readHash()); diff --git a/core/src/main/java/org/bitcoinj/core/Transaction.java b/core/src/main/java/org/bitcoinj/core/Transaction.java index d34ff2a0..29e5b85a 100644 --- a/core/src/main/java/org/bitcoinj/core/Transaction.java +++ b/core/src/main/java/org/bitcoinj/core/Transaction.java @@ -122,11 +122,6 @@ public class Transaction extends ChildMessage { */ public static final Coin MIN_NONDUST_OUTPUT = Coin.valueOf(546); // satoshis - /** - * Max initial size of inputs and outputs ArrayList. - */ - public static final int MAX_INITIAL_INPUTS_OUTPUTS_SIZE = 20; - // These are bitcoin serialized. private long version; private ArrayList inputs; @@ -606,7 +601,7 @@ public class Transaction extends ChildMessage { private void parseInputs() { long numInputs = readVarInt(); optimalEncodingMessageSize += VarInt.sizeOf(numInputs); - inputs = new ArrayList<>(Math.min((int) numInputs, MAX_INITIAL_INPUTS_OUTPUTS_SIZE)); + inputs = new ArrayList<>(Math.min((int) numInputs, Utils.MAX_INITIAL_ARRAY_LENGTH)); for (long i = 0; i < numInputs; i++) { TransactionInput input = new TransactionInput(params, this, payload, cursor, serializer); inputs.add(input); @@ -619,7 +614,7 @@ public class Transaction extends ChildMessage { private void parseOutputs() { long numOutputs = readVarInt(); optimalEncodingMessageSize += VarInt.sizeOf(numOutputs); - outputs = new ArrayList<>(Math.min((int) numOutputs, MAX_INITIAL_INPUTS_OUTPUTS_SIZE)); + outputs = new ArrayList<>(Math.min((int) numOutputs, Utils.MAX_INITIAL_ARRAY_LENGTH)); for (long i = 0; i < numOutputs; i++) { TransactionOutput output = new TransactionOutput(params, this, payload, cursor, serializer); outputs.add(output); diff --git a/core/src/main/java/org/bitcoinj/core/TransactionWitness.java b/core/src/main/java/org/bitcoinj/core/TransactionWitness.java index 05c15286..ae0b4dec 100644 --- a/core/src/main/java/org/bitcoinj/core/TransactionWitness.java +++ b/core/src/main/java/org/bitcoinj/core/TransactionWitness.java @@ -26,7 +26,7 @@ public class TransactionWitness { private final List pushes; public TransactionWitness(int pushCount) { - pushes = new ArrayList<>(pushCount); + pushes = new ArrayList<>(Math.min(pushCount, Utils.MAX_INITIAL_ARRAY_LENGTH)); } public byte[] getPush(int i) { diff --git a/core/src/main/java/org/bitcoinj/core/Utils.java b/core/src/main/java/org/bitcoinj/core/Utils.java index 15872fb2..1c7d5fab 100644 --- a/core/src/main/java/org/bitcoinj/core/Utils.java +++ b/core/src/main/java/org/bitcoinj/core/Utils.java @@ -60,6 +60,13 @@ public class Utils { /** Hex encoding used throughout the framework. Use with HEX.encode(byte[]) or HEX.decode(CharSequence). */ public static final BaseEncoding HEX = BaseEncoding.base16().lowerCase(); + /** + * Max initial size of variable length arrays and ArrayLists that could be attacked. + * Avoids this attack: Attacker sends a msg indicating it will contain a huge number (eg 2 billion) elements (eg transaction inputs) and + * forces bitcoinj to try to allocate a huge piece of the memory resulting in OutOfMemoryError. + */ + public static final int MAX_INITIAL_ARRAY_LENGTH = 20; + private static BlockingQueue mockSleepQueue; /** diff --git a/core/src/test/java/org/bitcoinj/core/BlockTest.java b/core/src/test/java/org/bitcoinj/core/BlockTest.java index 630ddcf0..37600e51 100644 --- a/core/src/test/java/org/bitcoinj/core/BlockTest.java +++ b/core/src/test/java/org/bitcoinj/core/BlockTest.java @@ -31,7 +31,11 @@ import org.bitcoinj.wallet.Wallet.BalanceType; import org.junit.Before; import org.junit.Test; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.OutputStream; import java.math.BigInteger; +import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; import java.util.List; @@ -294,4 +298,38 @@ public class BlockTest { assertTrue(block370661.isBIP66()); assertTrue(block370661.isBIP65()); } + + @Test + public void parseBlockWithHugeDeclaredTransactionsSize() throws Exception{ + Block block = new Block(UNITTEST, 1, Sha256Hash.ZERO_HASH, Sha256Hash.ZERO_HASH, 1, 1, 1, new ArrayList()) { + @Override + protected void bitcoinSerializeToStream(OutputStream stream) throws IOException { + Utils.uint32ToByteStreamLE(getVersion(), stream); + stream.write(getPrevBlockHash().getReversedBytes()); + stream.write(getMerkleRoot().getReversedBytes()); + Utils.uint32ToByteStreamLE(getTimeSeconds(), stream); + Utils.uint32ToByteStreamLE(getDifficultyTarget(), stream); + Utils.uint32ToByteStreamLE(getNonce(), stream); + + stream.write(new VarInt(Integer.MAX_VALUE).encode()); + } + + @Override + public byte[] bitcoinSerialize() { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + try { + bitcoinSerializeToStream(baos); + } catch (IOException e) { + } + return baos.toByteArray(); + } + }; + byte[] serializedBlock = block.bitcoinSerialize(); + try { + UNITTEST.getDefaultSerializer().makeBlock(serializedBlock, serializedBlock.length); + fail("We expect ProtocolException with the fixed code and OutOfMemoryError with the buggy code, so this is weird"); + } catch (ProtocolException e) { + //Expected, do nothing + } + } } diff --git a/core/src/test/java/org/bitcoinj/core/FilteredBlockAndPartialMerkleTreeTests.java b/core/src/test/java/org/bitcoinj/core/FilteredBlockAndPartialMerkleTreeTests.java index 8b557013..fd1a5448 100644 --- a/core/src/test/java/org/bitcoinj/core/FilteredBlockAndPartialMerkleTreeTests.java +++ b/core/src/test/java/org/bitcoinj/core/FilteredBlockAndPartialMerkleTreeTests.java @@ -26,6 +26,8 @@ import org.junit.*; import org.junit.runner.*; import org.junit.runners.*; +import java.io.IOException; +import java.io.OutputStream; import java.math.*; import java.util.*; @@ -202,4 +204,34 @@ public class FilteredBlockAndPartialMerkleTreeTests extends TestWithPeerGroup { // Peer 1 goes away. closePeer(peerOf(p1)); } + + @Test + public void parseHugeDeclaredSizePartialMerkleTree() throws Exception{ + final byte[] bits = new byte[1]; + bits[0] = 0x3f; + final List hashes = new ArrayList<>(); + hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000001")); + hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000002")); + hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000003")); + PartialMerkleTree pmt = new PartialMerkleTree(UNITTEST, bits, hashes, 3) { + public void bitcoinSerializeToStream(OutputStream stream) throws IOException { + uint32ToByteStreamLE(getTransactionCount(), stream); + // Add Integer.MAX_VALUE instead of hashes.size() + stream.write(new VarInt(Integer.MAX_VALUE).encode()); + //stream.write(new VarInt(hashes.size()).encode()); + for (Sha256Hash hash : hashes) + stream.write(hash.getReversedBytes()); + + stream.write(new VarInt(bits.length).encode()); + stream.write(bits); + } + }; + byte[] serializedPmt = pmt.bitcoinSerialize(); + try { + new PartialMerkleTree(UNITTEST, serializedPmt, 0); + fail("We expect ProtocolException with the fixed code and OutOfMemoryError with the buggy code, so this is weird"); + } catch (ProtocolException e) { + //Expected, do nothing + } + } } diff --git a/core/src/test/java/org/bitcoinj/core/TransactionTest.java b/core/src/test/java/org/bitcoinj/core/TransactionTest.java index 7b99f728..5a14971c 100644 --- a/core/src/test/java/org/bitcoinj/core/TransactionTest.java +++ b/core/src/test/java/org/bitcoinj/core/TransactionTest.java @@ -25,10 +25,13 @@ import org.bitcoinj.testing.*; import org.easymock.*; import org.junit.*; +import java.io.IOException; +import java.io.OutputStream; import java.math.BigInteger; import java.util.*; import static org.bitcoinj.core.Utils.HEX; +import static org.bitcoinj.core.Utils.uint32ToByteStreamLE; import static org.easymock.EasyMock.*; import static org.junit.Assert.*; @@ -469,4 +472,103 @@ public class TransactionTest { }; } } + + @Test + public void parseTransactionWithHugeDeclaredInputsSize() throws Exception { + Transaction tx = new HugeDeclaredSizeTransaction(UNITTEST, true, false, false); + byte[] serializedTx = tx.bitcoinSerialize(); + try { + new Transaction(UNITTEST, serializedTx); + fail("We expect ProtocolException with the fixed code and OutOfMemoryError with the buggy code, so this is weird"); + } catch (ProtocolException e) { + //Expected, do nothing + } + } + + @Test + public void parseTransactionWithHugeDeclaredOutputsSize() throws Exception { + Transaction tx = new HugeDeclaredSizeTransaction(UNITTEST, false, true, false); + byte[] serializedTx = tx.bitcoinSerialize(); + try { + new Transaction(UNITTEST, serializedTx); + fail("We expect ProtocolException with the fixed code and OutOfMemoryError with the buggy code, so this is weird"); + } catch (ProtocolException e) { + //Expected, do nothing + } + } + + @Test + public void parseTransactionWithHugeDeclaredWitnessPushCountSize() throws Exception { + Transaction tx = new HugeDeclaredSizeTransaction(UNITTEST, false, false, true); + byte[] serializedTx = tx.bitcoinSerialize(); + try { + new Transaction(UNITTEST, serializedTx); + fail("We expect ProtocolException with the fixed code and OutOfMemoryError with the buggy code, so this is weird"); + } catch (ProtocolException e) { + //Expected, do nothing + } + } + + private static class HugeDeclaredSizeTransaction extends Transaction { + + private boolean hackInputsSize; + private boolean hackOutputsSize; + private boolean hackWitnessPushCountSize; + + public HugeDeclaredSizeTransaction(NetworkParameters params, boolean hackInputsSize, boolean hackOutputsSize, boolean hackWitnessPushCountSize) { + super(params); + this.protocolVersion = NetworkParameters.ProtocolVersion.WITNESS_VERSION.getBitcoinProtocolVersion(); + Transaction inputTx = new Transaction(params); + inputTx.addOutput(Coin.FIFTY_COINS, LegacyAddress.fromKey(params, ECKey.fromPrivate(BigInteger.valueOf(123456)))); + this.addInput(inputTx.getOutput(0)); + this.getInput(0).disconnect(); + TransactionWitness witness = new TransactionWitness(1); + witness.setPush(0, new byte[] {0}); + this.getInput(0).setWitness(witness); + Address to = LegacyAddress.fromKey(params, ECKey.fromPrivate(BigInteger.valueOf(1000))); + this.addOutput(Coin.COIN, to); + + this.hackInputsSize = hackInputsSize; + this.hackOutputsSize = hackOutputsSize; + this.hackWitnessPushCountSize = hackWitnessPushCountSize; + } + + @Override + protected void bitcoinSerializeToStream(OutputStream stream, boolean useSegwit) throws IOException { + // version + uint32ToByteStreamLE(getVersion(), stream); + // marker, flag + if (useSegwit) { + stream.write(0); + stream.write(1); + } + // txin_count, txins + long inputsSize = hackInputsSize ? Integer.MAX_VALUE : getInputs().size(); + stream.write(new VarInt(inputsSize).encode()); + for (TransactionInput in : getInputs()) + in.bitcoinSerialize(stream); + // txout_count, txouts + long outputsSize = hackOutputsSize ? Integer.MAX_VALUE : getOutputs().size(); + stream.write(new VarInt(outputsSize).encode()); + for (TransactionOutput out : getOutputs()) + out.bitcoinSerialize(stream); + // script_witnisses + if (useSegwit) { + for (TransactionInput in : getInputs()) { + TransactionWitness witness = in.getWitness(); + long pushCount = hackWitnessPushCountSize ? Integer.MAX_VALUE : witness.getPushCount(); + stream.write(new VarInt(pushCount).encode()); + for (int i = 0; i < witness.getPushCount(); i++) { + byte[] push = witness.getPush(i); + stream.write(new VarInt(push.length).encode()); + stream.write(push); + } + + in.getWitness().bitcoinSerializeToStream(stream); + } + } + // lock_time + uint32ToByteStreamLE(getLockTime(), stream); + } + } }