-
Notifications
You must be signed in to change notification settings - Fork 30
build: require CapnProto 0.7.0 or better #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. |
Even if we want to go lower, older than v0.8.1 wouldn't make sense if it has a CVE: bitcoin/bitcoin#33176 (comment) Ubuntu 22.04 LTS is still getting security updates, so they could still bump. I asked here how to make that happen: https://answers.launchpad.net/ubuntu/+source/capnproto/+question/822317 |
For <0.8.1 you're referring to this one? https://www.cve.org/CVERecord?id=CVE-2022-46149 1.0.1 also has a CVE: https://www.cve.org/CVERecord?id=CVE-2023-48230 Though you're not using any web socket functionality? |
The CI run of Sjors/bitcoin#100 should reveal if any of our non-depends machines use an older version... |
As @fanquake points out, Debian Bookworm is at 0.9.2. bitcoin/bitcoin#31802 (comment) So I could go lower, but then we have to either rely on manual testing or add Debian to the CI. In that case maybe 0.8.2 is a better choice, given hopefully Ubuntu 22.04 will bump to that. |
Lowered the documented minimum capnproto to 0.9.2 to support Debian Bookworm (manually tested as part of Bitcoin Core). Unless there's another distro out there that has 0.8.2, I think we should hold off on lowering further until Ubuntu 22.04 LTS actually ships it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK b8d4387
re: some of the comments above, I don't think the CVE should be a factor in what minimum version is required because the fixes listed in GHSA-qqff-4vw4-f6hx go all the way back to 0.5.
IMO, it'd be nice with #194 to only require version 0.7 or later, so IPC just works without headaches on a wide variety of OS's including Ubuntu 22.04. Version 0.7 has been required since #88.
But this PR looks goods good as-is and unless there are any objections I'll merge it soon. Versions of 0.7 and 0.8 already don't work currently due to the bug in bitcoin/bitcoin#33176, so this PR is just accurately documenting current requirements.
Also to be clear, just checking that capnproto version>=0.9.2 is not sufficient for checking the CVE, since versions 0.10.0, 0.10.1, and 0.10.2 also exist and were affected. It would be nice to trigger an error when compiling against an affected version, but I think doing this reliably would require a separate check. |
Ok, if you think the CVE can safely be worked around, then I'm fine with supporting older versions. And good point about it not being easy to rule out specific versions. I would sleep a bit better if this repo had test coverage for these older versions, which #194 also introduces. |
Note that 1.0.1 is the oldest version currently covered by Bitcoin Core's extensive CI.
Lowered it to 0.7.0 in anticipation of the CI improvements in #194. |
I think I need to read more to know if we are affected. https://capnproto.org/news/2022-11-30-CVE-2022-46149-security-advisory.html says "The vulnerability is exploitable only if an application performs a certain unusual set of actions." and I haven't looked into what those are. Regardless, I think the build should refuse to use any version affected by the CVE. It will just require a custom check that I can add in #194. I think I'll go ahead and merge this now to work on that. Thanks for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 30930df. Planning to follow up in #194 to actually test minimum version and error if capnproto version detected is affected by CVE-2022-46149
Although 1.0.1. is the oldest version currently covered by Bitcoin Core's extensive CI, Debian Bookwork ships 0.9.2 and #194 introduces test coverage for even older versions. 0.7 has been required since #88.
The CI run of Sjors/bitcoin#100 @ 3d55222 previously checked Bitcoin Core CI against 1.0.1 as the minimum. Lowering the minimum further should not be a problem for that CI.