fixes for crashes found with fuzzing

[AlekseyCherepanov]

It closes #5157. It affects the following formats:

rsvp,gpg,bitlocker,net-md5,o10glogon,o3logon,SSHA512,ike,dynamic_15,vtp,nsec3,wowsrp,SSH,hdaa,PBKDF2-HMAC-MD5,krb5asrep,scrypt,dynamic_1552,bitlocker-opencl,gpg-opencl,SSH-opencl,PBKDF2-HMAC-MD5-opencl

--test=0 and --fuzz=../run/fuzz.dic are ok.

I prepared fixes quickly, so some of them might be suboptimal.

JFYI I was caught by the following problem fixing wowsrp: I increased buffer size right in get_salt instead of change to SALT_SIZE. So salt would be truncated and there would be a crash during cracking or uncrackable hash. Self-tests do not catch such mistake: all tests worked well with smaller buffer. Fuzzer does not catch it too.

My fixes for memory leaks in #5158 introduced use-after-free for dynamic salts because ldr_free_db is called in fuzzer and in john.

==4006==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d00001bbe8 at pc 0x5604db9ddef1 bp 0x7ffc3e4223f0 sp 0x7ffc3e4223e8
READ of size 1 at 0x60d00001bbe8 thread T0
    #0 0x5604db9ddef0 in dyna_salt_remove_fp /home/user/john/src/dyna_salt.c:56
    #1 0x5604dba2dd33 in ldr_free_db /home/user/john/src/loader.c:1358
    #2 0x5604dba1ede3 in john_done /home/user/john/src/john.c:1953
    #3 0x5604dba1ede3 in main /home/user/john/src/john.c:2083

The problem affects a subset of formats using dyna_salt (approximately gpg,7z,pkzip,zip + their opencl variants).

The last commit fixes the use-after-free. To allow excessive calls to ldr_free_db, I just set db->salts to NULL in ldr_free_db. I am not sure if it is ok. But ASan does not report leaks. I would like this bit to be reviewed better.

If everything is fine, this PR is ready. Thanks!

[solardiz]

I only looked at what's in patch context. I made some nitpicks, but overall this looks fine. Thank you, Aleksey!

[solardiz]

JFYI I was caught by the following problem fixing wowsrp: I increased buffer size right in get_salt instead of change to SALT_SIZE. So salt would be truncated and there would be a crash during cracking or uncrackable hash. Self-tests do not catch such mistake: all tests worked well with smaller buffer. Fuzzer does not catch it too.

I guess this means we need to enhance the self-tests to truncate a copy of the returned salt to the format's salt_size.

However, we seem to have no way to guarantee the data following the truncated salt won't just happen to match what's in the full returned salt (unless we over-read the returned salt just to ensure we produce different e.g. first byte there, but such over-reading is unsafe, so let's not do it). So this will be a best-effort test, which is fine. We can over-allocate, say, 1 byte to put some varying value in that extra byte.

[AlekseyCherepanov]

I fixed hdaa: harden valid()... commit and rebased further commits on it. I used magit-commit-instant-fixup in emacs, that should be something like git commit --fixup=... ... + git rebase --autosquash .... So all further commits are re-written. With such number of commits, button Compare seems invaluable.

[solardiz]

With such number of commits, button Compare seems invaluable.

Yes, but it only shows cumulative changes - it would not help detect e.g. the desired changes inadvertently being in a wrong commit, or some commit introducing extra changes that another commit then removes. I wish GitHub provided a tool to easily detect those.

[AlekseyCherepanov]

I guess this means we need to enhance the self-tests to truncate a copy of the returned salt to the format's salt_size.

I guess copying into test db truncates at salt_size. The problem should be that self-tests do not include cases for maximum acceptable length.

Fuzzer could find overflow for too small buffers. Static analysis could find that buffer size is not in sync with salt_size in the format structure (also formats where buffer size might vary depending on data). Actually debugging info should contain sizes and ASan tracks such info for heap allocations, so it should be possible to check at run-time during fuzzing.

Talking about checks of additional constraints during fuzzing, I would check that result from split passes valid.

[solardiz]

self-tests do not include cases for maximum acceptable length.

I don't understand this.

I would check that result from split passes valid.

Please feel free to implement that. Thanks!

[AlekseyCherepanov]

it only shows cumulative changes
I wish GitHub provided a tool to easily detect those.

Force-pushing to keep nice "source history" seems a popular variant of workflow and GitHub is informed about it. OTOH I don't see discussions about highlighting commits with new changes to review breakdown of cumulative changes.

[AlekseyCherepanov]

self-tests do not include cases for maximum acceptable length.

Some formats has salt of variable length. Usually it is stored with padding and treated as fixed size by code outside of format. (There is dyna_salt feature allowing variable lengths without padding, but it is used for a small number of formats where really big salts can happen.) So some of formats with variable lengths of salts do not include test cases for maximum allowed length of salt. For instance, wowsrp has self-tests that use 22 bytes of salt at maximum, while old declared salt_size is 67 and new salt_size is 101.