php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #71475 openssl_seal() uninitialized memory usage
Submitted: 2016-01-28 09:42 UTC Modified: 2016-02-04 16:33 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: shm@php.net Assigned: stas (profile)
Status: Closed Package: OpenSSL related
PHP Version: 7.0.3RC1 OS:
Private report: No CVE-ID: None
 [2016-01-28 09:42 UTC] shm@php.net
Description:
------------
http://lxr.php.net/xref/PHP_7_0/ext/openssl/openssl.c#4890:


4888/* {{{ proto int openssl_seal(string data, &string sealdata, &array ekeys, array pubkeys)
4889   Seals data */
4890PHP_FUNCTION(openssl_seal)
4891{
4892    zval *pubkeys, *pubkey, *sealdata, *ekeys, *iv = NULL;
4893    HashTable *pubkeysht;
4894    EVP_PKEY **pkeys;
[...]
4895    zend_resource ** key_resources; /* so we know what to cleanup */
4905    if (zend_parse_parameters(ZEND_NUM_ARGS(), "sz/z/a/|sz/", &data, &data_len,
4906                &sealdata, &ekeys, &pubkeys, &method, &method_len, &iv) == FAILURE) {
4907        return;
4908    }
4909    pubkeysht = Z_ARRVAL_P(pubkeys);
4910    nkeys = pubkeysht ? zend_hash_num_elements(pubkeysht) : 0;
4911    if (!nkeys) {
4912        php_error_docref(NULL, E_WARNING, "Fourth argument to openssl_seal() must be a non-empty array");
4913        RETURN_FALSE;
4914    }
[...]
4935    pkeys = safe_emalloc(nkeys, sizeof(*pkeys), 0);
[...]
4939    key_resources = safe_emalloc(nkeys, sizeof(zend_resource*), 0);
4940    memset(key_resources, 0, sizeof(zend_resource*) * nkeys);
4941
4942    /* get the public keys we are using to seal this data */
4943    i = 0;
4944    ZEND_HASH_FOREACH_VAL(pubkeysht, pubkey) {
4945        pkeys[i] = php_openssl_evp_from_zval(pubkey, 1, NULL, 0, &key_resources[i]);
4946        if (pkeys[i] == NULL) {
4947            php_error_docref(NULL, E_WARNING, "not a public key (%dth member of pubkeys)", i+1);
4948            RETVAL_FALSE;
4949            goto clean_exit;
4950        }
4951        eks[i] = emalloc(EVP_PKEY_size(pkeys[i]) + 1);
4952        i++;
4953    } ZEND_HASH_FOREACH_END();
[...]
5000clean_exit:
5001    for (i=0; i<nkeys; i++) {
5002        if (key_resources[i] == NULL) {
5003            EVP_PKEY_free(pkeys[i]);
5004        }
[...]
5008    }

Let's analyze this function, in line 4939 code allocates key_resources table
followed by zeroing it, this table is used to mark keys that are intended to
be freed. key_resources table is filled by loop between lines 4944 and 4953.
nkeys is a number of elements passed to the function in pubkeys array. Now if
one of the array members is not a valid public key, then code goes to
clean_exit routine that iterates over key_resources table and frees pkeys
structures. pkeys itself is not initialized - loop starting in 4944 line is
supposed to do so, but in case of firing up clean_exit we end up with
uninitialized part of the array. Now let's recall that key_resources was
zeroed, it means that we're going to call EVAP_PKEY_free() on uninitialized
pkeys members.

The bug was introduced by commit 424aebbf3643b3fc1b1074ecddf2104cb9465f02 [1],
quick review confirms that it affects branch 7.x only, so most distros are
safe as they let cook 7.x branch for a while.

# Is it exploitable?

Well, it depends what EVP_PKEY_free does, so let's see the implementation:
[http://nxr.netbsd.org/xref/src/crypto/external/bsd/openssl/dist/crypto/evp/p_lib.c#376]

376 void EVP_PKEY_free(EVP_PKEY *x)
377 {
378     int i;
379 
380     if (x == NULL)
381         return;
382 
383     i = CRYPTO_add(&x->references, -1, CRYPTO_LOCK_EVP_PKEY);
[...]
387     if (i > 0)
388         return;
[...]
395     EVP_PKEY_free_it(x);
396     if (x->attributes)
397         sk_X509_ATTRIBUTE_pop_free(x->attributes, X509_ATTRIBUTE_free);
398     OPENSSL_free(x);
399 }

Thanks to x == NULL check it wasn't found by unit tests. One obvious way to
exploit this bug is to trigger double free and then try to mess up something,
but OpenSSL uses allocator from libc which usually deals with double free
pretty well. There's an option to manipulate memory via CRYPTO_add (as we
control x), but decreasing by 1 will not get us far. Let's dig deeper and see
the EVP_PKEY_free_it() implementation:

http://nxr.netbsd.org/xref/src/crypto/external/bsd/openssl/dist/crypto/evp/p_lib.c#EVP_PKEY_free_it

401 static void EVP_PKEY_free_it(EVP_PKEY *x)
402 {
403     if (x->ameth && x->ameth->pkey_free) {
404         x->ameth->pkey_free(x);
405         x->pkey.ptr = NULL;
406     }
[...]

404 line contains call to pkey_free(), address of which is extracted from x
pointer and comes from uninitialized memory, which under some circumstances we
control. Therefore, it can gain us code execution!

Simple crash PoC:

~/src/php-7.0.2/sapi/cli$ gdb ./php
[...]
(gdb) r -r 'str_repeat("A", 512); openssl_seal($_, $_, $_, array_fill(0,64,0));'
Starting program: /home/rj4/src/php-7.0.2/sapi/cli/php -r 'str_repeat("A", 512); openssl_seal($_, $_, $_, array_fill(0,64,0));'
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Warning: openssl_seal(): not a public key (1th member of pubkeys) in Command line code on line 1

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5a3d837 in CRYPTO_add_lock () from /lib/x86_64-linux-gnu/libcrypto.so.1.0.0
(gdb) x/i $rip
=> 0x7ffff5a3d837 <CRYPTO_add_lock+71>: add    (%r12),%r13d
(gdb) i r
[...]
r12            0x208    520
[...]
(gdb) up
#1  0x00007ffff5ad0199 in EVP_PKEY_free () from /lib/x86_64-linux-gnu/libcrypto.so.1.0.0
(gdb) 
#2  0x00000000004f0d12 in zif_openssl_seal (execute_data=0x7ffff28130d0, return_value=0x7ffff28130c0) at /home/rj4/src/php-7.0.2/ext/openssl/openssl.c:5003
5003                            EVP_PKEY_free(pkeys[i]);
(gdb) print i
$3 = 2
(gdb) print pkeys[i]
$11 = (EVP_PKEY *) 0x200
(gdb) print pkeys[i+1]
$12 = (EVP_PKEY *) 0x4141414141414141
(gdb) print pkeys[i+2]
$13 = (EVP_PKEY *) 0x4141414141414141

Patch:

openssl_seal():
        }
 
        pkeys = safe_emalloc(nkeys, sizeof(*pkeys), 0);
-       memset(pkeys, 0, sizeof(*pkeys) * nkeys);
        eksl = safe_emalloc(nkeys, sizeof(*eksl), 0);
        eks = safe_emalloc(nkeys, sizeof(*eks), 0);
        memset(eks, 0, sizeof(*eks) * nkeys);


Test script:
---------------
<?php
str_repeat("A", 512);
openssl_seal($_, $_, $_, array_fill(0,64,0));

Expected result:
----------------
PHP does not crash

Actual result:
--------------
(gdb) r -r 'str_repeat("A", 512); openssl_seal($_, $_, $_, array_fill(0,64,0));'
Starting program: /home/rj4/src/php-7.0.2/sapi/cli/php -r 'str_repeat("A", 512); openssl_seal($_, $_, $_, array_fill(0,64,0));'
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Warning: openssl_seal(): not a public key (1th member of pubkeys) in Command line code on line 1

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5a3d837 in CRYPTO_add_lock () from /lib/x86_64-linux-gnu/libcrypto.so.1.0.0
(gdb) x/i $rip
=> 0x7ffff5a3d837 <CRYPTO_add_lock+71>: add    (%r12),%r13d
(gdb) i r
[...]
r12            0x208    520
[...]
(gdb) up
#1  0x00007ffff5ad0199 in EVP_PKEY_free () from /lib/x86_64-linux-gnu/libcrypto.so.1.0.0
(gdb) 
#2  0x00000000004f0d12 in zif_openssl_seal (execute_data=0x7ffff28130d0, return_value=0x7ffff28130c0) at /home/rj4/src/php-7.0.2/ext/openssl/openssl.c:5003
5003                            EVP_PKEY_free(pkeys[i]);
(gdb) print i
$3 = 2
(gdb) print pkeys[i]
$11 = (EVP_PKEY *) 0x200
(gdb) print pkeys[i+1]
$12 = (EVP_PKEY *) 0x4141414141414141
(gdb) print pkeys[i+2]
$13 = (EVP_PKEY *) 0x4141414141414141

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-02-01 04:21 UTC] stas@php.net
-Assigned To: +Assigned To: stas
 [2016-02-01 04:21 UTC] stas@php.net
