json: fail read_string if string contains trailing garbage #6226

1. 
   laanwj commented at 7:46 am on June 3, 2015: member

   Change read_string to fail when not the entire input has been consumed. This avoids unexpected, even dangerous behavior (fixes #6223).

   The new JSON parser adapted in #6121 also solves this problem so in master this is a temporary fix, but should be backported to older releases.

   Also adds tests for the new behavior.

   This now errors as expected:

   0$ src/bitcoin-cli -testnet sendtoaddress msj42CCGruhRsFrGATiUuh25dtxYtnpbTx 1.0dsfs
   1error: Error parsing JSON:1.0dsfs
   

   Needs backport to 0.10 and 0.11. I don’t think older versions are affected, as they did manual parsing of bitcoin-cli arguments.

   Note: normally I wouldn’t change json_spirit directly as it is an upstream library, but it is going to be dropped anyway, so doing the straightforward fix makes sense

2. laanwj added the label RPC on Jun 3, 2015
3. 
   laanwj commented at 7:59 am on June 3, 2015: member

   Hm this currently breaks other things, looking into it…

   It now rejects even valid whitespace at the end, e.g.,

   0BOOST_CHECK_EQUAL(read_string_test(std::string("1.0 "), value), true);
   

   Must be a better way (there is - json-spirit’s info.full, just needs to be bubbled up no, that doesn’t work either).

   Which leads me into a jungle of boost issues… Looks like this problem :https://svn.boost.org/trac/boost/ticket/1048 “Spirit returns full = false in 1.34 if there is trailing spaces in input”

   OK I followed the instructions there and http://thread.gmane.org/gmane.comp.parsers.spirit.general/9839, this should do it.

4. laanwj force-pushed on Jun 3, 2015
5. laanwj added the label Priority High on Jun 3, 2015
6. 
   jonasschnelli commented at 9:39 am on June 3, 2015: contributor

   Nice! utACK. Will test in the next couple of hours.
7. 
   maaku commented at 10:07 am on June 3, 2015: contributor

   You should add test cases for the specific case which triggered this in the wild: interpreting an address ‘1p2pkh…’ or ‘3p2sh…’ as a value and getting 1btc / 3btc.
8. json: fail read_string if string contains trailing garbage

   Change `read_string` to fail when not the entire input has been
   consumed. This avoids unexpected, even dangerous behavior (fixes #6223).
   
   The new JSON parser adapted in #6121 also solves this problem so in
   master this is a temporary fix, but should be backported to older releases.
   
   Also adds tests for the new behavior.

   4e157fc60d
9. laanwj force-pushed on Jun 3, 2015
10. 
    laanwj commented at 10:19 am on June 3, 2015: member

    @maaku OK, added testcases that parsing bitcoin addresses as JSON will fail
11. 
    jonasschnelli commented at 10:41 am on June 3, 2015: contributor

    Tested ACK. Tested specific error behavior after #6223 and testes that nothing got broken by playing around with some rpc commands and ran the -extened version of pull tester.
12. 
    laanwj commented at 11:58 am on June 3, 2015: member

    Thanks for testing @jonasschnelli
13. laanwj merged this on Jun 3, 2015
14. laanwj closed this on Jun 3, 2015

    ---

15. laanwj referenced this in commit c7272a50bd on Jun 3, 2015
16. laanwj referenced this in commit a7dcb7eb04 on Jun 3, 2015
17. laanwj referenced this in commit 5901596548 on Jun 3, 2015
18. laanwj referenced this in commit 181771b712 on Jun 3, 2015
19. 
    laanwj commented at 1:18 pm on June 3, 2015: member

    Backported to 0.11 via a7dcb7eb04357462f65f528a01c37ec59a23ec43 and to 0.10 via 181771b71296ffc242a4301a472f5a7ad5f6cc76
20. reddink referenced this in commit 9bfca8c140 on May 27, 2020
21. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj jonasschnelli maaku

Labels
RPC/REST/ZMQ