Skip to content

Minor fixes for some formats where cmp_all() often returned non-zero #5782

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

Merged
merged 6 commits into from
May 30, 2025

Conversation

solardiz
Copy link
Member

I tried benchmarking all formats with bench.c patched to alert when cmp_all returns non-zero. Here's the list I got:

Benchmarking: BestCryptVE4, BestCrypt Volume Encryption v4 (32768, 16, 1) [scrypt Salsa20/8 128/128 AVX512VL, AES/TwoFish/Serpent/Camellia]... (4xOMP) 
Benchmarking: BKS, BouncyCastle [PKCS#12 PBE (SHA1) 512/512 AVX-512 16x]... (4xOMP) 
Benchmarking: krb4, Kerberos v4 TGT [DES 32/64]... 
Benchmarking: openssl-enc, OpenSSL "enc" encryption (AES-128, MD5) [32/64]... (4xOMP) 
Benchmarking: PKZIP [32/64]... (4xOMP) 
Benchmarking: Siemens-S7 [HMAC-SHA1 32/64]... (4xOMP) 

It's not that bad. I expected it'd be the case for many more.

Out of these:

  • BestCryptVE4 had a bug (fixed here, so would no longer get on the above list)
  • BKS turned out to have frequent collisions (true or false positives? either way, we now dynamically set FMT_NOT_EXACT)
  • krb4 only has parity check in cmp_all and real computation and check in cmp_one (not changed here, but ideally we'd make bigger changes also adding OpenMP)
  • openssl-enc is known to have frequent false positives (no good fix)
  • PKZIP only checks short checksums, deferring further checking to cmp_exact (this may be fine, albeit not optimal with OpenMP if cmp_exact is reached so often)
  • Siemens-S7 pre-checks 32 bits in cmp_all and is fast, so it's just luck that one 32-bit collision occurred for it during my testing (that's fine, could also occur for many other formats - but maybe a reminder that we could prefer ARCH_WORD or 64-bit checks now that we mostly care about performance on 64-bit archs)

Also patched here is "BKS format: With SIMD, limit final scalar loops to actual key count", but I'm not sure we should. The same could apply to some other formats.

Other observations:

In PKZIP, the chk array is always fully written to by all threads, which means the cache lines holding it are bounced between CPUs. We could want to add our any_cracked logic and conditionally memset the array instead.

In PKZIP, cmp_all uses an add loop unlike what we have in other formats. This optimizes for the case of no matches (so having to run the loop to the end anyway), but with high keys counts there are quite often some matches. Maybe this optimization is outdated by increasing key counts.

@solardiz
Copy link
Member Author

I tried benchmarking all formats with bench.c patched to alert when cmp_all returns non-zero.

I did this only for CPU formats. We could want to also test our OpenCL formats like this.

We could also want to make this a standard feature, e.g. enabled at maximum verbosity or in debugging builds.

@solardiz
Copy link
Member Author

  • BKS turned out to have frequent collisions (true or false positives? either way, we now dynamically set FMT_NOT_EXACT)

Oh, apparently I don't see comments. There it was:

                        // mackeylen is only 2 bytes, and this results in lot
                        // of collisions (which work just fine)
                        //
                        // FMT_NOT_EXACT can be turned on for BKS keystores
                        // for finding more possible passwords

@solardiz
Copy link
Member Author

We could want to also test our OpenCL formats like this.

I just did. Only 3 had cmp_all return non-zero during benchmark: argon2-opencl, keepass-argon2-opencl, and zip-opencl. For all of them this was as expected and no big deal, but I'm adding commits to implement cmp_all for argon2-opencl and zip-opencl. Maybe these loops without function call overhead per comparison are a little bit faster than proceeding to cmp_one.

@solardiz solardiz merged commit 8d60fae into openwall:bleeding-jumbo May 30, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant