The JSON parser has known DoS issues, and is not robust enough to be used in an unauthenticated protocol.
The JSON format for the getutxos call should be removed - one complication here is that a lot of the tests in rest.py rely on it. Alternatively a simpler input syntax could be chosen that does not rely on JSON.
The other formats, as well as other REST calls are not affected as they never use JSON as an input format.
Issue reported by @sergiodemianlerner
Thanks for the report. I'll fix this.
Thanks!
@SergioDemianLerner: Is the DoS attack vector a JSON-Spirit issue or a general JSON parsing problem? Would using the (new) UniValue parser reduce the attack possibilities. (https://github.com/bitcoin/bitcoin/blob/master/src/univalue/univalue_read.cpp and #6121)?
@jonasschnelli IMO, the problem is fundamental with using JSON in the first place. The REST interface should have simple flat input which is easy to validate, not a writhing snake-pit of recursive complexity.
It is likely that the new JSON parser has better properties regarding out-of-memory or CPU denial of service, but I have not seen any definitive results in that direction.
But the fundamental issue remains the same. Ideally getutxos's input would just be an URI scheme like with the other REST methods.
Agreed. What i'm not sure is, if the getutxos input structure is not to complex to flat down to a standard REST URI syntax. But i'll give it a try.
What also could make sense to add some DoS counter-measures (for REST in general because it's unauthenticated).
If the input structure is fundamentally too complex, maybe we should think about removing the method, as it is not in accordance with the idea of the REST interface, which is to offer a light and simple interface. Remember that one of the persistent complaints about the original getutxos P2P proposal was, too, that it was kind of DoS sensitive (it made it possible to overflow the send buffer).
Maybe submitting a list of queries at the same time is problematic; e.g. maybe change it to query one utxo at a time?
Agreed. I initially added a limiting constant (https://github.com/bitcoin/bitcoin/blob/master/src/rest.cpp#L22). Maybe we should use a value between 1 and 15. REST clients probably should serialize queries if they need more txid to be checked.
I think the URI scheme is okay:
/rest/getutxos/checkmempool/txid-n/txid-n/txid-n/txid-n/..
or this for not querying the mempool:
/rest/getutxos/txid-n/txid-n/txid-n/txid-n/..
Allow a URI max length of 1024 chars this would basically allow to query ~14 txid per request which is reasonable IMO.
Remind me what the benefit of using HTTP and JSON over the patch I wrote is, again?
@mikehearn: this is probably a different discussion. :)
But JSON support is not the main transport format for the rest/getutxos command.
You can/should use the binary p2p serialized format described in Bip64 (as HTTP/REST inputs and output).
IMO bin (p2p serialized) over HTTP/REST is absolutely reasonable and has advantages over P2P as specified in Bip64. HTTP can easily be cached, filtered, blocked, whitelisted, etc.
@mikehearn re: offering it through the HTTP interface, at least this interface isn't exposed to every P2P peer.
I don't think there is any advantage to using JSON here, at all, which is why I'm arguing against it.
Independently from what we do with this particular API call, we should fix the JSON parser so it can be freely used without worries in any part of the code. That should be easy.
@SergioDemianLerner a new JSON parser has existed in the source tree for a while (written by @jgarzik, the whole tree is converted in #6121), it would he belpful if you could review it to see if it doesn't have the problems you encountered in json::spirit.
+1 @laanwj @SergioDemianLerner Review from a security perspective is always welcome
Example tests may be found in https://github.com/bitcoin/bitcoin/blob/master/src/test/univalue_tests.cpp
Milestone
0.11.0