RPC Server blindly believes whatever you specify in content-length header and allocates data and does a read() #4343

The RPC server accepts what it's given in the Content-Length header as fact, and does very little checking before allocating and doing a read() command. It also does all of this before evaluating IP filters via rpcallowip and before evaluating authentication information. Meaning if someone can connect to your RPC server they can exhaust some memory and exhaust all of your connections with little maintenance or network capacity.

This means you can do:

for i in seq 125; do curl http://127.0.0.1:44555 -X POST -H "content-length: 30000000" -d "" & done;

substituting 44555 with your RPC server's port and the loopback IP with the IP of your RPC server. This performs that way due to this code:

This is where it reads the content-length header https://github.com/bitcoin/bitcoin/blob/v0.9.2/src/rpcprotocol.cpp#L169-L170

Here's where it gets that header data, and uses it to allocate a buffer and starts a read() call which will never finish due to that amount of data not being there.

https://github.com/bitcoin/bitcoin/blob/v0.9.2/src/rpcprotocol.cpp#L185-L195

I understand this is not a huge issue as it's highly discouraged to allow any access to your RPC server's port, and it lacks basic functionality like timeouts to ensure clients do not exhaust its connections, but I thought of bringing this one up particularly because it trusts this value and allocates a buffer prior to authentication and evaluation of rpcallowip rules.

You are not correct that this is done before the rpcallowip check. The rpcallowip check is done immediately after the connection comes in in RPCAcceptHandler.

Nevertheless, see #4373.

Related bug: we use C++'s getline(), which is of unbounded length.

I see that in RPCAcceptHandler, I believe my test method was flawed for that claim. My mistake.

I don't think it's worth trying to work around this in the current state. Preventing connection exhaustion attacks in a remotely sane way would need a complete overhaul of the RPC server.

The combination of boost::asio with getline() and stream.read() and friends is very strange to begin with. The HTTP(S) server should use async I/O all the way through, which would allow for setting timeouts for every connection phase. There are some HTTP server examples here: http://www.boost.org/doc/libs/1_55_0/doc/html/boost_asio/examples/cpp03_examples.html

@laanwj Agree 100% I even prototyped a multi-threaded async I/O JSON-RPC server based on boost::asio here: https://github.com/jgarzik/rpcsrv It is a fully working JSON-RPC server skeleton.

@jgarzik Cool! Any specific reason why that was never integrated?

@laanwj Just time. Also, it's a lot of code for bitcoin, where a pure async I/O server might be more simple.

boost::asio provides several HTTP server examples. I took their M-T server example, and adapted it. I can probably do the same for their async HTTP server example too.

It's more code, sure, but the current rpcserver.cpp+rpcserver.h+rpcprotocol.cpp+rpcprotocol.h is also a lot of code, and less organized. It could be put in a subdir of src.

And indeed a single-threaded asynchronous HTTP server would likely be enough for bitcoind's expected loads.

We'd still need a thread pool for the actual RPC command handling, but that can be completely separate from the HTTP server thread.

@laanwj hmmm, true. My server does offer M-T execution as well as socket handling. Maybe it is worth merging that one. It's tested, working M-T JSON-RPC code after all.

Yes, agreed.