bugfix: rest: avoid segfault for invalid URI #27468
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
theStack
suggested changes
Apr 16, 2023
There was a problem hiding this comment.
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.
Thanks @theStack. I'll fix it asap. |
vasild
approved these changes
Apr 17, 2023
`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.
827b14c to
11422cc
Compare
|
Updated changes:
|
|
ACK 11422cc |
theStack
reviewed
Apr 17, 2023
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 17, 2023
11422cc bugfix: rest: avoid segfault for invalid URI (pablomartin4btc) Pull request description: Minimal fix to get it promptly into 25.0 release (suggested by [stickies-v](bitcoin#27253 (review)) and supported by [vasild](bitcoin#27253 (review)) ) Please check bitcoin#27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix. ACKs for top commit: achow101: ACK 11422cc stickies-v: re-ACK 11422cc Tree-SHA512: 5af6b53fb266a12b463f960910556d5e97bc88b3c2a4f437ffa343886b38749e1eb058cf7bc64d62e82e1acf6232a186bddacd8f3b4500c87bf9e550a0153386
theStack
added a commit
to theStack/bitcoin
that referenced
this pull request
Apr 17, 2023
Prior to PR bitcoin#27468 (commit 11422cc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way).
fanquake
added a commit
that referenced
this pull request
Apr 18, 2023
…aults) 6a77d29 test: add regression tests for #27468 (invalid URI segfaults) (Sebastian Falbesoner) Pull request description: Prior to PR #27468 (commit 11422cc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way). ACKs for top commit: stickies-v: ACK 6a77d29 vasild: ACK 6a77d29 Tree-SHA512: b5dd22d7d448f92236575ea950287259795a957a3f8e364682510c7c1ede5f9d67e7daccc5146c8d0817bcb71742d49273801574bd2bb96e44a9ae5a006ac2a7
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Apr 18, 2023
`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. Github-Pull: bitcoin#27468 Rebased-From: 11422cc
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Apr 18, 2023
dc711fb doc: update 24.1 release notes (fanquake) fc8c1a8 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack) 3a26b19 bugfix: rest: avoid segfault for invalid URI (pablomartin4btc) c40b1da depends: fix compiling bdb with clang-16 on aarch64 (fanquake) 0bac52d Don't return OutputType::UNKNOWN in ParseOutputType (Pttn) Pull request description: Backports: * bitcoin#27279 (only f73782a) * bitcoin#27462 * bitcoin#27468 * bitcoin#27473 ACKs for top commit: stickies-v: ACK dc711fb hebasto: re-ACK dc711fb jonatack: ACK dc711fb Tree-SHA512: 72c673be82689e3c3a1c2564a1fdd6afe0b357b7aa8bec9524fe6999804fbccf310da0b074e647af14b753e5e695024e268fe4f69aa58747f541f7f429ebede6
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 18, 2023
…valid URI segfaults) 6a77d29 test: add regression tests for bitcoin#27468 (invalid URI segfaults) (Sebastian Falbesoner) Pull request description: Prior to PR bitcoin#27468 (commit 11422cc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way). ACKs for top commit: stickies-v: ACK 6a77d29 vasild: ACK 6a77d29 Tree-SHA512: b5dd22d7d448f92236575ea950287259795a957a3f8e364682510c7c1ede5f9d67e7daccc5146c8d0817bcb71742d49273801574bd2bb96e44a9ae5a006ac2a7
RandyMcMillan
pushed a commit
to RandyMcMillan/bitcoin
that referenced
this pull request
May 27, 2023
Prior to PR bitcoin#27468 (commit 11422cc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way).
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.