Author

Topic: Buggy CVE-2013-4627 patch, open new vectors of attack (Read 871 times)

legendary
Activity: 1120
Merit: 1160
The reason for mapRelay (i.e., storing relayed data in wire format, rather than in parsed form), was I believe historically because Satoshi wanted to be able to extend the different data structures later in newer versions, and have old code relay them. I'm sure he didn't consider DoS attacks when designing this.

FWIW I took a quick look at v0.1 - it has the comment "Save original serialized message so newer versions are preserved" in RelayMessage() so I'm sure your theory is correct.
sr. member
Activity: 336
Merit: 250
Cuddling, censored, unicorn-shaped troll.
I'm not sure I understood all of this, sorry for being a newbie.
But are offchain Tx likely to be double spent too, using the same vin.size trick ?
Or does "unconfirmed transactions" already mean offchain, too ?  Lips sealed
legendary
Activity: 1072
Merit: 1189
The reason for mapRelay (i.e., storing relayed data in wire format, rather than in parsed form), was I believe historically because Satoshi wanted to be able to extend the different data structures later in newer versions, and have old code relay them. I'm sure he didn't consider DoS attacks when designing this.

This may still be useful, and initially I was in favor of trying to keep this behaviour - there are certainly ideas for the future where this would be useful. However, each of those would require some form of authentication - a proof that the extension was in fact created by the creator of the transaction itself. The problem is that without having a definition for that extension, no old node can make this verification, and could easily be made to forward incorrect data with a transaction - potentially risking being considered a DoS attacker itself.

Given that, I think there is no safe way to use that feature anyway, and I'm in favor of just forwarding parsed data.
hero member
Activity: 555
Merit: 654
My proposals to reduce this kinds of risks are:

- Make non-compact Var_ints non-standard (this should harm nobody, since I don't know why a client should store them in any other way that the encoding of minimum size)

- Remove the MapRelay functionality (or at least explain what exactly is the functionality!)

legendary
Activity: 2053
Merit: 1356
aka tonikt
So are you saying that he actually noticed the attack, and only drew your attention by proposing a stupid patch to prevent it?
Well, good for you, man. Smiley
Though, their solution, how to fix it, is better, IMO
legendary
Activity: 1120
Merit: 1160
Quote from: Sergio_Demian_Lerner
It seems that Satoshi code was not designed to protect you from DoS, but it’s also true that the core developers have tried to add some DoS preventions since the Satoshi times. In any case, it’s only a minor vulnerability or flaw. What worries me is not that a bug was found, nor that a bug in the patch was found, but that the github commit of the patch does not show a history of  a discussion regarding the patch correctness, nor it is recorded if the code was audited and by whom. This lack of standardized protocol for the treatment of sensitive patches should be corrected, and I offer myself to review security critical code, whenever it is needed, and whenever I can.

FWIW the discussion regarding the patch happened in private email CC'd to all core developers and the bitcoinj developers; nearly everyone CC'd participated in the discussion, and did so in only a few hours. This patch was unusual because it was written during an active attack on mainnet, and doubly unusual because you only need a small number of patched nodes to stop the attack against the network as a whole. (individual nodes are still vulnerable)

It's fine to think about protocols for vetting security-related patches, but it'll be awhile before the Bitcoin project has the resources to hire the multiple full-time staff with overlapping schedules we need to really use those protocols effectively. Unfortunately it's an response time vs. risk tradeoff without one-size-fits-all answers.

Frankly the best I can say is donate to the Bitcoin foundation so they can hire more developers.
legendary
Activity: 2053
Merit: 1356
aka tonikt
BTW, once again I want to raise my unheard voice, that txs with inputs that are not mined yet should be threat same as non-standard.
They don't make any economical sense for anyone, except saroshidice.
So unless you guys get a cut of it, or play there, please stop it Smiley
legendary
Activity: 1120
Merit: 1160
It looks like even having that cache enables and/or makes worse other types of attacks, so right now I'm thinking it probably should be removed completely, but I want to think more about the issue. (the extra memory usage is probably more expensive than re-serializing on demand)

Also yet another example of why zero-conf is dangerous.
legendary
Activity: 2053
Merit: 1356
aka tonikt
Sorry, its a bit long to read, but if you don't mind I have a question already at the beginning.

Quote
Almost all objects that are exchanged between Satoshi clients are deserialized to be stored in memory and serialized again when they need to be forwarded. . Transactions are also first deserialized in order to be parsed, but they are stored “as is” in a temporary cache called MapRelay, and forwarded without being serialized again. I really don’t know why this is done. But the point is that the storage of messages as they are received leads to the possibility that an attacker creates messages much bigger than the actual transaction object contained, which, when stored in MapRelay memory, can be used to exhaust the memory of a node. This is the code that patches the vulnerability:

So, you are saying that checking whether a serialized size at the moment you receive the transaction, cutting if needed, and keeping the transaction as raw data is less DoS-resistant than keeping the transaction in objects and assembling it each time any node wants it?

The only thing I would change here is only banning those that send me too long txs.
But since we know that they might just route the shit, because of the bug, banning them would not be polite.
So its a hard moral problem Smiley
hero member
Activity: 555
Merit: 654
After talking with the core dev about this issue, their opinion is that this is not a security threat, nor a vulnerability. Accordingly, it's best for the community to be informed about the problem so each user or organization evaluates if upgrading to the 0.8.3 version (instead of using the previous version, vulnerable to CVE-2013-4627) is desirable or not, depending on risks associated with their own Bitcoin infrastructure.

The original disclosure can be found at: https://bitslog.wordpress.com/2013/07/18/buggy-cve-2013-4627-patch-open-new-vectors-of-attack/

Here I'm copying the contents so bitcointalk users do not need to click on the link:

Buggy CVE-2013-4627 patch, open new vectors of attack

Secure coding is hard. But in Bitcoin, secure coding also means understanding every little detail of the undocumented (or code-documented) rules that Satoshi the great has brought to us mortals. CVE-2013-4627 patches a DoS vulnerability discovered by Peter Todd. The vulnerability is easy to spot once you read the code after the patch was applied. Bitcoin network messages can be of arbitrary sizes, and this is generally not a problem, with the sole exception of transaction messages. Almost all objects that are exchanged between Satoshi clients are deserialized to be stored in memory and serialized again when they need to be forwarded. . Transactions are also first deserialized in order to be parsed, but they are stored “as is” in a temporary cache called MapRelay, and forwarded without being serialized again. I really don’t know why this is done. But the point is that the storage of messages as they are received leads to the possibility that an attacker creates messages much bigger than the actual transaction object contained, which, when stored in MapRelay memory, can be used to exhaust the memory of a node. This is the code that patches the vulnerability:


Code:
unsigned int nSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
unsigned int oldSize = vMsg.size();

if (nSize < oldSize) {
  vMsg.resize(nSize); ...
}

This patch that supposedly solves the CVE-2013-4627 bug opens the door at least two other attacks. Transactions on the wire may have an encoding which is different from the encoding you get when you serialize and deserialize it.
Still after many efforts to standardize transactions, preventing ambiguity and storing signatures in a canonical form, transactions are malleable. So when you truncate vMsg, you don’t know what are you truncating.

Double-spend Attack

The attack works against sites that accept 0 confirmations like SatoshiDice. It could allow 100% effective double-spends. The idea is that you create a message vMsg with a transaction Tx such that:

Code:
vMsg.size() == n
nSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) == n-1

