[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 14 Aug 2015 10:49:15 +0800
From: Kai Zhao <loverszhao@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags
Hi Alexander,
On Fri, Aug 14, 2015 at 3:18 AM, Solar Designer <solar@...nwall.com> wrote:
> Kai,
>
> On Thu, Aug 13, 2015 at 01:57:10AM +0800, Kai Zhao wrote:
>> I changed the is_key_right() function to handle uppercasing. The truncation
>> was already supported by the old self-test. Below is my patch:
>>
>> https://github.com/loverszhaokai/JohnTheRipper/commit/29c513c8316cf310b405717b3a719ee282f38740
>
> I don't understand why you made both kinds of changes at once, instead
> of just one. This looks redundant to me, and unnecessarily reducing the
> scope of your testing.
>
> Specifically, I like how you're using FMT_CASE to choose between
> strncmp() and strncasecmp(), and I think this change alone should be
> sufficient. I don't understand why you also introduced is_test_fmt_case
> and are skipping this test when that variable is set. Is there a valid
> explanation for that, or is it in fact redundant (and should be dropped)?
Yes, I will explain it here. Why I add the is_test_fmt_case.
For example:
The LM should not set FMT_CASE. In the following steps we assume
that we set FMT_CASE for LM which is obviously wrong.
LM_fmt.c
static struct fmt_tests tests[] = {
{"$LM$a9c604d244c4e99d", "AAAAAA"},
};
When we test the "$LM$a9c604d244c4e99d", is_key_right("AAAAAA")
will return true, is_key_right("aaaaaa") will return false for the get_key()
since we have set FMT_CASE.
AAAAAA -> right
aaaaaa -> wrong
>From the results, we can conclude that LM is indeed case-sensitive.
But as we know, LM is case-insensitive.
The get_key() assumes that the FMT_CASE flag is right. So I think
when we test the FMT_CASE flag, what we need is the result
of cmp_all() not the get_key().
> When you say that "truncation was already supported", do you mean the
> use of strncmp() instead of strcmp()?
Yes.
> A more reliable test would be to
> also check that the length of the string returned by get_key() is not
> greater than plaintext_length. strncmp() treats the two strings
> equally, but for our purposes we allow for truncation of only one of
> them and not the other. In other words, if get_key() returns a string
> that is not properly NUL-terminated at plaintext_length (and presumably
> has garbage in further characters) when the plaintext was of this
> maximum length or more, we want this detected as an error.
Get it. I think what I need is to add the check that the length of the string
returned by get_key() is not greater than plaintext_length before strncmp().
Thanks,
Kai
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
