rpc: Prevent easy memory exhaustion attack

[laanwj]

Allocate memory for POST message data only as bytes come in, instead of
all at once at the beginning.

Fixes #4343.

[jgarzik]

Comments:

• Agree this wants fixing.
• Memory exhaustion in RPC is a low priority, since it's an internal control plane, not something that should be exposed to the open Internet.
• 16k seems small. It is easy to construct a valid JSON batch request of this size. Make it 1MB.
• During review, I noticed we use getline(), which is similarly unbounded input.

So, increase the limit and consider fixing getline() calls too.

[gavinandresen]

So with this fix, you actually have to SEND 4GB (or whatever) of data to exhaust 4GB (or whatever) of memory, instead of just sending a header SAYING that you'll send 4GB of data.

ACK: better is better. Although did you consider just setting an arbitrary, large limit on nLen? Code would be simpler...

[jgarzik]

@gavinandresen Actually, very good final point. Just change the limit here:

    int nLen = ReadHTTPHeaders(stream, mapHeadersRet);
    if (nLen < 0 || nLen > (int)MAX_SIZE)
        return HTTP_INTERNAL_SERVER_ERROR;

to 1MB.

Currently the limit is not infinite, either:

static const unsigned int MAX_SIZE = 0x02000000;

So, that's 32MB.

[jgarzik]

General comment: Our RPC code really wants to be smarter in any case. You can just as easily open a bunch of connections, even with the loop, and exhaust memory. Or another familiar HTTP attack is opening a bunch of connections, keeping them open for hours or days without making progress.

i.e. send one character per day across 10,000 connections. Stupid server will not timeout, and sockets will be rapidly exhausted.

[laanwj]

Yes there is already a limit, and yes this is low priority, I was mislead by the issue description that this happened before the rpcallow check. Closing.

[laanwj]

Also it's not about unbounded input, but as @gavinandresen remarks correctly that with this change you actually have to send the amount of bytes to get them allocated instead of being able to open 100 connections up-front and specifying 32 mbytes of data and keeping open the connection without actually sending it (which is very cheap to do).

[jgarzik]

@laanwj Agree, that is why I did not NAK the patch.

Adding a loop is fine with me... just 16k was much too low.

[sipa]

Untested ACK; I would raise the 16k too, though.

[laanwj]

OK, seemingly this fix-on-top-of-a-stack-of-hacks is good enough for now, will change POST_READ_SIZE to 1MiB.

[laanwj]

Rebased and changed POST_READ_SIZE to 256kiB.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4373_2ec5a3d212ac4b09e6c32d495f34ee3cdedc8c66/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.