Skip to content

Commit

Permalink
Allow passing $tag for non-authenticated encryption
Browse files Browse the repository at this point in the history
openssl_encrypt() currently throws a warning if the $tag out
parameter is passed for a non-authenticated cipher. This violates
the principle that a function should behave the same if a parameter
is not passed, and if the default value is passed for the parameter.

I believe this warning should simply be dropped and the $tag be
populated with null, as is already the case. Otherwise, it is not
possible to use openssl_encrypt() in generic wrapper APIs, that are
compatible with both authenticated and non-authenticated encryption.

Closes GH-6333.
  • Loading branch information
nikic committed Oct 14, 2020
1 parent d9058b6 commit 6c6a58e
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 13 deletions.
2 changes: 0 additions & 2 deletions ext/openssl/openssl.c
Expand Up @@ -6744,8 +6744,6 @@ PHP_OPENSSL_API zend_string* php_openssl_encrypt(char *data, size_t data_len, ch
}
} else if (tag) {
ZEND_TRY_ASSIGN_REF_NULL(tag);
php_error_docref(NULL, E_WARNING,
"The authenticated tag cannot be provided for cipher that doesn not support AEAD");
} else if (mode.is_aead) {
php_error_docref(NULL, E_WARNING, "A tag should be provided when using AEAD mode");
zend_string_release_ex(outbuf, 0);
Expand Down
7 changes: 7 additions & 0 deletions ext/openssl/tests/openssl_decrypt_basic.phpt
Expand Up @@ -28,9 +28,16 @@ var_dump(rtrim($output));
$encrypted = openssl_encrypt($data, "bf-ecb", $password, OPENSSL_DONT_ZERO_PAD_KEY);
$output = openssl_decrypt($encrypted, "bf-ecb", $password, OPENSSL_DONT_ZERO_PAD_KEY);
var_dump($output);

// It's okay to pass $tag for a non-authenticated cipher.
// It will be populated with null in that case.
openssl_encrypt($data, $method, $password, 0, $iv, $tag);
var_dump($tag);

?>
--EXPECT--
string(45) "openssl_encrypt() and openssl_decrypt() tests"
string(45) "openssl_encrypt() and openssl_decrypt() tests"
string(45) "openssl_encrypt() and openssl_decrypt() tests"
string(45) "openssl_encrypt() and openssl_decrypt() tests"
NULL
5 changes: 0 additions & 5 deletions ext/openssl/tests/openssl_decrypt_error.phpt
Expand Up @@ -23,8 +23,6 @@ var_dump(openssl_decrypt(array(), $method, $password));
var_dump(openssl_decrypt($encrypted, array(), $password));
var_dump(openssl_decrypt($encrypted, $method, array()));

// invalid using of an authentication tag
var_dump(openssl_encrypt($data, $method, $password, 0, $iv, $wrong));
?>
--EXPECTF--
Warning: openssl_encrypt(): Using an empty Initialization Vector (iv) is potentially insecure and not recommended in %s on line %d
Expand Down Expand Up @@ -53,6 +51,3 @@ NULL

Warning: openssl_decrypt() expects parameter 3 to be string, array given in %s on line %d
NULL

Warning: openssl_encrypt(): The authenticated tag cannot be provided for cipher that doesn not support AEAD in %s on line %d
string(44) "yof6cPPH4mLee6TOc0YQSrh4dvywMqxGUyjp0lV6+aM="
6 changes: 0 additions & 6 deletions ext/openssl/tests/openssl_encrypt_error.phpt
Expand Up @@ -21,9 +21,6 @@ var_dump(openssl_encrypt($arr, $method, $object));
var_dump(openssl_encrypt($data, $arr, $object));
var_dump(openssl_encrypt($data, $method, $arr));

// invalid using of an authentication tag
var_dump(openssl_encrypt($data, $method, $password, 0, $iv, $wrong));

// padding of the key is disabled
var_dump(openssl_encrypt($data, $method, $password, OPENSSL_DONT_ZERO_PAD_KEY, $iv));
?>
Expand All @@ -49,8 +46,5 @@ NULL
Warning: openssl_encrypt() expects parameter 3 to be string, array given in %s on line %d
NULL

Warning: openssl_encrypt(): The authenticated tag cannot be provided for cipher that doesn not support AEAD in %s on line %d
string(44) "iPR4HulskuaP5Z6me5uImk6BqVyJG73+63tkPauVZYk="

Warning: openssl_encrypt(): Key length cannot be set for the cipher method in %s on line %d
bool(false)

0 comments on commit 6c6a58e

Please sign in to comment.