Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Stop using MD5 checksum for hashing releases #4672

Closed
3esmit opened this issue Feb 24, 2017 · 11 comments
Closed

Stop using MD5 checksum for hashing releases #4672

3esmit opened this issue Feb 24, 2017 · 11 comments
Labels
F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. F8-enhancement 🎊 An additional feature request. M1-ci 🙉 Continuous integration. Q0-trivial 🎠 Can be fixed by anyone with access to a computer.

Comments

@3esmit
Copy link

3esmit commented Feb 24, 2017

The underlying MD5 algorithm is no longer deemed secure. Thus, while md5sum is well-suited for identifying known files in situations that are not security related, it should not be relied on if there is a chance that files have been purposefully and maliciously tampered. In the latter case, the use of a newer hashing tool such as sha256sum is recommended.

@rphmeier rphmeier added F8-enhancement 🎊 An additional feature request. M1-ci 🙉 Continuous integration. labels Feb 24, 2017
@5chdn
Copy link
Contributor

5chdn commented Mar 6, 2017

related #3574

@5chdn 5chdn added F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. and removed F8-enhancement 🎊 An additional feature request. labels Mar 14, 2017
@gavofyork
Copy link
Contributor

it is only used to allow us to distinguish releases, not to reference content and it is taken directly from the repo.

if an attacker were somehow to compromise our repo's push access without us noticing, then their ability to magically crack MD5 to allow them to insert a hacked code in there with the same git commit hash would be unnecessary anyway: push access would already have compromised our binaries.

@gavofyork gavofyork added Z9-invalid 👮‍♀️ Issue is invalid. Closer should comment why. and removed F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. M1-ci 🙉 Continuous integration. labels Mar 23, 2017
@arkpar
Copy link
Collaborator

arkpar commented Mar 23, 2017

The issue is concerning MD5 file hashes that we publish with github release notes, not git commits. Git commits are SHA1 btw.

@arkpar arkpar reopened this Mar 23, 2017
@arkpar arkpar added F8-enhancement 🎊 An additional feature request. and removed Z9-invalid 👮‍♀️ Issue is invalid. Closer should comment why. labels Mar 23, 2017
@3esmit
Copy link
Author

3esmit commented Mar 23, 2017

I've opened this issue forwarding the concern of a reddit user:
image
https://www.reddit.com/r/ethtrader/comments/5t166b/parity_152_released_with_chrome_extension/ddkt1th/

@5chdn
Copy link
Contributor

5chdn commented Mar 24, 2017

while I'm tempted to also write lol md5 checksum it's worth to note that producing a malicious binary with exactly the same md5sum (collision) is still very unlikely, even though not impossible. using sha1 also comes with a very low collision probability, but is considered slightly more secure.

the issue is that (afaik) windows utilities do not provide native support for sha256sum, and this could lead to users stop checking checksums at all. best would be signed and verified builds as described in #3574

@5chdn 5chdn added the F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. label Apr 11, 2017
@5chdn 5chdn added M1-ci 🙉 Continuous integration. Q0-trivial 🎠 Can be fixed by anyone with access to a computer. labels May 9, 2017
@5chdn
Copy link
Contributor

5chdn commented May 9, 2017

This issue is labelled with security, but has neither an assignee nor a deadline.

Please, assign this issue to someone at @paritytech/ci, attach a milestone and comment on the progress within 7 days or close as stale otherwise.

@MicahZoltu
Copy link
Contributor

@arkpar You are getting both the MD5 hash and the file from GitHub over HTTPS. If someone had the ability to compromise one, they could compromise the other. This means that if an attacker has the ability to change the file, they could simply also change the hash to match the new file. There is not an attack vector where the attacker has the ability to change the file without also having the ability to change the hash.

@5chdn
Copy link
Contributor

5chdn commented Aug 8, 2017

What's involved for switching to sha256sum? Before we leave this ticket open for another three months, I'd rather add the checksums manually for now.

@5chdn
Copy link
Contributor

5chdn commented Aug 23, 2017

@5chdn 5chdn closed this as completed Aug 23, 2017
@MicahZoltu
Copy link
Contributor

@5chdn That link took me to a verified and sha256 checksummed release earlier today, but now it isn't and 1.7.0 is showing as latest. Was the release pulled?

@5chdn
Copy link
Contributor

5chdn commented Aug 24, 2017

Yes it was pulled. Stay tuned. But I will ensure there will be sha256sums for future releases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. F8-enhancement 🎊 An additional feature request. M1-ci 🙉 Continuous integration. Q0-trivial 🎠 Can be fixed by anyone with access to a computer.
Projects
None yet
Development

No branches or pull requests

6 participants