do not silently truncate the key in openssl_encrypt()

[divinity76]

Description

Silently truncating keys in security-sensitive code/API's sounds horrible.
However, given PHP's commitment to backwards-compatibility, perhaps make truncation "deprecated" for a while, and make it throw in the future?

The following code:

<?php
$cipher = "aes-128-ctr";
$data = "test";
$passphrase = "KeyLengthIs16_12";
$iv = str_repeat("\x00", openssl_cipher_iv_length($cipher));
$flags = 0;

$m1 = openssl_encrypt(
    $data,
    $cipher,
    $passphrase,
    $flags,
    $iv
);

$passphrase .= "3";
$m2 = openssl_encrypt(
    $data,
    $cipher,
    $passphrase,
    $flags,
    $iv
);
var_dump($m1 === $m2);

Resulted in this output:

bool(true);

But I expected this output instead:

Fatal error: Uncaught LengthException: cipher key is too long, this cipher expects a key of precisely 16 bytes, 17 bytes provided.

PHP Version

PHP 8.1.7

Operating System

Ubuntu 22.04

[bukka]

I think the problematic part is that we don't expose EVP_CIPHER_key_length so it might not be that easy to always get the required length. Of course for AES ciphers you can get it from the name but that's not the same for other ciphers (at least the naming format) and users would have to look that up somewhere and hard code it in the app. It might be just easier for them to generate fixed size random data and use that instead. We should probably also somehow expose EVP_MAX_KEY_LENGTH for that purpose but the can just select that value (64) as it is a single value for all ciphers.

Thinking about it really, it's not such a problem because the password should always be a random value so if it is longer than used by cipher, it's not such a big issue except you saving extra data. So not sure if it's even worth it.

[divinity76]

Maybe a openssl_cipher_key_length() method like the pre-existing openssl_cipher_iv_length() ? Fwiw @iluuu1994 's PR looks like an improvement imo

[bukka]

@divinity76 Just created a PR for addition of openssl_cipher_key_length

[divinity76]

hmm @iluuu1994 you wanna make a openssl_cipher_key_length() ?

[bukka]

It's done here: #9368

[bukka]

Why should @iluuu1994 make it too? 😄

[divinity76]

@iluuu1994 made PR #9302 , so he seemed interested in the subject, i personally have a lot (too much) to do already, i can probably make it 27 august if nobody else has by then

[bukka]

Closing as #9368 is merged now. I don't think we want to prevent the truncation as there are valid use cases for that.