Skip to content
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

monero wallet-rpc: check binaries sha256sum #673

Closed
cirocosta opened this issue Aug 22, 2021 · 6 comments · Fixed by #1572
Closed

monero wallet-rpc: check binaries sha256sum #673

cirocosta opened this issue Aug 22, 2021 · 6 comments · Fixed by #1572
Labels
security Security issue
Projects

Comments

@cirocosta
Copy link

After downloading the binaries (in case we don't see them in the filesystem already)

let response = reqwest::get(DOWNLOAD_URL).await?;

there are no checks for integrity being performed, but I believe it'd be best if we did - sure, downloading over HTTPS would indeed guarantee that we're not getting anything that's not coming from the servers we trust (as long as we trust the root CAs), but ideally we should make sure they are also what we expect them to be: the exact contents are reproducibly built by the community (e.g., see https://github.com/monero-project/gitian.sigs/blob/master/v0.17.2.0-linux/selsta/monero-linux-0.17-build.assert), not just what's said to be true by the getmonero.org servers.

My recommendation:

  1. like we have a constant for the URL to fetch binaries for each OS & arch, have a constant for their checksums
  2. pick the sums from https://www.getmonero.org/downloads/hashes.txt after validating that the signed message is valid (+ confirm looking at gitian sigs)
  3. after reqwest downloads the contents and before the extraction, pipe those bytes through sha256sum and check

Wdyt?

thx!

@cirocosta cirocosta changed the title wallet-rpc: check binaries sha256sum monero wallet-rpc: check binaries sha256sum Aug 22, 2021
@thomaseizinger
Copy link
Contributor

Yes sounds like a good idea!

Would definitely accept a PR that implements this.

As for the actual implementation, doing as much as possible in pure Rust would be preferable, i.e. we don't want to rely on the use having sha256sum installed as a binary, instead we should do the actual checksum check ourselves.

@thomaseizinger thomaseizinger added the security Security issue label Aug 23, 2021
@rishflab rishflab added this to Security Improvements in Maintenance Aug 27, 2021
@ikmckenz
Copy link
Contributor

Seems difficult to accomplish this with the current "decompress as the file is downloaded" behavior.

let mut stream = FramedRead::new(
async_compression::tokio::bufread::BzDecoder::new(StreamReader::new(byte_stream)),
BytesCodec::new(),
)

@delta1
Copy link
Collaborator

delta1 commented Feb 21, 2024

@ikmckenz maybe look into separating the download and decompress stages?

@ikmckenz
Copy link
Contributor

That's fine with me, but we might lose this "decompress as download is still happening" trick. Not that it's super important I guess, this download is a one time setup related thing.

@delta1
Copy link
Collaborator

delta1 commented Feb 22, 2024

Yeah. And since the hashes come from the same domain, we would also have to verify the pgp signature.

@ikmckenz
Copy link
Contributor

ikmckenz commented Mar 9, 2024

Since https://www.getmonero.org/downloads/hashes.txt only has the most recent releases hashes and we are rarely on the most recent copy of the cli, I propose just hardcoding the hashes for the version we use in the code and checking against that. Still offers protection of hash check, without needing to add even more complexity for pgp verification (can be done by developers on updating).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security issue
Projects
Maintenance
Security Improvements
Development

Successfully merging a pull request may close this issue.

4 participants