bugfix: rest: avoid segfault for invalid URI #27468

1. 
   pablomartin4btc commented at 11:48 PM on April 14, 2023: member

   Minimal fix to get it promptly into 25.0 release (suggested by stickies-v and supported by vasild )

   Please check #27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix.

2. 
   DrahtBot commented at 11:48 PM on April 14, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stickies-v, achow101
   Stale ACK	vasild

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #27253 (httpserver, rest: fix segmentation fault on evhttp_uri_get_query by pablomartin4btc)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. pablomartin4btc cross-referenced this on Apr 14, 2023 from issue httpserver, rest: improving URI validation by pablomartin4btc
4. fanquake added this to the milestone 25.0 on Apr 15, 2023
5. fanquake requested review from vasild on Apr 15, 2023
6. fanquake requested review from stickies-v on Apr 15, 2023
7. stickies-v approved
8. 
   stickies-v commented at 12:52 PM on April 15, 2023: contributor

   ACK 827b14c33f39131ede35ddecde75cc052d977ec5

9. theStack changes_requested
10. 
    theStack commented at 4:19 PM on April 16, 2023: contributor

    It's still possible to provoke a segfault via the /rest/mempool endpoint, which also uses GetQueryParameter() (tested on a bitcoind -signet -rest instance):

    $ curl localhost:38332/rest/mempool/contents.json?%
    curl: (52) Empty reply from server
    
    bitcoind output:
    libc++abi: terminating with uncaught exception of type std::runtime_error: URI parsing failed, it likely contained RFC 3986 invalid characters
    Abort trap (core dumped)
    

    Should be easy to fix by catching std::runtime_error also at the proper places within rest_mempool.

11. 
    pablomartin4btc commented at 12:03 AM on April 17, 2023: member

    It's still possible to provoke a segfault via the /rest/mempool endpoint, which also uses GetQueryParameter() (tested on a bitcoind -signet -rest instance):

    Thanks @theStack. I'll fix it asap.

12. vasild approved
13. 
    vasild commented at 7:16 AM on April 17, 2023: contributor

    ACK 827b14c33f39131ede35ddecde75cc052d977ec5

14. bugfix: rest: avoid segfault for invalid URI

    `evhttp_uri_parse` can return a nullptr, for example when the URI
    contains invalid characters (e.g. "%").
    `GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
    straight into `evhttp_uri_get_query`, which means that anyone calling
    a REST endpoint in which query parameters are used (e.g. `rest_headers`)
    can cause a segfault.
    
    This bugfix is designed to be minimal and without additional behaviour change.
    Follow-up work should be done to resolve this in a more general and robust way,
    so not every endpoint has to handle it individually.

    11422cc572
15. pablomartin4btc force-pushed on Apr 17, 2023
16. 
    pablomartin4btc commented at 1:16 PM on April 17, 2023: member

    Updated changes:

    • Fixed issue detected by @theStack on the rest/mempool endpoint that was also calling GetQueryParameter().
17. fanquake requested review from stickies-v on Apr 17, 2023
18. fanquake requested review from vasild on Apr 17, 2023
19. fanquake requested review from theStack on Apr 17, 2023
20. stickies-v approved
21. 
    stickies-v commented at 1:48 PM on April 17, 2023: contributor

    re-ACK 11422cc

22. 
    achow101 commented at 1:51 PM on April 17, 2023: member

    ACK 11422cc5720c8d73a87600de8fe8abb156db80dc

23. fanquake merged this on Apr 17, 2023
24. fanquake closed this on Apr 17, 2023

    ---

25. 
    theStack commented at 2:31 PM on April 17, 2023: contributor

    post-merge ACK 11422cc5720c8d73a87600de8fe8abb156db80dc

26. sidhujag referenced this in commit 200330a67d on Apr 17, 2023
27. theStack referenced this in commit 6a77d290da on Apr 17, 2023
28. theStack cross-referenced this on Apr 17, 2023 from issue test: add regression tests for #27468 (invalid URI segfaults) by theStack
29. fanquake referenced this in commit 467fa89438 on Apr 18, 2023
30. stickies-v cross-referenced this on Apr 18, 2023 from issue [24.x] Additional backports for 24.1 by fanquake
31. fanquake referenced this in commit 3a26b19df2 on Apr 18, 2023
32. fanquake referenced this in commit 15a24781d0 on Apr 18, 2023
33. sidhujag referenced this in commit 21e773bf32 on Apr 18, 2023
34. pablomartin4btc cross-referenced this on Apr 23, 2023 from issue http, rest: improving segfault bugfix logic - optimisation by pablomartin4btc
35. RandyMcMillan referenced this in commit b027eedcb1 on May 27, 2023
36. bitcoin locked this on Apr 16, 2024

Review Requested
vasild

Milestone
25.0