php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #81713 NULL byte injection in several OpenSSL functions working with certificates
Submitted: 2022-03-10 11:30 UTC Modified: 2022-06-10 08:46 UTC
From: thomas dot chauchefoin at sonarsource dot com Assigned: bukka (profile)
Status: Closed Package: OpenSSL related
PHP Version: PHP 7.4 OS: all
Private report: No CVE-ID: None
 [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.

Patches

patch-openssl-null-byte-injection-v1 (last revision 2022-03-10 11:30 UTC by thomas dot chauchefoin at sonarsource dot com)

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2022-03-11 15:34 UTC] cmb@php.net
-Status: Open +Status: Verified -PHP Version: master-Git-2022-03-10 (Git) +PHP Version: PHP 7.4 -Assigned To: +Assigned To: cmb
 [2022-03-11 15:34 UTC] cmb@php.net
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/).
 [2022-03-12 00:00 UTC] thomas dot chauchefoin at sonarsource dot com
> 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).

I wasn't sure of how to proceed here: we are not directly in the context of a 
PHP_FUNCTION so return_value is not available and I believe that distinguishing 
this specific error case from other ones would require significant changes in 
regard of the return types of these functions.

The other option I see would be to add this check in every single function dealing 
with certificates, right after argument parsing. There is always the risk of 
missing functions and leaving this hole open for some of them. 

In this v2, I still added a check on EG(exception). I also removed the call to TMP_CLEAN,
it was useless in this case.

> 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/).

Both patches are available at https://gist.github.com/thomas-chauchefoin-sonarsource/35652b91bd72cddb65c70f3965df72ac/.
Thank you for the reviews!
 [2022-03-14 12:40 UTC] cmb@php.net
> […]  we are not directly in the context of a PHP_FUNCTION so
> return_value is not available […]

Ah, right!  I think you can drop the exception assertion in this
case.

> I also removed the call to TMP_CLEAN, it was useless in this
> case.

It might still make sense to stick with TMP_CLEAN for consistency.

> Both patches are available at
> https://gist.github.com/thomas-chauchefoin-sonarsource/35652b91bd72cddb65c70f3965df72ac/.

It seem to me that the paths in the test can't work; shouldn't
that be

    "file://$crt_file"

instead of

    "file:///$crt_file"

etc.?

Also note that the first CHECK_NULL_PATH could be replaced by the more
concise

    CHECK_ZVAL_NULL_PATH(val)

Other than that, the patches look good to me.  Thank you!
 [2022-03-14 14:23 UTC] thomas dot chauchefoin at sonarsource dot com
> Ah, right!  I think you can drop the exception assertion in this case. [...]
> It might still make sense to stick with TMP_CLEAN for consistency.

Done; I updated the patches accordingly. 

> It seem to me that the paths in the test can't work; shouldn't
that be "file://$crt_file" instead of "file:///$crt_file"

$crt_file is created using __DIR__, that's why it works. It was not required 
though, so I removed the extra slash.
 [2022-03-14 14:56 UTC] cmb@php.net
-Assigned To: cmb +Assigned To: stas
 [2022-03-14 14:56 UTC] cmb@php.net
> $crt_file is created using __DIR__, that's why it works.

On *nix systems, __DIR__ is supposed to always start with a slash,
so there would be four slashes; I find it strange that this works,
but that's unlikely to be PHP's problem.  However, on Windows
file:///C:\… wouldn't work, while file://C:\… is fine.

Anyhow, the patches look fine to me now.  Stas, could you please
handle this from here?
 [2022-03-15 07:26 UTC] remi@php.net
I don't really think is a real security exploitable issue.
Or a "low" one.

I don't see CVE-2022-24715 being related to nul injection, rather to allowing the use of a local file.

According to https://wiki.php.net/security

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.
(of course can be fix in 7.4+, and be next security release)
 [2022-03-15 14:28 UTC] thomas dot chauchefoin at sonarsource dot 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.
 [2022-04-19 11:20 UTC] cmb@php.net
> 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.
 [2022-04-19 16:37 UTC] thomas dot chauchefoin at sonarsource dot com
> Unfortunately, this did not happen. However, there is still a 
> week till the next RCs.

Is there anything I can do to help here, e.g. by creating a PR on GitHub?
 [2022-04-19 17:25 UTC] cmb@php.net
I would very much appreciate if somebody from the security team
assesses this issue, before we go public.
 [2022-04-20 06:33 UTC] stas@php.net
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.
 [2022-04-20 08:45 UTC] bukka@php.net
Please no zend_argument_value_error in those functions. We have got plans to make them re-usable for TLS context so they are kind of generic.
 [2022-04-20 09:00 UTC] bukka@php.net
Also this is very incomplete and it should be fixed for all cases having this issue (pkcs7, cms...). I have got on my list to look to properly review and test this PR from Christoph ( https://github.com/php/php-src/pull/7438 ) that is addressing slightly different issue but targeting similar places that are needed here. I might look on using more generic check that would possibly address both issues.
 [2022-05-25 21:35 UTC] stas@php.net
-Assigned To: stas +Assigned To: bukka
 [2022-05-31 19:29 UTC] bukka@php.net
The following pull request has been associated:

Patch Name: Fix bug #50293 and #81713: file path checking in OpenSSL functions
On GitHub:  https://github.com/php/php-src/pull/8667
Patch:      https://github.com/php/php-src/pull/8667.patch
 [2022-05-31 19:36 UTC] bukka@php.net
I just created a PR. It's relatively big and unfortunately hard to backport to 7.4 due to many changes so it will go only to 8.0. As it was noted this doesn't need a CVE so it's not really worth that significant extra effort to get it to 7.4 IMHO. We will sort of treat it as a normal bug though.
 [2022-06-10 08:26 UTC] bukka@php.net
-Status: Verified +Status: Closed
 [2022-06-10 08:46 UTC] bukka@php.net
Just for the reference it will land in 8.0.21 and 8.1.8
 
PHP Copyright © 2001-2022 The PHP Group
All rights reserved.
Last updated: Wed Aug 10 17:05:45 2022 UTC