Fix added in security repo as 33b1fbbb5c0459a623ab91b492f1a37c5262329c and in https://gist.github.com/smalyshev/5e0e829f3128c7c21cd7.

Please verify.
 [2016-02-01 08:10 UTC] shm@php.net
Looks good to me, thanks.
 [2016-02-02 05:20 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2016-02-02 05:20 UTC] stas@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2016-02-02 08:29 UTC] shm@php.net
Forgot to add proper credits in submission, bug was found and analysed by Mateusz Kocielski, s1m0n and n1x0n.
 [2016-02-04 16:33 UTC] bukka@php.net
The check for key_resources[i] == NULL seems unnecessary now. I'd like to remove it as it makes the condition slightly confusing. If anyone sees a problem with that, please let me know.
 [2016-02-04 16:36 UTC] bukka@php.net
Just clarify - I mean to do this:

diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
index 04ac7e5..5bdc46c 100644
--- a/ext/openssl/openssl.c
+++ b/ext/openssl/openssl.c
@@ -5316,7 +5316,7 @@ PHP_FUNCTION(openssl_seal)
 
 clean_exit:
        for (i=0; i<nkeys; i++) {
-               if (key_resources[i] == NULL && pkeys[i] != NULL) {
+               if (pkeys[i] != NULL) {
                        EVP_PKEY_free(pkeys[i]);
                }
                if (eks[i]) {
 [2016-02-04 21:44 UTC] arekm at maven dot pl
This bug doesn't affect php5.X? I don't see fix for it in freshly released 5.x versions.
 [2016-02-05 11:52 UTC] shm@php.net
From the report

[...] 

The bug was introduced by commit 424aebbf3643b3fc1b1074ecddf2104cb9465f02 [1],
quick review confirms that it affects branch 7.x only, so most distros are
safe as they let cook 7.x branch for a while.

[...]

So branch 5.x is not affected.


----

bukka@, what about:

4939    pkeys = safe_emalloc(nkeys, sizeof(*pkeys), 0);
4940    eksl = safe_emalloc(nkeys, sizeof(*eksl), 0);
4941    eks = safe_emalloc(nkeys, sizeof(*eks), 0);
4942    memset(eks, 0, sizeof(*eks) * nkeys);
4943    key_resources = safe_emalloc(nkeys, sizeof(zend_resource*), 0);
4944    memset(key_resources, 0, sizeof(zend_resource*) * nkeys);
4945    memset(pkeys, 0, sizeof(*pkeys) * nkeys);

transforming this to ecalloc.

diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
index 04ac7e5..5bdc46c 100644
--- a/ext/openssl/openssl.c
+++ b/ext/openssl/openssl.c
@@ -5316,7 +5316,7 @@ PHP_FUNCTION(openssl_seal)
 
 clean_exit:
        for (i=0; i<nkeys; i++) {
-               if (key_resources[i] == NULL && pkeys[i] != NULL) {
+               if (pkeys[i] != NULL) {
                        EVP_PKEY_free(pkeys[i]);
                }
                if (eks[i]) {



I think that this is wrong, why we keep key_resources table then?
 [2016-02-05 15:58 UTC] bukka@php.net
Thinking about it a bit more, I actually think that the key_resources and key_resource in other functions are mostly useless. It doesn't make much sense to me because having a key (or keys) should be always enough and not sure why we need to allocate resources for each of them if we don't return them. I will need to verify that but if I don't find any reason I will add it on my TODO list and hopefully later clean it up from all places where it's not needed. So I won't commit the above just yet.
 [2016-07-20 11:33 UTC] davey@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=33b1fbbb5c0459a623ab91b492f1a37c5262329c
Log: Fixed bug #71475: openssl_seal() uninitialized memory usage
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Nov 19 01:31:42 2017 UTC