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 10:49:15 +0800
From: Kai Zhao <>
Subject: Re: auditing our use of FMT_* flags

Hi Alexander,

On Fri, Aug 14, 2015 at 3:18 AM, Solar Designer <> 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:
> 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.


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()?


> 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().



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.