Add an option for no zero padding of the key in openssl_encrypt and openssl_decrypt

[bukka]

This fixes a problem with filling key with \0 if it's shorter than default length of the cipher and instead prioritise setting a variable key length. That applies only on ciphers with variable key length (Blowfish, RC4 and some others). Please note that this has no influence on ciphers with defined fixed key length like AES. It fixes bug 72362 and bug 71917

The reason for a new flag is to prevent BC issues that would happen if it was done by default because the used key would be different (it means that the key would no longer filled with \0).

I targeted 7.1 which I think should be fine. It's a new option (constant) which fixes 2 bugs. @krakjoe Are you ok with such addition to 7.1?

[nikic]

To clarify, what is the current behavior when a too-short key is passed to fixed-length primitive like AES? I assume it is rejected and not padded, right?

[bukka]

Nope such key is padded with \0. Yeah I know, it's not really nice but I guess lots of code is dependent on it so we can't really change default and adding warning / notice for that is sort of out of scope for this PR. I think that it should be at least documented though...

This is meant just for variable length cipher to allow providing shorter keys where it is possible.

[nikic]

In that case I'd prefer leaving 7.1 alone and just fix this completely in next minor, without introducing an option. If people want to pad, they can go pad themselves.

Btw, we started rejecting incorrectly sized keys in mcrypt in PHP 5.6.

[bukka]

I think that it doesn't make sense for variable length cipher as the result just changes without any warning. Please consider following:

openssl_encrypt($data, 'bf-ecb', '12345678');

Then it will be different between two PHP versions and without searching changelog you won't know why. That's a big problem if you have got some old code that hasn't been changed for ages and then you just don't know why it doesn't work. So what most people probably do is that they won't update. That's just not right IMHO.

[bukka]

@nikic so are you ok with this or do we need RFC? I don't agree that this should be changed in the next minor without any migration path (e.g. first emitting notice or something like that). Especially for ciphers with variable length where would be BC break really ugly as noted above.

[nikic]

I'll trust your judgement here, no need to RFC.

[bukka]

@nikic Thanks

@krakjoe When you have time, please could you let me know if it's ok for 7.1 or you prefer master only (7.2). Cheers. ;)

[nikic]

It would be nice to have a nicer name for this option. VARIABLE_LENGTH_PRIORITY doesn't really tell me what this does. Maybe something like DONT_ZERO_PAD_KEY? (Though I guess this is confusing in that it only applies to variable-length keys, hm...)

[bukka]

Well it's not even completely correct for variable length cipher that the key won't be zero padded. It will be still padded if the key len is 0 (empty string). The only thing it does is trying to set key len first.

Thinking about that, it's still kind of weird. So how about something a little bit different. We could introduce the flag that you suggest - DONT_ZERO_PAD_KEY. If it's set, then it would try to set the key len (when shorter than default key len) and if that fails (e.g. fixed len cipher), then it would fail with warning and the function would return false. What do you think?

[nikic]

@bukka I like that, sounds like a cleaner and more comprehensive solution.

[bukka]

@nikic I modified the patch so all should be as discussed. ;)

@krakjoe I'm taking that Bugfix label as an ACK for 7.1. :) Please let know in case I'm wrong and you don't think that it's good for 7.1 or have any other objections.

[krakjoe]

@bukka fine for 7.1, sorry about the delay there ...

[bukka]

Merged via 0c707fc