search

monero-project / monero

Monero: the secure, private, untraceable cryptocurrency
https://getmonero.org
Other
8.9k stars 3.1k forks source link

readme: Update minimal dependencies #9448

Open Asurar0 opened 1 month ago

Asurar0 commented 1 month ago

Monero should keep minimal versions of dependencies updated to reasonable and secure versions to avoid introducing vulnerabilities in third-party builds and stay coherent with its current building process.

The following packages have been updated in this PR:

sneurlax commented 1 month ago

To review this, should we compare these docs to the versions used in contrib?

Asurar0 commented 1 month ago

@sneurlax I'm unsure to understand the question. Do you mean that a thorough review of minimal versions used in contrib should be done ?

sneurlax commented 1 month ago

Sorry @Asurar0, I should have been more clear; it wasn't a question to you specifically but rather to the next reviewer.

I would think that these docs should match the actual dependencies used/vendored in contrib, but I'm not sure, so instead of asserting that confidently, I phrased it as a question as a way to pass it on up to someone with better domain-specific knowledge on dependencies. Otherwise the PR looks good to me

This is not a review.

selsta commented 1 month ago

Shouldn't the minimum version be the minimum version that can be used to compile monero? Maybe we should add a recommended column but for example Libunbound constantly puts of vulnerbility fixes so I assume there is no good "recommneded" version for it other than a recent version.

Asurar0 commented 1 month ago

Shouldn't the minimum version be the minimum version that can be used to compile monero? Maybe we should add a recommended column but for example Libunbound constantly puts of vulnerbility fixes so I assume there is no good "recommneded" version for it other than a recent version.

Most of these vulnerabilities (libunbound and libzmq) are memory related and are in part mitigated by flags used in Cmake. I would argue however that some are not memory related, and therefore out of scope of the compiler. For example, ZMQ <4.3.3 suffers from CVE-2020-15166

I don't see any reason to set at minimum version any packages that either have reach EOL or include non-memory vulnerabilities. No one can use the former, and no one would want to use the latter.

MrCyjaneK commented 1 month ago

From what I can see this is only a README change, which doesn't change the fact that the code can still be build with older toolchain/with older dependencies. I don't see a huge value in this change because, as @selsta pointed out minimal version is not recommended version (well maybe for glibc it is... but this is out of the scope of this PR), it is more of a information to somebody who is willing to compile monero on a machine running older distribution.

I would consider updating contrib/depends more important because it actually changes what version of dependencies most of the people will run on their devices, and for those who build monero manually - I think that it doesn't matter to bump the version in readme if the code is still linkable against older libraries - because those usually contain charry-picked patches from distro maintainers, as can be seen in the CVE-2020-15166 example that you mention - if you use zeromq3 from debian repositories (which is on 4.3.1 currently) you will see a patch for that and other issues: https://sources.debian.org/patches/zeromq3/4.3.1-4+deb10u2/

So I vote for not artificially bumping the version numbers if the code is still compatible with older libraries.

Asurar0 commented 1 month ago

@MrCyjaneK I concur with your perspective. It may be worthwhile to consider opening a separate PR to revisit and update the minimum version requirements in contrib, ensuring they remain reasonable and aligned with current versions.

Concerning GCC 7, I would appreciate if any contributors can confirm its ability to compile monero.

tobtoht commented 1 month ago

@Asurar0 We currently use GCC 7 for release builds, so yes.

Edit: never mind, C++17 bump hasn't made it into the release branch and building master with 7 is broken.

Asurar0 commented 1 month ago

@Asurar0 We currently use GCC 7 for release builds, so yes.

In the github build workflow in release-v0.18 branch:

https://github.com/monero-project/monero/blob/b089f9ee69924882c5d14dd1a6991deb05d9d1cd/.github/workflows/build.yml#L15C3-L15C20

The workflow includes the installation of the build-essential package, which in turn requires gcc and g++. According to my understanding, on Ubuntu 20.04, these packages correspond to gcc version 9. However, I'm not familiar with Github actions so I may have misinterpreted the yaml file.

Edit: never mind, C++17 bump hasn't made it into the release branch and building master with 7 is broken.

It seems like there are valid reasons to keep minimal dependencies as is because of linux distribution packaging. Unless GCC is wished to be raised in the future or contrib investigated, we can close this PR. @selsta Recommended column might be a solution. Otherwise, a warning could be added at the top of this section to warn users to be aware of security patches delivered by their Linux distributions.

tobtoht commented 1 month ago

In the github build workflow in release-v0.18 branch

For releases, we use Gitian with an Ubuntu 18.04 based image, see: https://github.com/monero-project/monero/blob/master/contrib/gitian/gitian-linux.yml#L11

Minimum GCC should be bumped to 8 for master, since we can't build it with 7 (unless someone wants to fix that). This is in line with #9446.

iamamyth commented 1 month ago

A recommended column seems more harmful than helpful, as such things tend to fall out of step with reality (someone would need to monitor all the libraries and keep the recommendations up to date). Dynamic linking, plus a minimum supported version, lets users choose the best version of a library for their needs, and tends to incorporate security patches at the distro level, where appropriate.

I do think a new PR incorporating https://github.com/monero-project/monero/issues/9446 into the build process by updating the gitian base image to Ubuntu 20 and using a newer gcc version would address some of the issues raised here: The 18 base image doesn't get security updates, so there's a legitimate concern newer release builds contain too many old libraries with known vulnerabilities.

tobtoht commented 1 month ago

@iamamyth We can't bump the base image in Gitian without affecting runtime glibc requirements.

8929 replaces Gitian, uses GCC 12.3.0 and doesn't have this issue.

The 18 base image doesn't get security updates

This is less of a concern for build tools. All libraries we statically link are built with contrib/depends and their versions can be kept up to date without affecting minimum version requirements.

sneurlax commented 1 month ago

... I would consider updating contrib/depends more important because it actually changes what version of dependencies most of the people will run on their devices, and for those who build monero manually - I think that it doesn't matter to bump the version in readme if the code is still linkable against older libraries - because those usually contain charry-picked patches from distro maintainers, as can be seen in the CVE-2020-15166 example that you mention - if you use zeromq3 from debian repositories (which is on 4.3.1 currently) you will see a patch for that and other issues: https://sources.debian.org/patches/zeromq3/4.3.1-4+deb10u2/

So I vote for not artificially bumping the version numbers if the code is still compatible with older libraries.

contrib/depends has had a lot of work put into it to ensure that monero builds on all systems (or most, heh)

contributing there makes the most sense to me and that's why I initially thought that the docs should reflect contrib: because that's our shared workspace for ensuring builds work for all the various supported platforms

tobtoht commented 1 month ago

contrib/depends has had a lot of work put into it to ensure that monero builds on all systems (or most, heh)

It requires host toolchains. Currently building master on 18.04 (GCC 7) with depends is broken, too.

For reference, the state of contrib/depends:

Package Depends Latest Vulnerable
android-ndk 18b 27 No
boost 1_64_0 1.86.0 No
eudev 3.2.14 - No
expat 2.6.0 2.6.2 No
gtest 1.8.1 1.15.2 No
hidapi 0.13.1 0.14.0 No
libsodium 1.0.18 1.0.20 No
libusb 1.0.27 - No
ncurses 6.1 6.5 Yes
openssl 3.0.13 3.3.1 No
protobuf 21.12 27.3 No
readline 8.0 8.2p13 No
unbound 1.19.1 1.21.0 No
zeromq 4.3.5 - No