o3logon format error for salts with 8bit character #2243
Comments
|
This may not be a bug in the format. Here is the bug. within valid, we call enc_to_utf16() which has 2 code paths (one for if we are not in utf8 mode, and one when we are). Then when we load the salt, we call enc_to_utf16_be() which does the same thing, BUT this time, jtr is IN utf8 mode, so it does utf8 decoding on the user name. What we have here, is the last byte when read in utf8 says there should be 4 trailing bytes. So the convert function nulls out the string, and returns -8. That is what causes the catastrophic error. But I think fixing the bug this way is not good. I think I need to look at when that option gets set, and make sure that it's set prior to valid getting called, looking for the proper format. @magnumripper any ideas on this one??? |
|
Here is the debugging code (in valid) memcpy(tmp, ciphertext, cp-ciphertext);
tmp[cp-ciphertext] = 0;
len = enc_to_utf16((UTF16 *)cur_key_mixedcase, MAX_USERNAME_LEN+1, (unsigned char*)tmp, strlen(tmp));
printf ("%x %x %s (in valid) %d\n", options.target_enc, UTF_8, tmp, len);
if (len < 0 || len > MAX_USERNAME_LEN)
return 0;In get_salt() salt.userlen = enc_to_utf16_be(salt.user, MAX_USERNAME_LEN, tmp, cp-ciphertext);
printf ("%x %x %s (in get_salt)\n", options.target_enc, UTF_8, tmp);
printf ("%x %x %s\n", salt.userlen, 2*salt.userlen, tmp);
salt.userlen *= 2;
base64_convert(cp+1,e_b64_hex,32,salt.auth_sesskey,e_b64_raw,16,0,0);
cp = strchr(cp+1, '$') + 1;Ok, here is the output: As you can see from the 2nd line of output, the options.target_enc went from 0 to 16 (0 in valid). Also the length of this user name was 9 in valid (not using utf8 internal code), but -8 in get_salt since the processing WAS done in utf8 mode there. |
|
I wonder if this code at the bottom of john_load_conf() is wrong (it is loading wrong values, I think) /* Pre-init in case some format's prepare() needs it */
internal = options.internal_cp;
target = options.target_enc;
initUnicode(UNICODE_UNICODE);
options.internal_cp = internal;
options.target_enc = target;
options.unicode_cp = CP_UNDEF; |
|
@magnumripper please look at this change to john.c This eliminates the problem, but I am not 100% sure it is correct. In this case, when prepare and valid are called, the target_enc is set the same as when get_salt is later called. Thus this bad hash is rendered invalid, since the user name length is -8 diff --git a/src/john.c b/src/john.c
index b8ade71..98411d4 100644
--- a/src/john.c
+++ b/src/john.c
@@ -901,7 +901,10 @@ static void john_load_conf(void)
target = options.target_enc;
initUnicode(UNICODE_UNICODE);
options.internal_cp = internal;
- options.target_enc = target;
+ if (target)
+ options.target_enc = target;
+ else
+ options.target_enc = options.input_enc;
options.unicode_cp = CP_UNDEF;
}
|
|
It doesn't look right: The idea is to reset everything as it was since this happens before the real Unicode init. So this may upset the real init. But I'll have a look at this today. |
|
The reason for this pre-init is we have a hen-or-egg problem with options vs. config values vs defaults depending on format (eg. our Unicode defaults is different with LM from the ones with NT). It may be that the suggested fix is perfectly fine, but it might also screw up some handling of defaults in certain situations. |
|
That is why I tossed it out as a question. My thinking was if the user requested encoding as an option, keep it, otherwise use the default. What is happening now, is that if no option is set on command line, then 0 is set (iso-8859-1 ??) but only for prepare and the FIRST valid (i.e. finding out which format to use). Once a format is found, I think the proper value from config is set (if nothing specified on command line). But again, I do not know this logic well at all. BUT the way the comment reads, I really think that if the user did not specify anything, that defaults should be set at this point, and NOT keep it set to undefined (i.e. 0). Set the way I have it, it 100% passes -test-full=0 (in asan mode), and fully passes test suite (again in asan). BUT that does not test all nuances of using -enc= command line switches against all type formats, nor any of the other complex things about this. Also with this change, the hash @frank-dittrich provided is skipped as invalid (which it is in utf8 mode) |
|
Ok, new question. You said some formats HAVE defaults. Is this the problem, in that we simply have not set the default encoding for o3logon ? If that is the case, then we might want to look for other formats that do unicode stuff, to make sure that they are setup properly in their prepare/valid (first call). |
|
Ok, within loader, at this point, We are calling prepare and valid without calling ldr_set_encoding. Should we instead do something like this: |
|
Ok, I really think we need to look at moving all of the ldr_set_encoding(fmt) to above the prepare() function. Or else, list that any unicode based decoding based upon code page, is not valid to do within prepare() or valid() Here is an example: Note, if run without the -format, it is MUCH more confusing of a run: Here, no crash, and we get the warning from within mscash valid function. The reason we get this, is that the format found was hmac-md5. But loader keeps looking (to output warning messages). When it hits mscash valid this time, the target_enc is set, so the length returned is -4 and valid returns 0. Even though it 'appears' that the run should be running against mscash (from the input file), it is actually running against hmac-md5 which does not have the encoding problem. Putting ldr_set_encoding(fmt) call before the prepare call (when searching for the format) solves this issue. NOTE, I have no idea how this would cause other problems. NOTE, once we HAVE hammered down on a format, we probably should NOT call ldr_set_encoding(fmt) any more, OR we should save off the encoding, to restore it at the bottom of that loader function. But I really think we need to have this set before we call prepare()/valid() |
|
Here's one thing your first proposed fix breaks: Before: After: Unfortunately the Unicode heuristics and calls to initUnicode() are scattered in john_load_conf(), ldr_set_encoding(), john_load_conf_db() and finally john_init() (in that order). Reason being the mentioned hen-or-egg problem with picking defaults:
For performance reasons, I believe we have to do the latter, if anything. However, I would argue that this issue's test case would be a PEBCAK (please note that user _is being warned_ that invalid UTF-8 was seen!) so the fix is just to ensure we don't segfault. We'll load it but can't ever crack it, but user was warned so he's on his own. Same would happen with other formats if incorrect encodings are specified (or defaulted to). So I propose that dc632b3 is the only thing we actually do for now. |
If nothing else, this should be a recommendation. But how else would o3logon be able to do the right thing (given proper -enc options) in prepare()? |
I would argue right back that your 'assumption' is false. The user is NOT being warned. The only reason the warning was issued for mscash was because another format SET the encoding! Again, run that mscash hash but you have to use the -form=mscash or else hmac-md5 format will snag the processing. When you run that baby with the -format=mscash then that format is the one used, it does NOT warn at all (since it is using '0' target_enc type), and there is a cato crash. Same as was found by @frank-dittrich original test hash for this issue. Again, if you expect to not change this, then by all means, we need to document that using any enc_utf16 type stuff is NOT valid in prepare() or valid() functions period. I would much rather see this 'fixed' by some means. NOTE, this is only done spinning through loader, initially looking for a usable format. I do not think that is done for each line BEING loaded (or am I wrong in that assumption). |
|
mscash? I was talking about o3logon. The warning is seen in Frank's OP. But I see now what you mean. Anyway I DO agree we should improve this if possible. But it's definitely not a simple fix! And the slight change in o3logon improved the situation for now. |
|
@magnumripper this does nothing to fix the bug (it tries to hide it). Again, here is another example (there ARE others) |
|
I think we should close this issue and open another one for the "core issue" we are talking about now. |
|
I can agree with that. Makes sense. |
A slightly modified o3logon self test:
The text was updated successfully, but these errors were encountered: