|
|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[2022-03-10 11:30 UTC] thomas dot chauchefoin at sonarsource dot com
Description:
------------
Any function whose implementation relies on php_openssl_pkey_from_zval(), php_openssl_x509_from_str() or php_openssl_csr_from_str() (openssl_x509_*(), openssl_pkey_*(), etc.) is susceptible to a NULL byte injection.
For instance, php_openssl_pkey_from_zval() supports various argument formats and has a special case when val is a string: its value can be either a path to a certificate or the certificate itself. When it’s a path, the effective file is obtained as follows:
if (Z_STRLEN_P(val) > 7 && memcmp(Z_STRVAL_P(val), "file://", sizeof("file://") - 1) == 0) {
filename = Z_STRVAL_P(val) + (sizeof("file://") - 1);
// [...]
}
It is later passed to the OpenSSL API:
if (filename) {
in = BIO_new_file(filename, PHP_OPENSSL_BIO_MODE_R(PKCS7_BINARY));
}
Zvals are binary-safe but the OpenSSL API only supports char* arguments; processing will stop at the first NULL byte.
This quirk led to CVE-2022-24715 in icingaweb2, where openssl_pkey_get_private() was used to validate the contents of a file before writing it to the local filesystem. Attackers using file:// to point to an existing certificate followed by a NULL byte allowed creating files with nearly-arbitrary contents and gain code execution on the server.
The attached patch prevents the use of NULL bytes along with file:// in any PHP function based on php_openssl_pkey_from_zval(), php_openssl_x509_from_str() or php_openssl_csr_from_str(). A test case with coverage for these three functions is included. I'm not calling every affected OpenSSL function because they all use the same code internally, e.g. PHP's openssl_pkey_get_public() is covered so openssl_pkey_get_private() is not part of the test case even if it is also vulnerable.
I marked this bug as Security because I saw it was the case for similar bug reports; I'll be happy to also submit it on GitHub if I got it wrong.
Test script:
---------------
<?php
$cert = 'file:///path/to/a/valid/pem/certificate.pem';
var_dump(openssl_pkey_get_public($cert));
var_dump(openssl_pkey_get_public("$cert\x00foo"));
Expected result:
----------------
Both calls to openssl_pkey_get_public() return a valid OpenSSLAsymmetricKey.
Actual result:
--------------
The second call to openssl_pkey_get_public() should fail.
Patchespatch-openssl-null-byte-injection-v1 (last revision 2022-03-10 11:30 UTC by thomas dot chauchefoin at sonarsource dot com)Pull Requests
Pull requests:
HistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||
Copyright © 2001-2024 The PHP GroupAll rights reserved. |
Last updated: Thu Apr 18 07:01:27 2024 UTC |
Thanks for reporting this issue! This looks indeed like a security bug. Also thanks for the PR! Generally, I think the NUL byte checks should come before the open_basedir checks. And since this would be a security fix, it needs to be applied to PHP-7.4 as well, where we can't throw ValueErrors, but would need to raise a warning instead; something like: php_error_docref(NULL, E_WARNING, "Argument #1 must not contain any null bytes when file:// is used"); For PHP-8.0 the ValueErrors are correct, but we should not return NULL afterwards, but rather employ `RETURN_THROWS()` (that clarifies intent, and also adds an assertion that an exception has actually been raised). The test case is basically fine, but the --EXTENSIONS-- section is only properly supported as of PHP 8.1.0. For older versions, one needs to add a --SKIPIF-- section, and bail out if the openssl extension is not loaded: --SKIPIF-- <?php if (!extension_loaded("openssl")) die("skip openssl extension not available"); ?> Do you want to provide an updated patch for PHP-7.4? If so, please consider to provide it as a *secret* Gist (https://gist.github.com/).> I don't see CVE-2022-24715 being related to nul injection, rather to allowing > the use of a local file. This is a combination of several factors; I'm not trying to debate whether this is a PHP security bug or not, I only want to clarify this point. There is obviously an unrelated path traversal vulnerability that shouldn't have been here in the first place, but without the NULL byte injection openssl_pkey_get_private() would have been enough to prevent its exploitation. Let's consider this code, where one of the OpenSSL functions is used to validate the contents of a file before writing it to the filesystem: if (openssl_pkey_get_private($_POST['data'])) { file_put_contents($_POST['cert'], $_POST['data']); } Attackers can pass this check by using the local file construct and pointing to a certificate already present on the system, e.g. "file:///usr/local/var/mysql/private_key.pem" on my development host. On its own, this is useless for the attacker as "file:///usr/local/var/mysql/private_key.pem" will be written to the destination file. The NULL byte injection allows getting around this limitation and writing arbitrary data to the file, like "file:///usr/local/var/mysql/private_key.pem\x00<?php phpinfo();". Without this edge case, CVE-2022-24715 wouldn't have been exploitable in a way to gain arbitrary code execution. The OpenSSL extension doesn't have other ways to validate the format of keys, so this is likely to happen again if we don't patch it.> I don't think this needs a CVE, and the fix can probably go in > the next RC versions for proper validation by the community. Unfortunately, this did not happen. However, there is still a week till the next RCs. > if (openssl_pkey_get_private($_POST['data'])) { > file_put_contents($_POST['cert'], $_POST['data']); > } It seems to me the actual problem here is that openssl_pkey_get_private() (and some other openssl functions) accepts a filename *or* a key. That is, in my opinion, a bad design decision, but we cannot easily change that for BC reasons. User should be aware of this, and should check whether a filename or a key are given; likely something were we could improve the documentation.Sorry, this kinda fell off my radar, but looking at it now, adding zero check into paths probably doesn't hurt. As for this code: > if (openssl_pkey_get_private($_POST['data'])) { > file_put_contents($_POST['cert'], $_POST['data']); > } It looks like this is assuming a valid cert can not be a valid PHP file, but I am not sure why exactly not? The standard https://datatracker.ietf.org/doc/html/rfc7468#section-5.2 allows arbitrary text before BEGIN and after END lines, and you can easily insert PHP code there. Assuming that if the text is a valid certificate for OpenSSL then it's safe for all other purposes is just not secure. In general, cert filenames is not something that is commonly being taken from the third-party users, so I tend to agree this does not need a CVE.