BCrypt $2b$ support in PHP

[Leigh]

Hi list,

I've submitted a patch to the PHP project to support revision 'b' of bcrypt.

Due diligence demands I seek consultation from others on
crypto-related changes, and Anthony Ferarra suggested mailing
crypto-dev in the interest of open communication, and on the
off-chance anyone is able to review the patch.

PHPs key-expansion is currently performed using an 18 x 4 nested loop,
rather than explicitly assigning the key length anywhere, which I am
assuming sidesteps the 8-bit length wrapping issue altogether.

The crypt 1.3 release states: Version 1.3 adds support for the $2b$
prefix introduced in OpenBSD 5.5+, which behaves exactly the same as
crypt_blowfish's $2y$.

As PHP already supports the 'y' revision, I again making an assumption
that supporting 'b' is as simple as using the same code path as 'y'.

Comments and/or review are both welcome and very much appreciated. The
patch is on github: https://github.com/php/php-src/pull/868

Thanks and kind regards,

Leigh.

[Solar Designer]

Hi Leigh,

Post by Leigh
I've submitted a patch to the PHP project to support revision 'b' of bcrypt.

Yeah. This is something I've been meaning to do, but never got around to.

Post by Leigh
PHPs key-expansion is currently performed using an 18 x 4 nested loop,
rather than explicitly assigning the key length anywhere, which I am
assuming sidesteps the 8-bit length wrapping issue altogether.
The crypt 1.3 release states: Version 1.3 adds support for the $2b$
prefix introduced in OpenBSD 5.5+, which behaves exactly the same as
crypt_blowfish's $2y$.
As PHP already supports the 'y' revision, I again making an assumption
that supporting 'b' is as simple as using the same code path as 'y'.

That's correct.

Post by Leigh
Comments and/or review are both welcome and very much appreciated. The
patch is on github: https://github.com/php/php-src/pull/868

While I like the simplicity of this patch, I'd prefer to merge
crypt_blowfish's more elaborate changes into its revision in PHP instead
of deviating from crypt_blowfish farther.

The additional changes would include, in patch context order:

- Updates to the lengthy comment that starts crypt_blowfish.c. I think
PHP's version of it should differ only as it relates to actual specifics
of the code in PHP.

- flags_by_subtype[] moved to global scope in that source file (but kept
static, so not exported), and used by the self-test wrapper as well.

- The more invasive changes to the self-test code, which avoid leaking
the subtype via overall timings - an extremely subtle point, probably
with no practical relevance, but it's not good for the code in PHP to
be different in this respect. In your patch, the comparison against
'x' is more or at least differently leaky.

- Extra test vectors from wrappers.c. Your added test vectors do not
check that 'b' is implemented as equivalent specifically to 'y', rather
than possibly to 'a' or 'x'.

Basically, please diff crypt_blowfish-1.2 to crypt_blowfish-1.3 and port
those changes. If any changes made between older versions of
crypt_blowfish haven't made it into the PHP tree yet, port those as well -
with a separate patch. We'll also need to take a look at how the final
code in PHP would differ from crypt_blowfish-1.3's, and make sure only
the actually intentional changes are in there. This will ease further
updates, if those are needed.

Thanks,

Alexander

[Leigh]

Alexander, many thanks for taking the time to review this.

Post by Solar Designer
While I like the simplicity of this patch, I'd prefer to merge
crypt_blowfish's more elaborate changes into its revision in PHP instead
of deviating from crypt_blowfish farther.

I feel quite dense for not even looking at your reference code, and
just stumbling blindly into it. Although at the same time quite happy
I made some of the same changes :)

Post by Solar Designer
- Extra test vectors from wrappers.c. Your added test vectors do not
check that 'b' is implemented as equivalent specifically to 'y', rather
than possibly to 'a' or 'x'.

I've now taken the new test vectors from 1.3, as well as a batch that
were completely missing which revealed some bugs in PHPs
implementation; also now fixed.

Post by Solar Designer
Basically, please diff crypt_blowfish-1.2 to crypt_blowfish-1.3 and port
those changes. If any changes made between older versions of
crypt_blowfish haven't made it into the PHP tree yet, port those as well -
with a separate patch. We'll also need to take a look at how the final
code in PHP would differ from crypt_blowfish-1.3's, and make sure only
the actually intentional changes are in there. This will ease further
updates, if those are needed.

I've reviewed the 1.1 to 1.2, and 1.2 to 1.3 diffs and am happy to say
that only changes between 1.2 and 1.3 needed to be applied. I have
preserved all of the PHP specific hacks and updated everything else to
be in line with 1.3

Again, many thanks for your time.

Leigh.