Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