Undefined Behavior Sanitizer "errors"

[claudioandre-br]

clang version 18.0.0 (https://github.com/llvm/llvm-project.git d50b56d18c96e0ce462d7236eb268c54098cbaf9)

Target CPU ......................................... x86_64 AVX2, 64-bit LE
AES-NI support ..................................... depends on OpenSSL
Target OS .......................................... linux-gnu

Optional libraries/features found:
[...]
OpenMP support ..................................... no
OpenCL support ..................................... no
[...]
Experimental code (default disabled) ............... no
ZTEX USB-FPGA module 1.15y support ................. no

Development options (these may hurt performance when enabled):
AddressSanitizer ("ASan") .......................... disabled
UndefinedBehaviorSanitizer ("UbSan") ............... enabled
Fuzzing test ....................................... no

Testing: andOTP [SHA256 32/64]... aes_gcm_plug.c:168:8: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior aes_gcm_plug.c:168:8 in 

Testing: RACF-KDFAES [KDFAES (DES + HMAC-SHA256/64 + AES-256)]...
racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int'

Testing: ZIP, WinZip [PBKDF2-SHA1 256/256 AVX2 8x2]...
zip_fmt_plug.c:144:27: runtime error: index 64 out of bounds for type 'unsigned char[60]'

[solardiz]

Testing: andOTP [SHA256 32/64]... aes_gcm_plug.c:168:8: runtime error: applying zero offset to null pointer SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior aes_gcm_plug.c:168:8 in

Per my review, this isn't even the only place where we end up doing NULL+0 in that file.

The code looks like it's from the hostap project, which Dhiru imported (with changes) in 2018. It looks like it wasn't the latest version even back then, missing some upstream changes from 2012. However, those changes don't appear to have fixed these NULL+0 issues, so I guess even upstream hostap still has those.

We might want to sync with upstream. As to NULL+0, one workaround may be to pass "" (empty string) down to functions in place of NULL pointers, which along with a length of 0 shouldn't matter anyway.

[solardiz]

Testing: RACF-KDFAES [KDFAES (DES + HMAC-SHA256/64 + AES-256)]... racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int'

This looks easy to fix, so I will.

Testing: ZIP, WinZip [PBKDF2-SHA1 256/256 AVX2 8x2]... zip_fmt_plug.c:144:27: runtime error: index 64 out of bounds for type 'unsigned char[60]'

This looks like a real bug, but it's not obvious to me what fix is right. @magnumripper you seem to have introduced this in 4320dbe38b7b951f6286731bec03863f296aeda6 so perhaps it's yours to look into?

[solardiz]

zip_fmt_plug.c:144:27: runtime error: index 64 out of bounds for type 'unsigned char[60]'

I'm puzzled as to why this is only detected by UbSan, but not ASan. Any ideas?

[claudioandre-br]

No. The interesting thing is that the error message is very clear and direct!

So I tried to debug it myself, but I couldn't [1].

[1] It's a non-OpenMP build, maybe I mixed things up when I tried it.

[solardiz]

Testing: andOTP [SHA256 32/64]... aes_gcm_plug.c:168:8: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior aes_gcm_plug.c:168:8 in 

Testing: RACF-KDFAES [KDFAES (DES + HMAC-SHA256/64 + AES-256)]...
racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int'

@claudioandre-br Please try re-enabling these tests in whatever setup you had detected the errors. These two should be fine now. Thank you!

[claudioandre-br]

They are already enabled. This is the log obtained this Monday.

echo '------------------------- UBSAN fuzzing --------------------------'

../run/john --test=0
Testing: descrypt, traditional crypt(3) [DES 256/256 AVX2]... PASS
[...]
Testing: andOTP [SHA256 32/64]... PASS
[...]
Testing: RACF [DES 32/64]... PASS
Testing: RACF-KDFAES [KDFAES (DES + HMAC-SHA256/64 + AES-256)]... PASS
Testing: radius, RADIUS authentication [MD5 32/64]... PASS
[...]
Testing: crypt, generic crypt(3) [?/64]... PASS
All 423 formats passed self-tests

[solardiz]

Testing: ZIP, WinZip [PBKDF2-SHA1 256/256 AVX2 8x2]... zip_fmt_plug.c:144:27: runtime error: index 64 out of bounds for type 'unsigned char[60]'

This looks like a real bug, but it's not obvious to me what fix is right. @magnumripper you seem to have introduced this in 4320dbe so perhaps it's yours to look into?

I tried adding an explicit check for this condition, but it does not trigger. clang/UbSan bug?

+++ b/src/zip_fmt_plug.c
@@ -141,8 +141,12 @@ static int crypt_all(int *pcount, struct db_salt *salt)
                pbkdf2_sha1_sse((const unsigned char **)pin, lens, saved_salt->salt, SALT_LENGTH(saved_salt->v.mode),
                                KEYING_ITERATIONS, pout, BLK_SZ, early_skip);
                for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
+{
+int ofs = 2 * key_len - late_skip;
+if (ofs + 2 > 3 * BLK_SZ) printf("ofs %d\n", ofs);
                        if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2))
                                something_hit = hits[i] = 1;
+}
                if (something_hit) {
                        for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
                                pout[i] = pwd_ver[i];

Tried the above in builds with gcc and clang, testing with --test, --test=0, --test-full=0.

In summary, 2 issues fixed, 1 more (above) looks like false positive. Closing.

[solardiz]

clang version 18.0.0 (https://github.com/llvm/llvm-project.git d50b56d18c96e0ce462d7236eb268c54098cbaf9)

Target CPU ......................................... x86_64 AVX2, 64-bit LE

Also tried specifically ./configure CC=clang --enable-simd=avx2. But my clang is much older.

[solardiz]

Also tried specifically ./configure CC=clang --enable-simd=avx2. But my clang is much older.

Adding --enable-ubsan to this, I get:

$ ../run/john -test=0 -form=zip
Will run 4 OpenMP threads
Testing: ZIP, WinZip [PBKDF2-SHA1 256/256 AVX2 8x2]... (4xOMP) zip_fmt_plug.c:147:27: runtime error: index 64 out of bounds for type 'unsigned char [60]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior zip_fmt_plug.c:147:27 in 
PASS

So UbSan triggers, but my own check does not.

Changing the line:

                        if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2))

to use my ofs instead of 2 * key_len - late_skip silences UbSan. In fact, the below also silences UbSan:

+++ b/src/zip_fmt_plug.c
@@ -141,7 +141,7 @@ static int crypt_all(int *pcount, struct db_salt *salt)
                pbkdf2_sha1_sse((const unsigned char **)pin, lens, saved_salt->salt, SALT_LENGTH(saved_salt->v.mode),
                                KEYING_ITERATIONS, pout, BLK_SZ, early_skip);
                for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
-                       if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2))
+                       if (!memcmp(pwd_ver[i] + 0 + 2 * key_len - late_skip, saved_salt->passverify, 2))
                                something_hit = hits[i] = 1;
                if (something_hit) {
                        for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)

[solardiz]

I think we should add the + 0 workaround (and a source code comment) to allow testing of the rest of this code with UbSan. Re-opening for this.

[claudioandre-br]

to use my ofs instead of 2 * key_len - late_skip silences UbSan. In fact, the below also silences UbSan:

So ubsan is not good at analyzing and doing "math" on this kind of "complex" line of code. Probably, parentheses would also have fixed the issue.

This is odd.

[solardiz]

Probably, parentheses would also have fixed the issue.

Moreover, this was the real issue! I'm embarrassed I didn't realize at first.

So the only UbSan bug here is that + 0 would silence it. I think it shouldn't.