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.