build: require CapnProto 0.7.0 or better #193
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.
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.
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
…6f1e54 1b8d4a6f1e54 Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf5f Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d17 ci: Bump all tasks to actions/checkout@v5 486a510bbeff ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe79 Add missing thread include 98414e7d2867 ci: Add openbsd dc3ba2204606 cmake, doc: Add check for CVE-2022-46149 cb170d4913a2 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae401 ci: Add olddeps job to test old dependencies versions c4cb758eccb5 mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b06 build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
dd68d0f Squashed 'src/ipc/libmultiprocess/' changes from b4120d34bad2..1b8d4a6f1e54 (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#193 - bitcoin-core/libmultiprocess#195 - bitcoin-core/libmultiprocess#194 These changes are needed to build fix libmultiprocess build issue that happens on OpenBSD and work around an incompatibility between GCC versions <14 and cap'nproto versions <0.9 when compiling with c++20 that was fixed upstream in capnproto/capnproto#1170. The issues were reported: - #33219 - #33176 - willcl-ark/bitcoin-core-docker#43 The fixes added CI jobs upstream to catch these issues earlier. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: Sjors: ACK 323b3fd achow101: ACK 323b3fd hebasto: ACK 323b3fd, I've reproduced the subtree update locally. The two issues noted in this PR are unrelated to its changes and can be addressed separately. Tree-SHA512: 3d03693d269c04d9ed10e8dd03e8059062929f37616d974c6fdf346ee62737c990ec550e013575e7474bfa4efcead3938bf9b259d62c073d76e720ebafe4ff66
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.