Redesign Self-test

[sayan1an]

Self test as of now is too stifling to implement comparisons on device. So here is a draft pseudo-code for self-test to mitigate the issue.

index = 0; max = format->params.max_keys_per_crypt;
do {
for (i = 0; i < max;  i++)
      set_key(max_len_cand, i);

binary = get_binary(test_vector);
source = get_source(test_vector);
plaintext = get_plaintext(test_vector);

/* set_key and get_key can be different for same index */
set_key(plaintext, index);

/* No hard and fast rule that k must be equal to index + 1 */
int count = index + 1;
k = crypt_all(&count, NULL/salt);

if(!cmp_all(binary, k)) error("cmp_all");

error string = "cmp_one";
for (i = 0; i < k; i++) {
      if (cmp_one(binary, i)) {
          error_string = "cmp_exact";
          if(cmp_exact(source, i)) {
                error_string = "get_hash";
                if(get_hash(i) == binary) {
                      error_string = "get_key";
                      if (get_key(i) == plaintext) {
                         error_string = NULL;
                          break;
                      }
                }
          }
   }
}
if ( i == k) error(error_string);

test_vector = next_test_vector();
index = next_index();

}  while (index < max);

Please review.

[magnumripper]

index = 0; max = format->params.max_keys_per_crypt;
do {
for (i = 0; i < max;  i++)
      set_key(max_len_cand, i);

I take it this pseudo-code imply a crypt_all() and clear_keys() after the above?

[magnumripper]

Your code still does a crypt_all() per set_key(). This is what slows down OpenCL formats' tests a lot. I think we should try to be more like real life: First set all keys, then crypt_all() and finally a lotta cmp_*() ad get_key() and other things.

[magnumripper]

BTW we have #948 for this. Unless you just want a quick hack for GPU mask/compare, you should re-post this there and close this issue.

But maybe we should just try to solve this GPU mask & compare issue (ie. this one) first, and take care of #948 at a later time. Did you try something like this pseudo code?

Hm, what does this line mean?

plaintext = get_plaintext(test_vector);

[magnumripper]

Hm, what does this line mean?

plaintext = get_plaintext(test_vector);

OK, I think it's the prepare->valid->split chain or perhaps just the prepare (with fields) stuff. It should be called fmt_get_plain() or something if we make a helper function for it.

[magnumripper]

I guess we could add what's in this block right now, but it's sort of buggy (or maybe the pseudo code just hides things).

/* No hard and fast rule that k must be equal to index + 1 */
k = crypt_all(index + 1, NULL/salt);

if(!cmp_all(binary, k)) error("cmp_all");

error string = "cmp_one";
for (i = 0; i < k; i++) {
      if (cmp_one(binary, i)) {
          error_string = "cmp_exact";
          if(cmp_exact(source, i)) {
                error_string = "get_hash";
-                if(get_hash(i) == binary) {
+                if(get_hash(i) == binary_hash(binary)) {
                      error_string = "get_key";
                      if (get_key(i) == plaintext) {
                          error_string = NULL;
                          break;
                      }
                }
          }
   }
}
if ( i == k) error(error_string);

[magnumripper]

This does not work

/* No hard and fast rule that k must be equal to index + 1 */
k = crypt_all(index + 1, NULL/salt);

You can't pass index + 1, you'd have to do something like

int count = index + 1; 
k = crypt_all(&count, NULL/salt);

From current code

        {
            int count = index + 1;
            int match = format->methods.crypt_all(&count, NULL);
/* If salt is NULL, the return value must always match *count the way it is
 * after the crypt_all() call. */
            if (match != count)
                return "crypt_all";
        }

Note that crypt_all() both can change count and return something else. Also note that when salt is NULL, the return value must indeed match count (as it is after call).

[magnumripper]

From formats.h

/*
...
 * Returns the last output index for which there might be a match (against the
 * supplied salt's hashes) plus 1.  A return value of zero indicates no match.
 * Note that output indices don't have to match input indices (although they
 * may and usually do).  The indices passed to get_key(), get_hash[](),
 * cmp_one(), and cmp_exact() must be in the 0 to crypt_all() return value
 * minus 1 range, although for infrequent status reporting get_key() may also
 * be called on indices previously supplied to set_key() as well as on indices
 * up to the updated *count minus 1 even if they're beyond this range.
 * The count passed to cmp_all() must be equal to crypt_all()'s return value.
 * If an implementation does not use the salt parameter or if salt is NULL
 * (as it may be during self-test and benchmark), the return value must always
 * match *count the way it is after the crypt_all() call.
 * The count is passed by reference and must be updated by crypt_all() if it
 * computes other than the requested count (such as if it generates additional
 * candidate passwords on its own).  The updated count is used for c/s rate
 * calculation.  The return value is thus in the 0 to updated *count range. */

[magnumripper]

We'd need to prepare a db_salt for using here...

[sayan1an]

 * If an implementation does not use the salt parameter or if salt is NULL
 * (as it may be during self-test and benchmark), the return value must always
 * match *count the way it is after the crypt_all() call.

Can't we remove this restriction ? In fact my psuedo-code tries to address this problem. I'll investigate what are the negative side effects of removing this restriction from self test.

We'd need to prepare a db_salt for using here...

I guess this won't be needed if we remove the if (match != count) return "crypt_all"; restriction altogether.

The indices passed to get_key(), get_hash[](),
 * cmp_one(), and cmp_exact() must be in the 0 to crypt_all() return value
 * minus 1 range, although for infrequent status reporting get_key() may also
 * be called on indices previously supplied to set_key() as well as on indices
 * up to the updated *count minus 1 even if they're beyond this range.

Yes, this rule is very important for implementing compare-on-device. Unfortunately, it is not strictly followed during self-test.
During self test, strlen(input key) must be equal to strlrn(get_key()) for same indices!!.

[magnumripper]

I think we should discuss this with Solar on john-dev.

[sayan1an]

Okay

[magnumripper]

During self test, strlen(input key) must be equal to strlrn(get_key()) for same indices!!.

As said on the ML, this is just the current "max length test" which is Jumbo-specific and we'd just change it as needed.

[sayan1an]

@magnumripper I have enabled the max-length test as it doesn't seem to be a major obstacle. More importantly, I have removed the "when salt is NULL...crypt_all() == *pcount" restriction in the gpu_mask2k15 branch.

I think we should start merging gpu_mask2k15 with bleeding-jumbo before the two branches diverges any further. So please test the gpu_mask2k15(at commit 3d6d628) and merge it with bleeding-jumbo.

[magnumripper]

Merged now. The raw-md4-opencl format works fine here, and is pretty fast (I think faster than the older GSOC version). However, auto-tune will fail when using mask (value picked is suitable for non-gpu-mask so watchdog kills us). Please have a look at that. Basically it should be as easy as lowering GWS and max-keys-per-crypt by the same amount as the mask "multiplier". The less easy fix would be actually using GPU mask during auto-tune, when applicable.

[magnumripper]

We got #1036 after merging. Not sure why.

[magnumripper]

#1036 is closed and I believe we can close this too.