Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.