search

cryptocoinjs / btc-transaction

Create, parse, serialize Bitcoin transactions.
11 stars 8 forks source link

Allow to limit the number of inputs/outputs #10

Open shesek opened 10 years ago

shesek commented 10 years ago

Without that, deserializing user-provided transactions could allow for a DoS attack by specifying a huge input/output count.

shesek commented 10 years ago

I think that it might also make sense to put the txCount under the new options argument, possibly in a backward-compatible manner for now (if options is a number, treat it as the txCount).

sidazhang commented 10 years ago

I think that is sensible.

In terms of backwards compatibility. Let's do that until another major release?

However, I think the better way to limit user provided transaction by limiting size. In practice, a transaction with over 6000 inputs + outputs can be deserialized/serialized in around 100ms. So this might not be a big issue. The server can limit anything above 200kb for example.

Thoughts?

shesek commented 10 years ago

In terms of backwards compatibility. Let's do that until another major release?

The current pull request is backward compatible. The suggestion about merging txCount into the options object can also be done without breaking API compatibility (as I mentioned, by treating the options argument as the txCount when its a number).

The server can limit anything above 200kb for example.

The transaction doesn't actually need to have a lot of of inputs/outputs - its enough to specify a large number of inputs/outputs. For example, parsing 01000000ffffffffffffffffff as the transaction (the version number 0x01 followed by an input count of 0xffffffffffffffff) would cause the inputs for loop to iterate 18446744073709551615 times, hugging all resources.

Another possible solution that I'm thinking of now is to check for MessageParser::hasFailed at the end of every input/output iteration and break the loop if we reached the end of the buffer, which would make a byte-size limitation work properly.

Edit: Oh - I just now noticed that this check already breaks out of the loop if there are no more bytes to consume, and would prevent an high input/output count with no actual inputs/outputs from going on forever... so yes - limiting the total transaction size does resolve that.