Add binary clean test (similar to salt-clean)

[jfoug]

same as #753 but for binary data. Needed if there are more than one length data in binary, that varies with the salt (like 1 salt saying to use md5, then next saying to use sha1)

[magnumripper]

A quick check (using an identical code block as #753 but s/salt/binary/g)

diff --git a/src/formats.c b/src/formats.c
index cfff256..c3fad59 100644
--- a/src/formats.c
+++ b/src/formats.c
@@ -142,7 +142,8 @@ static char *fmt_self_test_body(struct fmt_main *format,
        char *ciphertext, *plaintext;
        int i, ntests, done, index, max, size;
        void *binary, *salt;
-       int binary_align_warned = 0, salt_align_warned = 0, salt_cleaned_warned = 0
+       int binary_align_warned = 0, salt_align_warned = 0;
+       int salt_cleaned_warned = 0, binary_cleaned_warned = 0;
 #ifndef BENCH_BUILD
        int dhirutest = 0;
        int maxlength = 0;
@@ -329,6 +330,25 @@ static char *fmt_self_test_body(struct fmt_main *format,
                        puts("Warning: binary() returned misaligned pointer");
                        binary_align_warned = 1;
                }
+
+               /* validate that binary() returns cleaned buffer */
+               memset(binary, 0xAF, format->params.binary_size);
+               binary = format->methods.binary(ciphertext);
+               if (!binary_cleaned_warned)
+               if (((unsigned char*)binary)[format->params.binary_size-1] == 0xAF)
+               {
+                       memset(binary, 0xCC, format->params.binary_size);
+                       binary = format->methods.binary(ciphertext);
+                       if (((unsigned char*)binary)[format->params.binary_size-1] 
+                               /* possibly did not clean the binary. */
+                               puts("Warning: binary() not pre-cleaning buffer");
+                               binary_cleaned_warned = 1;
+                       }
+               }
+               /* Clean up the mess we might have caused */
+               memset(binary, 0, format->params.binary_size);
+
+               binary = format->methods.binary(ciphertext);
                memcpy(binary_copy, binary, format->params.binary_size);
                binary = binary_copy;

$ ../run/john --test=0 | grep -E "FAIL|Warn"
Testing: dummy [N/A]... Warning: binary() not pre-cleaning buffer
Testing: aix-ssha1, AIX LPA {ssha1} [PBKDF2-SHA1 8x SSE2]... Warning: binary() not pre-cleaning buffer
Testing: dragonfly4-32, DragonFly BSD $4$ w/ bugs, 32-bit [SHA512 64/64 OpenSSL]... Warning: binary() not pre-cleaning buffer
Testing: dragonfly4-64, DragonFly BSD $4$ w/ bugs, 64-bit [SHA512 64/64 OpenSSL]... Warning: binary() not pre-cleaning buffer
Testing: eCryptfs [65536x SHA-512]... Warning: binary() not pre-cleaning buffer
Testing: FormSpring [sha256($s.$p) 128/128 AVX 4x]... Warning: binary() not pre-cleaning buffer
Testing: IKE, PSK [HMAC MD5/SHA1 32/64]... Warning: binary() not pre-cleaning buffer
Testing: skein-256, Skein 256 [Skein 32/64]... Warning: binary() not pre-cleaning buffer
FAILED (get_hash[0](0))
Testing: skein-512, Skein 512 [Skein 32/64]... Warning: binary() not pre-cleaning buffer
Testing: Snefru-128 [32/64]... Warning: binary() not pre-cleaning buffer
1 out of 373 tests have FAILED

[magnumripper]

Dummy format is a red herring. The binary hash functions are safe.

Perhaps we could do a more elaborate test, like verifying that binary_hash() return the same value after we polluted the buffer and re-called binary(). That would take dummy away. Not sure if it would take some real problems away too.

I'm leaning towards all of them are non-issues. We should probably not commit this.

[magnumripper]

Or maybe they really are issues regardless of binary_hash():

$ git grep -hC3 'memcmp(binary' loader.c 
                        int collisions = 0;
                        if ((current_pw = db->password_hash[pw_hash]))
                        do {
                                if (!memcmp(binary, current_pw->binary,
                                    format->params.binary_size) &&
                                    !strcmp(piece, format->methods.source(
                                    current_pw->source, current_pw->binary))) {
--
                                                    ciphertext);
                if (!current->binary) /* already marked for removal */
                        continue;
                if (memcmp(binary, current->binary, format->params.binary_size))
                        continue;
                if (strcmp(ciphertext,
                    format->methods.source(current->source, current->binary)))

[magnumripper]

No I am committing this. Snefru and Skein were real bugs for sure. And the correct fix was not a memset().

[magnumripper]

IKE fixed, although it's a bodge (it was already a bodge but now it's safer).

Formspring I'll leave to you, it's a dynamic and I couldn't spot the problem quickly.

[magnumripper]

All fixed except Formspring. GPU formats checked too.

[jfoug]

FIxed formspring, BUT I need to double check all thin formats.

For dyna, all binary_size should be 16 bytes. for dyna, all binary align should be word for dyna all salt_size should be sizeof (char*) for dyna all salt align should be word

That is as long as binary and salt are being used from dyna (I think most thin would do that). For the hybrid ones, I would need to not return dyna's binary, or salt, but memcpy the return of binary/salt into the buffer to be returned. I think I did that in net-sha1/md5

But formspring is fixed, I will dig into all other thin dyna's.

[jfoug]

Good issue, it certainly WAS wrong in formspring. I bet there are other thins that are also wrong. Likely they would blow chunks on non-align machines, in the least.

[jfoug]

Fix for formspring: 0945f63 This fixes all other dyna thin formats. 4454406

[jfoug]

NOTE, the salt-smashing on dyna's prior to setting them to sizeof(char*), the new self-test salt smashing code was smashing 'something' in the dseg (well, where ever static's live). I am NOT sure why the salt check was not catching this??

The static buffer is this:

static union x {
    unsigned char salt_p[sizeof(unsigned char*)];
    ARCH_WORD p[1];
} union_x;

Also, all binary's in dyna are 16 bytes long. That is just the way it is.

[magnumripper]

For dyna, all binary_size should be 16 bytes. for dyna, all binary align should be word for dyna all salt_size should be sizeof (char*) for dyna all salt align should be word

Maybe Dynamic could have it's own self-tests in dynamic_THIN_FORMAT_LINK(), that just verifies the above, eg. something like:

    if (pFmt->params.binary_size > 16) /* or should it be != 16? */
        fprintf(stderr, "Warning: Thin dynamic format %s using incompatible binary size\n",
                pFmt->params.label);

Would this work at all?

[jfoug]

I do not think that would work. How would I be able to do a format like net-sha1, where the semi-thick format's format structure is 20 bytes, but it allows to optionally use dyna. So when I link, that one would always warn, even though I am now handling it properly.

I might look at adding some self test stuff to THIN_LINK, but i have to be really careful.