-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Drop the "Warning: excessive partial hash collisions detected" for salt-only and blob-binary formats #5736
Comments
I don't recall seeing this warning for such formats, but maybe I'm not using them enough. I think muting the warning is fine for now. BTW, what about salt-only formats in general, not only FMT_BLOB? Since they have zero binary size, surely they'd also have "hash collisions" for the binaries when many are loaded at once, wouldn't they? |
I haven't seen that happen, I presume that is muted already - or maybe this isn't about FMT_BLOB at all but all salt-only ones. |
Yes, I confirmed the issue affects salt-only formats in exactly the same way. My plan is to simply bump the threshold by 10x (or 100x?) for these types of formats, so we'll still get warnings/disabling but only for higher numbers. BTW I noticed that office-opencl use |
It's not hard at all btw, trivial fix. EDIT: Oh, I meant fixing so they can have a |
I see now FMT_HUGE_INPUT formats have the exact same problems - binary hash functions not supported. |
Wow, it turns out it's not only possible and sensible, we even have it already in the WPAPSK formats. This changes my road map a little here. |
WPAPSK doesn't have any get_hash functions though. I guess the binary_hash ones can only speed up loading. I hope (and think) this is allowed/supported (or we'd need a self-test disallowing it). Edit: I'm pretty sure we could add the corresponding get_hash functions. I need to look into that. |
@solardiz isn't it the other way around though? get_hash for loading and binary_hash for comparing? Then the above setup is just weird and the binary_hash functions should be dropped unless I can implement get_hash functions. EDIT: No, binary_hash is for loading input hashes, and get_hash for what comes out of crypt_all. |
WPAPSK does its final work in cmp_all as opposed to crypt_all. I can't remember right now but that might be a necessary property of FMT_BLOB - otherwise I can't see the point of that. But in that case I guess we can't trivially implement get_hash functions. |
For salt-only formats, bump the warning threshold by x10. Closes openwall#5736
Blob formats can't use the fmt_default_(binary|get)_hash_[0..6] functions but need more elaborate functions, or plain fmt_default_binary_hash. Salt-only formats doesn't even have a binary, but are allowed to use fmt_default_(binary|get)_hash (without the trailing _0[..6]). See openwall#5736
Indeed the combo of having binary_hash but not get_hash is fully supported, and is reasonable for BLOB formats. I tweaked the OP warning, added a few trivial self-tests and fixed the bug in Office formats (implemented proper binary hash functions). |
Due to how FMT_BLOB formats (office for one) are implemented, they can't have
binary_hash
functions. Either mute that warning for them (very easy) or fix the problem (very hard, I think. Can't remember the details but I think I've tried and gave up).The text was updated successfully, but these errors were encountered: