Skip to content

Commit

Permalink
Limit initial size of some structures
Browse files Browse the repository at this point in the history
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.

Cherry pick bitcoinj@26adf68
  • Loading branch information
oscarguindzberg committed Dec 28, 2018
1 parent 7bd0d36 commit f49faa4
Show file tree
Hide file tree
Showing 7 changed files with 146 additions and 9 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/org/bitcoinj/core/Block.java
Expand Up @@ -237,7 +237,7 @@ protected void parseTransactions(final int transactionsOffset) throws ProtocolEx

int numTransactions = (int) readVarInt();
optimalEncodingMessageSize += VarInt.sizeOf(numTransactions);
transactions = new ArrayList<Transaction>(numTransactions);
transactions = new ArrayList<Transaction>(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.
Expand Down
Expand Up @@ -116,7 +116,7 @@ protected void parse() throws ProtocolException {
transactionCount = (int)readUint32();

int nHashes = (int) readVarInt();
hashes = new ArrayList<Sha256Hash>(nHashes);
hashes = new ArrayList<Sha256Hash>(Math.min(nHashes, Utils.MAX_INITIAL_ARRAY_LENGTH));
for (int i = 0; i < nHashes; i++)
hashes.add(readHash());

Expand Down
9 changes: 2 additions & 7 deletions core/src/main/java/org/bitcoinj/core/Transaction.java
Expand Up @@ -117,11 +117,6 @@ public int compare(final Transaction tx1, final Transaction tx2) {
*/
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<TransactionInput> inputs;
Expand Down Expand Up @@ -571,7 +566,7 @@ protected void parse() throws ProtocolException {
// First come the inputs.
long numInputs = readVarInt();
optimalEncodingMessageSize += VarInt.sizeOf(numInputs);
inputs = new ArrayList<TransactionInput>(Math.min((int) numInputs, MAX_INITIAL_INPUTS_OUTPUTS_SIZE));
inputs = new ArrayList<TransactionInput>(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);
Expand All @@ -582,7 +577,7 @@ protected void parse() throws ProtocolException {
// Now the outputs
long numOutputs = readVarInt();
optimalEncodingMessageSize += VarInt.sizeOf(numOutputs);
outputs = new ArrayList<TransactionOutput>(Math.min((int) numOutputs, MAX_INITIAL_INPUTS_OUTPUTS_SIZE));
outputs = new ArrayList<TransactionOutput>(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);
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/org/bitcoinj/core/Utils.java
Expand Up @@ -56,6 +56,13 @@ public class Utils {

private static final Joiner SPACE_JOINER = Joiner.on(" ");

/**
* 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<Boolean> mockSleepQueue;

/**
Expand Down
38 changes: 38 additions & 0 deletions core/src/test/java/org/bitcoinj/core/BlockTest.java
Expand Up @@ -30,7 +30,11 @@
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;
Expand Down Expand Up @@ -286,4 +290,38 @@ public void isBIPs() throws Exception {
assertTrue(block370661.isBIP66());
assertTrue(block370661.isBIP65());
}

@Test
public void parseBlockWithHugeDeclaredTransactionsSize() throws Exception{
Block block = new Block(PARAMS, 1, Sha256Hash.ZERO_HASH, Sha256Hash.ZERO_HASH, 1, 1, 1, new ArrayList<Transaction>()) {
@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 {
PARAMS.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
}
}
}
Expand Up @@ -26,6 +26,8 @@
import org.junit.runner.*;
import org.junit.runners.*;

import java.io.IOException;
import java.io.OutputStream;
import java.math.*;
import java.util.*;

Expand Down Expand Up @@ -202,4 +204,34 @@ public void serializeDownloadBlockWithWallet() throws Exception {
// Peer 1 goes away.
closePeer(peerOf(p1));
}

@Test
public void parseHugeDeclaredSizePartialMerkleTree() throws Exception{
final byte[] bits = new byte[1];
bits[0] = 0x3f;
final List<Sha256Hash> hashes = new ArrayList<Sha256Hash>();
hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000001"));
hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000002"));
hashes.add(Sha256Hash.wrap("0000000000000000000000000000000000000000000000000000000000000003"));
PartialMerkleTree pmt = new PartialMerkleTree(PARAMS, 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(PARAMS, 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
}
}
}
65 changes: 65 additions & 0 deletions core/src/test/java/org/bitcoinj/core/TransactionTest.java
Expand Up @@ -25,10 +25,13 @@
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.*;

Expand Down Expand Up @@ -404,4 +407,66 @@ public void run() {
};
}
}

@Test
public void parseTransactionWithHugeDeclaredInputsSize() throws Exception {
Transaction tx = new HugeDeclaredSizeTransaction(PARAMS, true, false);
byte[] serializedTx = tx.bitcoinSerialize();
try {
new Transaction(PARAMS, 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(PARAMS, false, true);
byte[] serializedTx = tx.bitcoinSerialize();
try {
new Transaction(PARAMS, 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;

public HugeDeclaredSizeTransaction(NetworkParameters params, boolean hackInputsSize, boolean hackOutputsSize) {
super(params);
this.protocolVersion = NetworkParameters.ProtocolVersion.CURRENT.getBitcoinProtocolVersion();
Transaction inputTx = new Transaction(params);
inputTx.addOutput(Coin.FIFTY_COINS, ECKey.fromPrivate(BigInteger.valueOf(123456)).toAddress(params));
this.addInput(inputTx.getOutput(0));
this.getInput(0).disconnect();
Address to = ECKey.fromPrivate(BigInteger.valueOf(1000)).toAddress(params);
this.addOutput(Coin.COIN, to);

this.hackInputsSize = hackInputsSize;
this.hackOutputsSize = hackOutputsSize;
}

@Override
protected void bitcoinSerializeToStream(OutputStream stream) throws IOException {
// version
uint32ToByteStreamLE(getVersion(), stream);
// 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);
// lock_time
uint32ToByteStreamLE(getLockTime(), stream);
}
}
}

0 comments on commit f49faa4

Please sign in to comment.