I previously described the bcrypt pbkdf. The design is still the same, but yesterday djm noticed a fatal flaw in the implementation. The regress test which I created on amd64 was failing every test on i386. Obviously not good.

c is hard

I poured over the code for a while, looking for a type error somewhere. That seemed the most likely explanation. Some int vs long mixup. Couldn’t find it. Disabled a few parts of the algorithm until I was able to get consistent results. The keying code was wrong.

static void
bcrypt_hash(uint8_t sha2pass[SHA512_DIGEST_LENGTH],
    uint8_t sha2salt[SHA512_DIGEST_LENGTH], uint8_t out[BCRYPT_HASHSIZE])
{
	blf_ctx state;

	/* key expansion */
	Blowfish_initstate(&state);
	Blowfish_expandstate(&state, sha2salt, sizeof(sha2salt), sha2pass,
	    sizeof(sha2pass));

Looks right. We get two arrays, and we pass them and their size to expandstate. But, wait, oh god, no, they aren’t arrays. Despite the oh so similar to array syntax, they’re still function parameters, and are therefore pointers with different sizes on different platforms. And notably a lot shorter than intended.

It’s a stupid mistake and one I knew about, but still easy to make shuffling code around between functions. A local array suddenly becomes a not local array.

impact

The keys generated were using a lot less key material than intended. On amd64, 8 bytes or 64 bits. That’s probably not the end of the world, since the first 64 bits of a sha-512 hash should be random and unpredictable. You’d still need to make 2^63 brute force guesses. On i386, it’d be 32 bits. That kind of is the end of the world. And of course, the resulting keys wouldn’t be compatible, also bad.

Fortunately, we hadn’t started using this code yet, in part because it wasn’t fully trusted.

lessons

Crypto is hard? The bug went unobserved initially because I cheated when I created the regress test. I just ran the code and saved whatever output I got as the correct output. The keys generated looked random. They would have withstood whatever brute force testing I might have cooked up.

Don’t rush things? Even though we feel this code is an improvement over the sha-1 pbkdf, we didn’t rush to use it. My biggest fear was finding a bug exactly like this. I would have preferred no bug, but at least the damage is contained to my wounded pride.

We’re working on a second, independent implementation in a different language. That’s something I should have done sooner, but will certainly be done before anything actually uses this code.

The fix involved making the code look more like what was happening under the covers. I initially preferred the array syntax for the declaration because I thought it provided useful information to the reader, but since that information turns out to be misleading, it’s deleted.

static void
bcrypt_hash(uint8_t *sha2pass, uint8_t *sha2salt, uint8_t *out)
{
	blf_ctx state;
	size_t shalen = SHA512_DIGEST_LENGTH;

	/* key expansion */
	Blowfish_initstate(&state);
	Blowfish_expandstate(&state, sha2salt, shalen, sha2pass, shalen);