Rework the SSH CPU format

[solardiz]

This integrates the EC-specific ASN.1 checks we had into the function shared with non-EC, so that it would detect either kind of key without prior knowledge. Besides simplification, this should reduce the risk of false negatives, at a benign increase of risk of false positives. cc: @peshev @Nothing4You

I might add a few other commits to this PR, and will edit its title if so.

[solardiz]

In fact, I think the length check and the strict_mode checks are excessive. By the time we reach them, we seem to have checked at least ~55 bits. We may want to drop them in order to reduce risk of false negatives in case any valid key differs from our strict expectations.

[solardiz]

@kholia I am removing a function you added in 609a5d8 and I'm contemplating removing some more checks you had added. You might want to review. Any specific reasons why you felt strict_mode was needed?

[ghost]

A change to ssh_valid() is still required after this PR. #5746 is useless after this change.

[solardiz]

A change to ssh_valid() is still required after this PR.

Yes, I'm not fixing the OpenCL format to reject or properly handle anything extra in this PR.

[solardiz]

I think the length check and the strict_mode checks are excessive. By the time we reach them, we seem to have checked at least ~55 bits. We may want to drop them in order to reduce risk of false negatives in case any valid key differs from our strict expectations.

Added a commit to drop them, which (along with allowing ASN1_TAG_SEQUENCE for the last field we do check) allows us to enable checking most of the previously-disabled hashcat test vectors (except for one, which is for type 6 that we don't currently support and I don't yet know what it is).

[ghost]

I copied these changes to the OpenCL format and then it passed the self-test with EC AES-256-CBC enabled [1] and my random test [2].

[1] test vector provided to us by #5641 (comment)

../run/john-opencl --test -v=5 --format=ssh-opencl
Device 1: cpu-haswell-AMD Ryzen 5 3500U with Radeon Vega Mobile Gfx
Benchmarking: ssh-opencl, SSH private key [RSA/DSA/EC 3DES/AES OpenCL]... 
Loaded 15 hashes with 15 different salts to test db from test vectors

[2]

echo "" > solucao.txt

for i in {1..32}; do
    echo "-- Iteration: $i"
    string=$(tr -dc A-Za-z0-9 </dev/urandom | head -c $i)
    echo $string >> solucao.txt
    echo $string > johnpass
    openssl ecparam -name secp521r1 -genkey | openssl ec -aes256 -out "$i".key -passout file:johnpass
done

# Extracting hashes
python3 ../run/ssh2john.py *.key > ../ssh-hashes.txt

# Testing john
rm -f ../run/john.pot; ../run/john-opencl --wordlist=solucao.txt ../ssh-hashes.txt

The change itself is simple (basically a copy+paste). The problem is that I have no idea how much it does (or doesn't) affect performance on a real GPU.

If the format for the CPU is correct, the OpenCL format should also be correct.

[solardiz]

There's a lot of stuff in this PR now (15 commits). There's still a lot more room for improvement, but I think I'll stop here, unless there are regressions or major issues to fix. Then we'll need to bring the OpenCL format to parity with the CPU format, which @claudioandre-br has already started successfully experimenting with.

I'd appreciate review and testing, especially by @peshev @Nothing4You @vkhromov and indeed by @kholia @claudioandre-br @magnumripper. Thanks!

[solardiz]

This changes default benchmark speeds because the hashcat test vectors are now at the start of the array. We need to decide on which two we want to use for benchmarking, ideally two of the same type.

[solardiz]

This changes default benchmark speeds because the hashcat test vectors are now at the start of the array. We need to decide on which two we want to use for benchmarking, ideally two of the same type.

Actually, I think it's just one when we benchmark as Raw, and I've just moved a test vector to make things similar to what we had before, yet for length 7 for consistency with hashcat benchmarks.

[kholia]

LGTM.

Nice work!

I don't remember why I chose to check more bits when I originally wrote this a long time back. Yep, checking ~55 bits does seem excessive to me now.

[solardiz]

Hmm, no, this is not ready - I am in fact getting false positives, so I must have been wrong about how much we check. We'll need to figure this out.

[magnumripper]

I have many assorted private samples and archived personal keys that I can test this with in a few days.

I had fewer of those than I thought, and none were of any new/older and/or interesting kind. The output from ssh2john did not change, and they cracked just the same both before and after these changes.

I also tried digesting man pages for creating custom sample keys but ended up no wiser.

[magnumripper]

However, maybe it'd be better to run the checks to completion and print a warning asking to contact us and report the issue, so that we'd fix the code to also avoid false negatives for unlucky keys of future types like this?

I'd vote for such warning. Would it possibly be of any benefit to run such "extra" checks in eg. cmp_one()? Perhaps not considering all different types we handle here.

on a GPU we'd typically have a group of 64 work-items proceed to the second decryption if any of them passed the padding check, which per my math would happen ~22.2% of the time. Also, extra AES block decryption on a GPU is relatively more work than on a CPU with AES-NI.

Good thinking. We might also want to try opting in the bitsliced AES, which does the second block for free.

[solardiz]

However, maybe it'd be better to run the checks to completion and print a warning asking to contact us and report the issue, so that we'd fix the code to also avoid false negatives for unlucky keys of future types like this?

I'd vote for such warning.

OK, but this PR is taking too long to complete already, so I don't intend to get this in here. Also, our immediate task after merging this one should be to update the OpenCL format as well. Then we can revisit.

Would it possibly be of any benefit to run such "extra" checks in eg. cmp_one()?

No. We'd need logic to reach cmp_one in those cases first, which means we had already checked the condition.

[solardiz]

-mask='strongp?l?l?l?l?l?l?l' finds another one: strongpvlxjaor. So it wasn't just poor luck - they are in fact quite frequent.

There's more to this. This one is repeatedly "crackable" when running 8 threads, but not when running 4. Suggests we're processing uninitialized (stale) data somewhere.

It bothers me a bit that we never figured out why this false positive was unstable. Of course, we're not getting the false positive with the current set of checks. When I insert a return 0; after the first check in our current check_structure_asn1, I am able to get this false positive again (by directly specifying it as "mask"), and it's stable across different thread counts on the same system where I had previously observed it (as unstable back then). So hopefully whatever stability bug there was is now gone.

Tested with:

for N in `seq 1 9`; do OMP_NUM_THREADS=$N ./john pw-ssh -mask='strongpvlxja?l?l'; rm john.pot john.log john.rec; done

All of these crack strongpvlxjaor (with the code hack I mentioned).