[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150822011620.GA7135@openwall.com>
Date: Sat, 22 Aug 2015 04:16:20 +0300
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: The cmp_all() of cq
Kai,
Please improve upon your e-mail quoting. You should be quoting just the
relevant context - not more, not less.
On testing cmp_all():
On Sat, Aug 22, 2015 at 09:00:42AM +0800, Kai Zhao wrote:
> On Sat, Aug 22, 2015 at 2:38 AM, Solar Designer <solar@...nwall.com> wrote:
> > Are you getting many false positives when trying to catch potential
> > issues like this?
>
> Yes. There is really false positive. But I only found one that is openssl-enc.
openssl-enc may have full-blown false positives, not just at cmp_all()
level. It has FMT_NOT_EXACT set to indicate that those are expected.
I am surprised you haven't found more false positives at just that
cmp_all() level.
> > cmp_all() doesn't necessarily imply that any passwords were cracked.
> > It only says that some _might_ have been cracked. So a non-zero return
> > when you didn't pass any correct passwords doesn't always indicate that
> > there's a bug.
>
> Thanks for explanation. When I test every index with incorrect passwords,
> I should check cmp_one() ? If there is one cmp_one() return 1 in this case,
> I will report a warning. Is this right ?
cmp_one() also doesn't necessarily imply that the password was cracked.
cmp_exact() does.
However, for --test-full it's a matter of us making a decision on
whether we want the tests to be aggressive (and occasionally warn about
non-issues) or not (avoiding any false positive warnings, but also
missing some real issues).
Clearly, your aggressive (even if not entirely correct) testing of
cmp_all() has already uncovered two bugs that we otherwise might have
missed. So maybe continuing to print warnings about cmp_all() is OK,
and we'd need to add a whitelist of formats for which we'd mute those
warnings (upon checking that their cmp_all() is highly prone to false
positives on purpose).
> Here maybe another bug with keyring. I think the cmp_all() is always return 1.
Yes, looks like a bug to me.
> for (i = 0; i < MAX_KEYS_PER_CRYPT; ++i) {
> if (verify_decrypted_buffer(buffers[i],
> cur_salt->crypto_size)) {
> cracked[index+i] = 1;
> }
> #ifdef _OPENMP
> #pragma omp atomic
> #endif
> any_cracked |= 1;
> }
I think these four lines:
#ifdef _OPENMP
#pragma omp atomic
#endif
any_cracked |= 1;
should be moved up to before the closing curly brace, to have them
inside the "if (verify_decrypted_buffer(..." conditional block.
Please fix it and submit a pull request.
FWIW, I just looked through the output of:
fgrep -5 'any_cracked |= 1' *.c | less
and all other instances of this pattern (in other formats) look OK to me
(no bug in those).
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.
