Closed solardiz closed 4 years ago
@e-ago Would you be interested in debugging and fixing this?
The #4309 fix might or might not have worked around this issue, but either way set_key
must not overwrite its stack frame even when called with an incorrectly long key. So even if the issue is no longer reproducible as above, it still needs a separate fix.
In Jumbo, formats' code can assume that no over-long key is passed to set_key()
. If it is, it's a mode or core bug.
Thanks, @magnumripper. Do we need to let formats assume that no over-long key is passed to set_key()
, though? For slow hashes like this, it doesn't matter performance-wise, so I think it's better to use defensive programming. For fast hashes, it might sometimes matter somewhat, but when it does this indicates the format should have built-in mask support and that should be used for full performance anyway.
Do you know of specific formats that currently make this assumption, and why they choose to make it?
I'm pretty sure there are many of them, perhaps predominantly SIMD ones. Most are now using common-simd-setkey32.h
so it's only one file to fix (if we want to). It does contain this:
#ifdef DEBUG
/* This function is higly optimized and assumes that we are
never ever given a key longer than fmt_params.plaintext_length.
If we are, buffer overflows WILL happen */
if (len > PLAINTEXT_LENGTH) {
fprintf(stderr, "\n** Core bug: got len %u\n'%s'\n", len, _key);
error();
}
#endif
For fast hashes, it might sometimes matter somewhat, but when it does this indicates the format should have built-in mask support and that should be used for full performance anyway.
I can imagine how an OMP format could use built-in mask, but I'm not sure how we'd use it for fast SIMD formats?
I can imagine how an OMP format could use built-in mask, but I'm not sure how we'd use it for fast SIMD formats?
SIMD-aware mask mode is difficult, but potentially even more efficient. I had thought of something like this (before we even got mask mode) for LM hashes specifically, where we'd be altering the already transposed key bits matrix, instead of having to transpose it for each set of hash computations like we do now (byte transpose in set_key
and bit transpose in crypt_all
).
But you're right, realistically we're not getting that functionality - it's too much work and it complicates further maintenance.
When running
--test --format=opencl
on a V100 on AWS, most runs crash atBitLocker-opencl
(but one run did not). However, when running--test --format=bitlocker-opencl
, that one benchmark on its own succeeds.I caught one of those crashes in
gdb
:Looks like a stack overwrite in
set_key
with some ASCII string: