Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list] 
Message-ID: <CABtNtWG8jsHMR1WpHP006u21sArsUc9eWef8JoXOOUrJOhupPA@mail.gmail.com>
Date: Sat, 22 Aug 2015 09:00:42 +0800
From: Kai Zhao <loverszhao@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: The cmp_all() of cq

Hi Alexander,

On Sat, Aug 22, 2015 at 2:38 AM, Solar Designer <solar@...nwall.com> wrote:
> Kai,
>
> On Sat, Aug 22, 2015 at 12:26:42AM +0800, Kai Zhao wrote:
>> On Sat, Aug 22, 2015 at 12:23 AM, JimF <jfoug@....net> wrote:
>> > On Fri, 21 Aug 2015 11:14:57 -0500, Kai Zhao <loverszhao@...il.com> wrote:
>> >
>> >> The cmp_all() of cq seems never return 0. Is this right ?
>> >>
>> >> static int cmp_all(void *binary, int count)
>> >> {
>> >>         int i = 0;
>> >>
>> >> #if defined(_OPENMP) || MAX_KEYS_PER_CRYPT > 1
>> >>         for (i = 0; i < count; ++i)
>> >> #endif
>> >>         {
>> >>                 if ((*(unsigned int*)binary) == *(unsigned
>> >> int*)crypt_key[i])
>> >>                         return 1;
>> >>         }
>> >>
>> >>         return count;
>> >> }
>> >
>> >
>> > That looks like a bug to me.  self-test does not catch this?!
>>
>> The original --test did not catch this. The new --test-full option
>> catches this.
>
> The above is a real bug (thank you for finding it!), but:
>
> 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.

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

Here maybe another bug with keyring. I think the cmp_all() is always return 1.

static int crypt_all(int *pcount, struct db_salt *salt)
{
        const int count = *pcount;
        int index = 0;

        if (any_cracked) {
                memset(cracked, 0, cracked_size);
                any_cracked = 0;
        }

#ifdef _OPENMP
#pragma omp parallel for
#endif
        for (index = 0; index < count; index+=MAX_KEYS_PER_CRYPT)
        {
                int i;
                unsigned char (*buffers)[sizeof(cur_salt->ct)];

                // This is too big to be on stack. See #1292.
                buffers = mem_alloc(MAX_KEYS_PER_CRYPT * sizeof(*buffers));

                decrypt_buffer(buffers, index);

                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;
                }
                MEM_FREE(buffers);
        }
        return count;
}

static int cmp_all(void *binary, int count)
{
        return any_cracked;
}


Thanks,

Kai

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.