std: Cache HashMap keys in TLS #33318
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
ping? |
|
Why is it safe to key different HashMaps with keys that are known to each differ by only 1? |
|
@briansmith: Because if it was not safe that would constitute a related-key attack, and none is known for SipHash? |
|
|
@briansmith: Fair enough on both points - Not being assured of SipHash means that it may well be an issue, and I agree that the margin of security even so is unnecessarily thin. |
|
This logic is only used by the SipHash hasher. |
|
r=me on the code and perf front. But I don't feel qualified to judge the security/DDoS protection side of this. |
Others have quoted this, but again, this is what the SipHash paper says:
Thus, the easiest thing to do is what the SipHash authors recommend: Use Think about the threat model: We assume that the attacker can add or remove arbitrary (key, value) entries from any hash table used in the program. From this, it follows that we assume the attacker can change any hash table A into any other hash table B by removing all the items from A and then copying all the entries from B into A. Thus, it seems to not help if A or B have different keys, at least under this threat model. If you have a different threat model, it would be a good idea to document it. More generally, crypto people never generate a secret key by adding a constant value to another secret key. See https://en.wikipedia.org/wiki/Related-key_attack for an introduction to why. tl;dr: Knowing the difference of two secrets can help an attacker find the value of one (usually both) secrets, even if they wouldn't be able to find the values any other way. Because no crypto people would do this, it is unlikely that somebody will seriously study the problems that may or may not occur when somebody does what is proposed in this PR because we generally assume it is a-priori wrong to do. HTH. |
|
@briansmith Thanks for the comment! That does indeed help highlight some of the tradeoffs here. AIUI, the motivation for using these distinct (but related) keys is just to avoid clients of the default hashmap from accidentally assuming that all instances share a common key -- a behavior we could conceivably want to change in the future. But it could easily be that this cure is worse than the disease, and we'd be better off just very clearly documenting that you cannot rely on the apparent determinism. We just risk de facto lock-in to that behavior, but that seems (to me) better than taking a step that could easily end up revealing hashmap keys. @rust-lang/libs Thoughts here? |
alexcrichton
added
the
T-libs-api
|
Yeah I'm not too worried about switching to a per-process key with the risk of relying on a per-process deterministic iteration order. It's just a "nice to have" to make everything nondeterministic really I think. |
|
It seems to me that we could have this per-thread without the adjustment, but maybe having inter-thread differences isn't worth the slightly higher complexity vs. just being uniform through a process. |
|
Per-thread sounds like a good compromise across the board. @alexcrichton, want to update accordingly? |
This is a rebase and extension of rust-lang#31356 where we cache the keys in thread local storage. This should give us a nice speed bost in creating hash maps along with mostly retaining the property that all maps have a nondeterministic iteration order. Closes rust-lang#27243
alexcrichton
force-pushed
the
hashmap-seed
branch
from
d7503b2 to
eaeef3d
Compare
|
Sounds like a plan to me, I've updated the PR, the comment, and I also tweaked to use |
|
Thanks! @bors: r+ |
|
📌 Commit eaeef3d has been approved by |
std: Cache HashMap keys in TLS This is a rebase and extension of #31356 where we not only cache the keys in thread local storage but we also bump each key every time a new `HashMap` is created. This should give us a nice speed bost in creating hash maps along with retaining the property that all maps have a nondeterministic iteration order. Closes #27243
bluss
added
the
relnotes
This is a rebase and extension of #31356 where we not only cache the keys in
thread local storage but we also bump each key every time a new
HashMapiscreated. This should give us a nice speed bost in creating hash maps along with
retaining the property that all maps have a nondeterministic iteration order.
Closes #27243