Need a reality check: Is my analysis of this VB6 Blowfish bug correct?

Question

Recently I had cause to compare Blowfish algorithms. I was comparing outputs from DI Management's library and PHP's mcrypt. I could not get them to agree in any way.

This led me on an interesting chase. According to this posting on Bruce Schneier's website, there was a sign extension bug in early versions of the Blowfish code, and it would seem that the DI Management code implements the pre-bug-report code.

The blurb in the bug report says, in part,

bfinit(char *key,int keybytes)
{
    unsigned long data;
    ...
    j=0;
    ...
        data=0;
        for(k=0;k<4;k++){
            data=(data<<8)|key[j];/* choke*/
            j+=1;
            if(j==keybytes)
                j=0;
        }
        ...
}

It chokes whenever the most significant bit of key[j] is a '1'. For example, if key[j]=0x80, key[j], a signed char, is sign extended to 0xffffff80 before it is ORed with data.

The equivalent code in the blf_Initialise function in basBlowfish.bas is

   wData = &H0
    For k = 0 To 3
        wData = uw_ShiftLeftBy8(wData) Or aKey(j)
        j = j + 1
        If j >= nKeyBytes Then j = 0

The bug-report suggests the following fix to the C code:

data<<=8;
data|=(unsigned long)key[j]&0xff;

which I've implemented in VB6 as

wData = uw_ShiftLeftBy8(wData)
wData = wData Or ( aKey(j) And &HFF )

In fact, I've written it so that both methods are used and then put in an assertion to check whether the values are the same or not, viz:

wData = uw_ShiftLeftBy8(wData)
wData = wData Or (aKey(j) And &HFF)
wDCheck = uw_ShiftLeftBy8(wData) Or aKey(j)
Debug.Assert wData = wDCheck

When aKey(j) contains 255, I get an assertion error.

Am I reading this situation aright? Is a sign-extension error occurring or am I seeing bugs that aren't there?

Strangely, the tests that come with the DI Management code appear to work correctly both with and without this change (which may mean that my search for equivalence between the two algorithms may depend on something else.)

Answer 1

If I'm reading that right (certainly not guaranteed at this hour), you do have a bug. Maybe even two. Remember that in C, type casts have a higher precendence than bitwise operations. The C code casts the signed char to an unsigned long before &ing it with 0xFF. Written verbosely:

data = (data << 8) | ( ((unsigned long)key[j]) & 0xFF );

However, the VB code you posted is equivalent to:

wData = (wData << 8) | (unsigned long)(aKey[j] & 0xFF);

Hello, sign extension.

Also, did you mean to write this?

wDCheck = uw_ShiftLeftBy8(wDCheck) Or aKey(j)

Otherwise, you're setting wDCheck using the new value of wData.