But in the transaction stored in vMsg, the field vin.size is stored in a non-standard format that takes 4 bytes more.

Suppose that vin.size = 1. Then it is stored as: 0xfe 0×01 0×00 0×00 0×00, instead of being stored as “0×01″

When vMsg is truncated by vMsg.resize(nSize), it will become an invalid transaction (since 3 good bytes at the end will be chopped off).
Now the node that receives vMsg will process the Tx normally, but will relay the transaction vMsg (which is invalid). This is because the invalid tx in the msg will be stored in MapRelay for 15 minutes. Peers will ignore this invalid message. If the node processes Tx and responds with a transaction Tx’ (using Tx outputs, such as SatoshiDice) then Tx’ will end up in peer’s orphan cache, because Tx is not present since it wasn’t accepted.

So the attack works with 100% success rate for sites that create transactions as response to unconfirmed transactions, such as SatoshiDice, and are running version 0.8.3 of the Satoshi client.

Suppose that a site called InstantBitBets uses version 0.8.3 of the Satoshi client. The attack against InstantBitBets looks like this:

1. Locate the node of InstantBitBets. Connect directly to this node.
2. Create a bet in a Tx. Create a non-standard message vMsg with 1 extra byte and a non-standard  vin.size field.
3. Send it directly to InstantBitBets node.
4. If you receive a Tx’ saying that you won, broadcasts Tx to the rest of the network (normally).
5. If you’ve lost, move the funds from Tx to Tx2, and try again from Tx2 (this step might not even be necessary).

Also the attack could help to execute other kind of unknown attacks, since everybody expects a node to broadcasts successfully a Tx that it accepts.

Remote Anonymous Denial of Service Attack

If the right mix of Bitcoinj, old Satoshi nodes and new 0.8.3 nodes are present in the network, then a new attack vector emerges that is able to disconnect 0.8.3 nodes from Bitcoinj nodes remotely.
Consider, for example,  the following mix: old nodes=50%, 0.8.3 nodes=40%, Bitcoinj nodes=10%.

The attacker builds a transaction with an expanded var_int field (e.g.: input count). The expansion consist of encoding the count in more bytes than it is required. This transaction is forwarded to a bunch of 0.8.1 and prior clients. Alternatively, transactions passing through the attackers node can be modified on-the-fly, expanding the input count fields. Each of these clients will resend the tx without modification, and the tx will spread. When the transaction arrives to an old Satoshi code, it will broadcast it. When this transaction arrives to a 0.8.3 node, it will resend it truncated, and this will disconnect the 0.8.3 node from all Bitcoinj nodes surrounding it. I have not verified by testing that Bitcoinj does this, but I’m almost sure that the exception thrown will terminate the connection.
If the attack does not success the first time, the attacker can try as many times as he wants. As previously said, he can even use existing relayed transactions, expanding the input_count field. So after some minutes, and high number of modified txs, the attack must be successful.
The success probability depends on the number of old, 0.8.3 and bitcoinj nodes, and network topology. And the attackey may try sending the tx to different nodes to find the right path to the victim’s node. So I think that with enough tries, it will succeed even with very different client version mixes.

So basically the attack takes advantage of the network client mix, and remotely (an anonymously) harm 0.8.3/Bitcoinj nodes.
If the attacker knows that two victims are connected, one is a 0.8.3 nodes, and the other is a Bitcoinj node, he can remotely perform the attack to disconnect them.

Are these problems vulnerabilities of Bitcoin 0.8.3? It depends on what you expect from the protocol. If you expected the Satoshi code to be protected from DoS and 0-confirmation double-spends, then it is. It seems that Satoshi code was not designed to protect you from DoS, but it’s also true that the core developers have tried to add some DoS preventions since the Satoshi times.

In any case, it’s only a minor vulnerability or flaw.

Best regards, Sergio.

Jump to: