john-dev - Re: KRB4: fix memset in afs_cmu

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BLU0-SMTP25467071BD9925D06C097C3FDEE0@phx.gbl>
Date: Fri, 6 Jul 2012 15:31:31 +0200
From: Frank Dittrich <frank_dittrich@...mail.com>
To: john-dev@...ts.openwall.com
Subject: Re: KRB4: fix memset in afs_cmu_StringToKey()

On 07/06/2012 12:53 PM, magnum wrote:
>> Due to this code:
>>
>>     for (i=0; i<8; i++)
>>         if (password[i] == '\0') password[i] = 'X';
>>
>> the first 8 bytes cannot contain '\0', so we can even replace strncpy
>> with memcpy.
> 
> Is it called from set_key() or is it inner loop even? If neither, I
> think we can just leave it.

It is called from set_key(), if (saved_salt->realm[0] != '\0')

static void
krb4_set_key(char *key, int index)
{
        if (saved_salt->realm[0] != '\0')
                afs_string_to_key(key, saved_salt->realm, &saved_key.key);
        else
                des_string_to_key(key, &saved_key.key);

        strnzcpy(saved_key.string, key, sizeof(saved_key.string));
}


afs_string_to_key has this code:
    if (strlen(str) > 8)
        afs_transarc_StringToKey (str, realm, key);
    else
        afs_cmu_StringToKey (str, realm, key);

So, afs_cmu_StringToKey only gets called for strlen(str) <= 8.

This means, even this statement in afs_cmu_StringToKey() is not needed:

    if (passlen > 8) passlen = 8;

But you woiuldn't know that from just looking at afs_cmu_StringToKey(),
so I wouldn't drop the if statement.

Any possible performance improvement here is apparently smaller than the
variation between different runs of

$ ./john --test=10 --format=krb4

with the same binary.

Frank

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.