[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150814125826.GE25121@openwall.com>
Date: Fri, 14 Aug 2015 15:58:27 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags
Kai,
On Fri, Aug 14, 2015 at 10:49:15AM +0800, Kai Zhao wrote:
> On Fri, Aug 14, 2015 at 3:18 AM, Solar Designer <solar@...nwall.com> wrote:
> > 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().
OK, this makes sense.
> > 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().
Yes, that should do.
I think this might produce some false(?) positives, though: formats that
return the original string in get_key() without truncation, even if they
effectively have truncation for the hashing. If there are any test
vectors above plaintext_length, we'll see the check fail. Maybe this
special case needs to be checked for separately (and muted?)
Thanks,
Alexander
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.
