Remove (most) cryptographic algorithms from Rust #9744
Conversation
|
Are you going to make/maintain a third party repo with this stuff in it? |
|
If the Sha1 was used to create a mapping to a strictly unique ID, then SipHash can't replace it; it is not the same type of cryptographic hash function. Deduplication/Unique IDs (like git) needs a hash function like for example SHA-1, SHA-2 or BLAKE2. Regarding SipHash, Rust must simply bite the bullet and commit to supporting it right? And if we do that, I think SipHash should be available to rust users as well, with full documentation. |
|
I don't know a ton about SipHash, but, besides the small output size of 64 bits, why is it not suitable as a replacement for Sha1 for this particular purpose? As best as I can tell, SipHash is a cryptographically strong MAC function, which I believe implies it has the same basic characteristics of Sha1 (pre-image resistence, 2nd pre-image resistance, and collision resistence). It is a MAC, so it requires a key, but, using a fixed key, AFAIK, it basically acts just like Sha1 or any other cryptographic hash function. As I said, I'm no expert in SipHash, so, please correct me on anything that I'm wrong about! I certainly agree that Sha1 (or a Sha2 variant) would be a better choice. But, if those are to be removed, I'm not sure what else to use here. I guess other options would be to keep Sha1 around and maybe move it to rustpkg, but, if the goal is to get rid of cryptographic functions in the rust libraries, that doesn't seem ideal. Another option would be to write a wrapper around OpenSSL or NSS or some other library and then integrate that library into Rust. That does feel pretty heavyweight, though. I believe Rust uses SipHash to prevent DOS attacks on its hash tables. So, I don't think removing it is really possible. I don't think any of these changes increase or decrease the usability of SipHash. Was your comment mostly a general comment or is there something I did accidentally that decreased the usability of SipHash for users? |
|
I guess the other weakness of SipHash is that its very new and hasn't received anywhere near the scrutiny of Sha1 or the Sha2 family. Although, I don't know how bad that is for this particular use case. |
|
I have started writing a wrapper around OpenSSL's libcrypto for my own use. The hashing part of it seems pretty solid now, so it could save someone some time if that's the direction chosen. |
|
@huonw If this pull request is merged, I am planning on moving all of the deleted Digest functions (except maybe MD4) to their own separate module. |
|
SipHash simply doesn't have any security claim about collision resistance. (Authors write We comment that SipHash is not meant to be, and (obviously) is not, collision-resistant.) All applications of SipHash should use a secret key. What I meant was that the big disclaimer inserted in the siphash docs shouldn't be necessary. Protection against hash-flooding is a feature of Rust and it should be supported fully IMO, so no need to disclaim siphash, just describe it for what it is in the docs. |
|
OK, I'm convinced. SipHash is not a good choice. What's to be done then? Leave in the algorithms until someone merges in an OpenSSL wrapper? Something else? |
|
I'm attempting to get in contact with some security folks at Mozilla to get their opinions on what we should do about these digest functions. Let's hold off on merging until I get a response back from them. |
|
(If this crypto stuff does get moved into an external repo, adding it to Rust CI would make sure it stays up-to-date; or, at least, one is alerted to breaking changes at most 24 hours after they happen.) |
|
@huonw Rust CI does look pretty cool. I've gone ahead and created a project that has all the Digests and other crypto related work that I've done: https://github.com/DaGenix/rust-crypto. I also added it to Rust CI. |
|
I haven't forgotten about this! My emails aren't getting responded to much, but I plan on bringing this up at the next meeting again. |
|
Alright, the consensus at today's meeting was to remove all hashing algorithms that aren't used by the compiler itself, but leave the ones that are used by the compiler. I believe that this would leave sha1 around while jettisoning all of the other ones. I think we should also have a comment on the module saying that it's probably not suitable for crypto purposes, but that it will be maintained by us to fix bugs in the implementation (not security bugs). If you've run out of time recently, I can open an issue for this, but otherwise thanks for being patient! |
|
@alexcrichton - A couple questions:
|
|
@catamorphism would know more about the direction that rustpkg is going in. I think that we intend for workcache to remain part of libextra (or whatever libextra becomes), but taking a hash as a parameter and moving sha1 into rustpkg seems like a good idea to me. That being said, it certainly doesn't have to get done in this pull request, we can open an issue on this migration. I personally cannot say with 100% certainty that I'm confident that our SipHash implementation can be used for cryptographic purposes (like MACs as you mentioned), so I would be more comfortable with a message saying that we have not verified these for cryptographic use. We will continue to fix bugs and maintain these implementations, but there's no goal for us to verify the rust codebase as suitable for crypto purposes. (or at comment along the lines of that). |
|
Updated the pull request. It turns out that its easier than I though to remove the uses of Sha1 from workcache - Sha1 was only being used by two functions: one of which was unused (are there plans to add a warning for unused private functions?) and the other was only used by a test. I believe that that test was just hashing small files, so, instead of hashing the file contents, I just modified the test to use the file contents directly. This wouldn't be practical for larger files, of course. But, I think its fine for the test. I moved Sha1 and supporting code over to librustpkg. I've left in the patches that add comments to the SipHash and IsaacRng code, but I'm happy to remove those patches if that's not desired. While I was making changes, I also updated workcache_support to allocate Sha1 instances on the stack instead of the heap. |
I've put some comments about cryptographic security in #9810 (e.g. general overview and ISAAC specific), so feel free to drop the (On that note, this one needs a rebase too.) |
|
Rebased. Dropped the patch that added comments to the rand module and also the patch that changed workcache_support since both are no longer necessary. |
| @@ -523,7 +508,7 @@ fn test() { | |||
| let pth = pth.clone(); | |||
|
|
|||
| // FIXME (#9639): This needs to handle non-utf8 paths | |||
| prep.declare_input("file", pth.as_str().unwrap(), digest_file(&pth)); | |||
| prep.declare_input("file", pth.as_str().unwrap(), io::read_whole_file_str(&pth).unwrap()); | |||
There was a problem hiding this comment.
Is this intended? This doesn't seem to be calculating the hash on the contents anywhere else?
There was a problem hiding this comment.
This is the only place in libextra that Sha1 (or any other hash) is being used. The declare_input() function takes 3 parameters - a kind ("file"), name, and a value which was previously the Sha1 digest of the file. Nothing I can see in the workcache code requires that the 3rd argument be a Sha1 Digest, however. Since this is just a test case and we know that the file is going to be pretty small, what I'm thinking is that we can just use the file contents directly instead of hashing them first. If Sha1 were still in libextra, this would be a bit silly of course. But, it also seems to be a bit overkill to keep Sha1 in libextra just to support this one test case.
There was a problem hiding this comment.
Oh sorry about that! I didn't even realize that this was in a test case. Yeah this is totally not a reason to keep sha1 sticking around in libextra.
|
Rebased. Also, added a patch to fold the cryptoutil and digest modules into the sha1 module. |
…tographic purposes.
… the sha1 module.
|
Rebased to replace calls to io::read_whole_file_str() which no longer exists. |
Remove the Sha1, Sha2, MD5, and MD4 algorithms. SipHash is also cryptographically secure hash function and IsaacRng is a cryptographically secure RNG - I left those alone but removed comments that implied they were suitable for cryptographic use. I thought that MD4 was used for something by the compiler, but everything still seems to work with it removed, so, I guess not. One thing that I'm not sure about - workcache.rs and workcache_support.rs (in librustpkg) both depend on Sha1. Without Sha1, the only hash function left is SipHash, so I switched that code over to use SipHash. The output size of SipHash is only 64-bits, however - much less than 160 for Sha1. I'm not sure this is a problem. Without other cryptographic hashes in the tree, I'm not sure what else to do. I considered moved Sha1 into librustpkg, but I don't know if that makes sense. If merged, this closes #9300.
Remove the Sha1, Sha2, MD5, and MD4 algorithms. SipHash is also cryptographically secure hash function and IsaacRng is a cryptographically secure RNG - I left those alone but removed comments that implied they were suitable for cryptographic use. I thought that MD4 was used for something by the compiler, but everything still seems to work with it removed, so, I guess not.
One thing that I'm not sure about - workcache.rs and workcache_support.rs (in librustpkg) both depend on Sha1. Without Sha1, the only hash function left is SipHash, so I switched that code over to use SipHash. The output size of SipHash is only 64-bits, however - much less than 160 for Sha1. I'm not sure this is a problem. Without other cryptographic hashes in the tree, I'm not sure what else to do. I considered moved Sha1 into librustpkg, but I don't know if that makes sense.
If merged, this closes #9300.