-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Update HashMap docs regarding DoS protection #35371
Conversation
Because of changes to how Rust acquires randomness HashMap is not guaranteed to be DoS resistant. This commit reflects these changes in the docs themselves and provides an alternative method to creating a hash that is resistant if needed.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@bors: r+ rollup looks great, thanks a ton! |
📌 Commit 2683e84 has been approved by |
Glad I could help! |
Update HashMap docs regarding DoS protection Because of changes to how Rust acquires randomness HashMap is not guaranteed to be DoS resistant. This commit reflects these changes in the docs themselves and provides an alternative method to creating a hash that is resistant if needed. This fixes rust-lang#33817 and includes relevant information regarding changes made in rust-lang#33086
Update HashMap docs regarding DoS protection Because of changes to how Rust acquires randomness HashMap is not guaranteed to be DoS resistant. This commit reflects these changes in the docs themselves and provides an alternative method to creating a hash that is resistant if needed. This fixes rust-lang#33817 and includes relevant information regarding changes made in rust-lang#33086
How was our HashMap "guaranteed" to be HashDoS resistant before? Did we ever make that guarantee? |
@sfackler through the algorithm choice. That hasn't changed, but the source of random numbers can be weaker. |
How did our algorithm choice guarantee HashDoS resistance? What does it even mean to "guarantee resistance"? The change made was to fall back to a random source we already use on many systems. If we guaranteed resistance before, I don't see how that is no longer a guarantee. Or was it only "really" guaranteed on Linux 3.17+? Could you clarify what specifically was incorrect about the old documentation? |
I am still interested in an answer to my questions, btw. |
@sfackler sorry, it got lost somehow. My bad.
Normally, inserting into a HashMap is
This means that you wrote your code expecting roughly "DDoS resistance" here, then, is predicated on number two: how easy is it to generate collisions? "DDoS resistant hash algorithms" are then ones by which don't have the degenerate algorithmic complexity when it comes to collisions. Okay, so what's that have to do with Rust and HashMap? Well, SipHash is a "DDoS-resistant" hashing algorithm, but this property is only really guaranteed by the quality of the random numbers fed into it. If we can't guarantee that they're good enough, then SipHash can't guarantee that collisions are tough to figure out. So, you are right that it's always dependent on where those random numbers came from, and maybe we were over-stepping our bounds before. This change is just the one that brought the issue enough to the forefront to hedge a bit in the docs. Does that all make sense? |
(@gankro points out that |
( not a member, but this is a good read 👍 ) |
So I think I have two core issues here: I think the original docs hedged just fine - they even explicitly said that we make no guarantees about the quality of the random seed! I would probably weaken the guarantee of use of the highest quality RNG to say that we make a "reasonable best effort" to choose the highest quality RNG. This has nothing to do about the recent change, though, but rather the fact that we don't want to guarantee that we'll be able to immediately jump on new RNG APIs the second their introduced on all of the platforms we support. The new docs make the picture less clear by pulling in two concepts that really shouldn't be involved at all. It first says that we use a "slow" hash function. If we're going to talk about the speed of the default hash function, it really needs to be elaborated on in a way that talks about its performance relative to other hash functions in different use cases (e.g. I believe SipHash is very competitive for medium-long strings) and documents when it is appropriate to switch to a different hash function and how to. If it's mentioned off hand with no context, it seems like people will just come away with the impression that they shouldn't use HashMap because "it's slow". Secondly, it talks about "truly random numbers", which is maybe a meaningful concept in philosophy, but is certainly not in this context. I would not describe the data out get out of any RNG we're going to use as "truly random" (whatever that even means). |
To be more clear about the randomness point, the case that we fall back to |
@sfackler I would be happy to take more PRs to fix this up even more, for sure. |
Clean up hasher discussion on HashMap * We never want to make guarantees about protecting against attacks. * "True randomness" is not the right terminology to be using in this context. * There is significantly more nuance to the performance of SipHash than "somewhat slow". r? @steveklabnik Follow up to discussion on #35371
Because of changes to how Rust acquires randomness HashMap is not
guaranteed to be DoS resistant. This commit reflects these changes in
the docs themselves and provides an alternative method to creating
a hash that is resistant if needed.
This fixes #33817 and includes relevant information regarding changes made in #33086