[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150813191826.GA21326@openwall.com>
Date: Thu, 13 Aug 2015 22:18:26 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags
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)?
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.
> Now the LM, netlm, and those formats which does not set FMT_CASE can
> change their self-test passwords to lowercase or both lowercase and
> uppercase.
>
> When test FMT_CASE, I think we can ignore the results of get_key(), when
> we test a passwords which is differ in case, what we care is the result of
> cmp_all().
>
> I have tested this patch. It seems to work. Do you think so ?
It looks like it should work, yes, but it also appears unnecessarily
limited in what it tests. If we can test something cheaply, we should.
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.
