john-dev - Re: auditing our use of FMT

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABtNtWEdVMuv0ivwzdeW9Ai7i6tAw9go2Ux7K9LWoS7pR0bd2Q@mail.gmail.com>
Date: Thu, 10 Sep 2015 10:57:41 +0800
From: Kai Zhao <loverszhao@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: auditing our use of FMT_* flags

Hi,

On Tue, Sep 8, 2015 at 11:42 PM, Kai Zhao <loverszhao@...il.com> wrote:
>
> I think the current patch can detect the #1, #2 error. For #3, I wrote
> a pseudo code based on your ideas.
>
>     original:
>
>         original_split_ret = split(ciphertext, 0, format);
>         original_binary_ret = binary(ciphertext);
>         original_salt_ret = salt(ciphertext);
>
>         set_key();
>
>         original_crypt_ret = crypt_all(&count, NULL);
>         original_cmp_ret = cmp_all(binary, original_crypt_all);
>
>         copy original binary -> original binary
>         copy original salt   -> original salt
>
>     change ciphertext case:
>
>         new_split_ret = split(ciphertext, 0, format);
>         new_binary_ret = binary(ciphertext);
>         new_salt_ret = salt(ciphertext);
>
>         set_key();
>
>         new_crypt_ret = crypt_all(&count, NULL);
>         new_cmp_ret = cmp_all(binary, original_crypt_all);
>
>         copy new binary -> new binary
>         copy new salt   -> new salt
>
>     compare:
>
>         if (original_split_ret != new_split_ret)
>             goto should_set_fmt_split;
>
>         if (original_binary_ret != new_binary_ret)
>             goto should_set_fmt_split;
>
>         if (original_salt_ret != new_salt_ret)
>             goto should_set_fmt_split;
>
>         if (original_crypt_ret != new_crypt_ret)
>             goto should_set_fmt_split;
>
>         if (original_cmp_ret != new_cmp_ret)
>             goto should_set_fmt_split;
>
>         if (original bianry != new binary)
>             goto should_set_fmt_split;
>
>         if (original salt != new salt)
>             goto should_set_fmt_split;
>
>     should_set_fmt_split and implement it in split()
>
> Do you think this conform to your ideas ?
>

I try to find the #3 error, here is my step:

    If (did not set FMT_SPLIT_... ) and (binary_size is not 0)

        get the original binary

        change case

        get the current binary

        if (current binary is the same as original binary)

            we should check these formats, maybe it need to add
            FMT_SPLIT... and unifies case in split()

When should unify case in split() and add FMT_SPLIT... ?

I think we should do this when the binary() is case-insensitive.
E.g. the binary() of "HAVAL-256-3" is case-insensitive, so the
return of binary() is same when we change the case of ciphertext.

If my understand is right, the 'AFS' format maybe has the #3 error.

The binary() uses atoi16[] which is case-insensitive.

void common_init(void)
{
    atoi16['A'] = atoi16['a'];
    atoi16['B'] = atoi16['b'];
    atoi16['C'] = atoi16['c'];
    atoi16['D'] = atoi16['d'];
    atoi16['E'] = atoi16['e'];
    atoi16['F'] = atoi16['f'];
}

static void *get_binary(char *ciphertext)
{
    static ARCH_WORD out[6];
    char base64[14];
    int known_long;
    int index;
    unsigned int value;

    out[0] = out[1] = 0;
    strcpy(base64, AFS_SALT);
    known_long = 0;

    for (index = 0; index < 16; index += 2) {
        value = (int)atoi16[ARCH_INDEX(ciphertext[index + 4])] << 4;
        value |= atoi16[ARCH_INDEX(ciphertext[index + 5])];

        out[index >> 3] |= (value | 1) << ((index << 2) & 0x18);

        if (atoi64[value >>= 1] == 0x7F)
            known_long = 1;
        else
            base64[(index >> 1) + 2] = value;
    }

    if (known_long)
        out[2] = ~(ARCH_WORD)0;
    else
        memcpy(&out[2], DES_std_get_binary(base64), 16);

    return out;
}


Thanks,

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