php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #68276 Reproducible memory corruption: pgsql conflicts with openssl extension
Submitted: 2014-10-21 14:38 UTC Modified: 2016-06-19 17:09 UTC
Votes:15
Avg. Score:4.6 ± 0.6
Reproduced:13 of 13 (100.0%)
Same Version:6 (46.2%)
Same OS:10 (76.9%)
From: dmitry dot koterov at gmail dot com Assigned: bukka (profile)
Status: Closed Package: OpenSSL related
PHP Version: 5.5.18 OS: Ubuntu 14
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: dmitry dot koterov at gmail dot com
New email:
PHP Version: OS:

 

 [2014-10-21 14:38 UTC] dmitry dot koterov at gmail dot com
Description:
------------
Long story short: pgsql extension does not call libpg's PQinitSSL nor PQinitOpenSSL inside itself, though it is REQUIRED by libpq if OpenSSL is used elsewhere (e.g. in "openssl" PHP extension).

When openssl & pgsql extensions are used in the same script, php process sometimes crashes with a coredump (rarely reproducible). The core file shows an access violation in pgsql extension (at "efree(notice->message);" line), but the bug is caused NOT by this code, it is caused by a deep memory corruption which influences the above code (one of such influences).

I've found a 100% reproducible case for this bug, but with a bit different result. See the test script below: if you run it in a x86_64 machine, PHP 5.3 or 5.4+, PostgreSQL 9.1 or 9.3+ (at least), you see a strange error message.

If you play with the test buffer size (decrease it from 160K to e.g. 10K), the problem disappears. If no openssl extension is used (just remove the call to "openssl_pkey_get_public"), the problem disappears as well.

Why do I suspect that the problem is in the pgsql extension code? Because pgsql extension does not call PQinitSSL nor PQinitOpenSSL from libpg, but libpq requires these calls if it is used in an environment which already uses OpenSSL library: according to http://www.postgresql.org/docs/9.1/static/libpq-ssl.html "If your application initializes libssl and/or libcrypto libraries and libpq is built with SSL support, you should call PQinitOpenSSL to tell libpq that the libssl and/or libcrypto libraries have been initialized by your application, so that libpq will not also initialize those libraries."

P.S.
Here is the same-looking note from 2009: http://linux.m2osw.com/postgresql_openssl_conflict


Test script:
---------------
<?php                                                                                                               
// su - postgres -c psql                                                                                            
// # create database test;                                                                                          
// # create role test with password 'test' login;
// Also be sure that ssl=true is set in postgresql.conf!                                                                   
$sql = "SELECT repeat('a', 160000)";                                                                                
$c = pg_connect("host='127.0.0.1' dbname='test' user='test' password='test' sslmode='require'");
$r = openssl_pkey_get_public('a');                                                                                  
if ($r = pg_query($sql)) echo "OK\n"; else echo pg_last_error($c);                                                  

Expected result:
----------------
OK

Actual result:
--------------
PHP Warning:  pg_query(): Query failed: SSL error: no start line in /root/a.php on line 8

Sometimes (very-very rare) it crashes the php5-fpm process, see a backtrace for e.g. PHP 5.3 below (I did not test the crash itself at PHP 5.4+, but I tested the "SSL error: no start line" in all PHP versions, and the roots of these two behaviors are same). Note that there is nothing wrong with notice->message, it's a memory corruption happened long time before, and seems this memory corruption spreads over multiple requests to the same php-fpm process, because the crash is reproduced more frequently when I run the test script above than when I do not run it at all.

Program terminated with signal 11, Segmentation fault.
#0  0x00000000006ba1e3 in _zend_mm_free_canary_int (heap=0x2519320, p=0x73d7f11b0c14b99c) at /build/buildd/php5-5.3.10/Zend/zend_alloc_canary.c:2086
2086	/build/buildd/php5-5.3.10/Zend/zend_alloc_canary.c: No such file or directory.
(gdb) bt
#0  0x00000000006ba1e3 in _zend_mm_free_canary_int (heap=0x2519320, p=0x73d7f11b0c14b99c) at /build/buildd/php5-5.3.10/Zend/zend_alloc_canary.c:2086
#1  0x00007f8e033a5231 in _php_pgsql_notice_ptr_dtor (ptr=0x2519320) at /build/buildd/php5-5.3.10/ext/pgsql/pgsql.c:835
#2  0x00000000006a7ed0 in zend_hash_clean (ht=0x7f8e035bb168) at /build/buildd/php5-5.3.10/Zend/zend_hash.c:761
#3  0x00007f8e033a9dc0 in zm_deactivate_pgsql (type=38900512, module_number=202684828) at /build/buildd/php5-5.3.10/ext/pgsql/pgsql.c:1034
#4  0x00000000006a132c in module_registry_cleanup (module=0x2519320) at /build/buildd/php5-5.3.10/Zend/zend_API.c:2168
#5  0x00000000006a82ac in zend_hash_reverse_apply (ht=0xde83e0, apply_func=0x6a1310 <module_registry_cleanup>) at /build/buildd/php5-5.3.10/Zend/zend_hash.c:959
#6  0x000000000069a0f0 in zend_deactivate_modules () at /build/buildd/php5-5.3.10/Zend/zend.c:939
#7  0x0000000000647105 in php_request_shutdown (dummy=0x2519320) at /build/buildd/php5-5.3.10/main/main.c:1638
#8  0x000000000042b71c in main (argc=41420936, argv=0x2780628) at /build/buildd/php5-5.3.10/sapi/fpm/fpm/fpm_main.c:1913
(gdb) up
#1  0x00007f8e033a5231 in _php_pgsql_notice_ptr_dtor (ptr=0x2519320) at /build/buildd/php5-5.3.10/ext/pgsql/pgsql.c:835
835	 		efree(notice->message);

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-02-22 04:35 UTC] william dot welter at 4linux dot com dot br
I can reproduce de problem.. 

I do some debugging and understand the real problem:
1- Firstly the use of PQinitSSL nor PQinitOpenSSL not solve the problem, im already test it.
2 - Calling openssl_pkey_get_public('a') PHP try to evaluate the string as X509 certificate with libcrypto and this throws error.
( see stack: 
ERR_put_error() at err.c:726 0x7ffff6f206f4	
PEM_read_bio() at pem_lib.c:689 0x7ffff6f58586	
PEM_bytes_read_bio() at pem_lib.c:281 0x7ffff6f5706f	
PEM_ASN1_read_bio() at pem_oth.c:78 0x7ffff6f5a9e6	
php_openssl_x509_from_zval() at openssl.c:1.283 0x468700	
php_openssl_x509_from_zval() at openssl.c:1.226 0x468700	
php_openssl_evp_from_zval() at openssl.c:2.864 0x469cd4	
zif_openssl_pkey_get_public() at openssl.c:3.310 0x46bc2b	
)
3 - The problem is that libcryto/openssl share the same error struct on the entire thread, then libqp will use the same struct and will be aware of the errors:
see crypto/err/err.c
ERR_STATE *ERR_get_state(void)
{
    static ERR_STATE fallback;
    ERR_STATE *ret, tmp, *tmpp = NULL;
    int i;
    CRYPTO_THREADID tid;

    err_fns_check();
    CRYPTO_THREADID_current(&tid);
    CRYPTO_THREADID_cpy(&tmp.tid, &tid);
    ret = ERRFN(thread_get_item) (&tmp);

    /* ret == the error state, if NULL, make a new one */
    if (ret == NULL) {
        ret = (ERR_STATE *)OPENSSL_malloc(sizeof(ERR_STATE));
        if (ret == NULL)
            return (&fallback);
        CRYPTO_THREADID_cpy(&ret->tid, &tid);
        ret->top = 0;
        ret->bottom = 0;
        for (i = 0; i < ERR_NUM_ERRORS; i++) {
            ret->err_data[i] = NULL;
            ret->err_data_flags[i] = 0;
        }
        tmpp = ERRFN(thread_set_item) (ret);
        /* To check if insertion failed, do a get. */
        if (ERRFN(thread_get_item) (ret) != ret) {
            ERR_STATE_free(ret); /* could not insert it */
            return (&fallback);
        }
        /*
         * If a race occured in this function and we came second, tmpp is the
         * first one that we just replaced.
         */
        if (tmpp)
            ERR_STATE_free(tmpp);
    }
    return ret;
}
4 - This error only appear with 160K because with this size the multiplier with the buffersize over-read the length of data available and this make the return of "SSL_read()" be "-1" which triggers "SSL_get_error" function to return error. On normal conditions "SSL_get_error" should return called SSL_ERROR_WANT_READ that is normal and libpq can handle this. But in this case (with previous errors stored on the error struct) "ERR_peek_error" function called in the  "SSL_get_error" will get the first error occurred (no start line) which leads to return SSL_ERROR_SSL that is handle as a fatal error by libpq.

fe-secure.c	
pqsecure_read(PGconn *conn, void *ptr, size_t len)
{
	ssize_t		n;
	int			result_errno = 0;
	char		sebuf[256];

#ifdef USE_SSL
	if (conn->ssl)
	{
		int			err;

		DECLARE_SIGPIPE_INFO(spinfo);

		/* SSL_read can write to the socket, so we need to disable SIGPIPE */
		DISABLE_SIGPIPE(conn, spinfo, return -1);

rloop:
		SOCK_ERRNO_SET(0);
		n = SSL_read(conn->ssl, ptr, len);
		err = SSL_get_error(conn->ssl, n);
		switch (err)
		{
			case SSL_ERROR_NONE:
				if (n < 0)
				{
					/* Not supposed to happen, so we don't translate the msg */
					printfPQExpBuffer(&conn->errorMessage,
									  "SSL_read failed but did not provide error information\n");
					/* assume the connection is broken */
					result_errno = ECONNRESET;
				}
				break;
			case SSL_ERROR_WANT_READ:
				n = 0;
				break;
			case SSL_ERROR_WANT_WRITE:

				/*
				 * Returning 0 here would cause caller to wait for read-ready,
				 * which is not correct since what SSL wants is wait for
				 * write-ready.  The former could get us stuck in an infinite
				 * wait, so don't risk it; busy-loop instead.
				 */
				goto rloop;
			case SSL_ERROR_SYSCALL:
				if (n < 0)
				{
					result_errno = SOCK_ERRNO;
					REMEMBER_EPIPE(spinfo, result_errno == EPIPE);
					if (result_errno == EPIPE ||
						result_errno == ECONNRESET)
						printfPQExpBuffer(&conn->errorMessage,
										  libpq_gettext(
								"server closed the connection unexpectedly\n"
														"\tThis probably means the server terminated abnormally\n"
							 "\tbefore or while processing the request.\n"));
					else
						printfPQExpBuffer(&conn->errorMessage,
									libpq_gettext("SSL SYSCALL error: %s\n"),
										  SOCK_STRERROR(result_errno,
													  sebuf, sizeof(sebuf)));
				}
				else
				{
					printfPQExpBuffer(&conn->errorMessage,
						 libpq_gettext("SSL SYSCALL error: EOF detected\n"));
					/* assume the connection is broken */
					result_errno = ECONNRESET;
					n = -1;
				}
				break;
			case SSL_ERROR_SSL:
				{
					char	   *errm = SSLerrmessage();

					printfPQExpBuffer(&conn->errorMessage,
									  libpq_gettext("SSL error: %s\n"), errm);
					SSLerrfree(errm);
					/* assume the connection is broken */
					result_errno = ECONNRESET;
					n = -1;
					break;
				}
			case SSL_ERROR_ZERO_RETURN:

				/*
				 * Per OpenSSL documentation, this error code is only returned
				 * for a clean connection closure, so we should not report it
				 * as a server crash.
				 */
				printfPQExpBuffer(&conn->errorMessage,
								  libpq_gettext("SSL connection has been closed unexpectedly\n"));
				result_errno = ECONNRESET;
				n = -1;
				break;
			default:
				printfPQExpBuffer(&conn->errorMessage,
						  libpq_gettext("unrecognized SSL error code: %d\n"),
								  err);
				/* assume the connection is broken */
				result_errno = ECONNRESET;
				n = -1;
				break;
		}

		RESTORE_SIGPIPE(conn, spinfo);
	}
	else
#endif   /* USE_SSL */
	{
		n = recv(conn->sock, ptr, len, 0);

		if (n < 0)
		{
			result_errno = SOCK_ERRNO;

			/* Set error message if appropriate */
			switch (result_errno)
			{
#ifdef EAGAIN
				case EAGAIN:
#endif
#if defined(EWOULDBLOCK) && (!defined(EAGAIN) || (EWOULDBLOCK != EAGAIN))
				case EWOULDBLOCK:
#endif
				case EINTR:
					/* no error message, caller is expected to retry */
					break;

#ifdef ECONNRESET
				case ECONNRESET:
					printfPQExpBuffer(&conn->errorMessage,
									  libpq_gettext(
								"server closed the connection unexpectedly\n"
					"\tThis probably means the server terminated abnormally\n"
							 "\tbefore or while processing the request.\n"));
					break;
#endif

				default:
					printfPQExpBuffer(&conn->errorMessage,
					libpq_gettext("could not receive data from server: %s\n"),
									  SOCK_STRERROR(result_errno,
													sebuf, sizeof(sebuf)));
					break;
			}
		}
	}

	/* ensure we return the intended errno to caller */
	SOCK_ERRNO_SET(result_errno);

	return n;
}

ssl/ssl_lib.c	
int SSL_get_error(const SSL *s, int i)
{
    int reason;
    unsigned long l;
    BIO *bio;

    if (i > 0)
        return (SSL_ERROR_NONE);

    /*
     * Make things return SSL_ERROR_SYSCALL when doing SSL_do_handshake etc,
     * where we do encode the error
     */
    if ((l = ERR_peek_error()) != 0) {
        if (ERR_GET_LIB(l) == ERR_LIB_SYS)
            return (SSL_ERROR_SYSCALL);
        else
            return (SSL_ERROR_SSL);
    }

    if ((i < 0) && SSL_want_read(s)) {
        bio = SSL_get_rbio(s);
        if (BIO_should_read(bio))
            return (SSL_ERROR_WANT_READ);	
...


Another way to see that the shared error struct is the problem:
 <?php                                                                                                               
// su - postgres -c psql                                                                                            
// # create database test;                                                                                          
// # create role test with password 'test' login;
// Also be sure that ssl=true is set in postgresql.conf!                                                                   
$sql = "SELECT repeat('a', 160000)";                                                                                
$c = pg_connect("host='127.0.0.1' dbname='test' user='test' password='test' sslmode='require'");
$r = openssl_pkey_get_public('a');

while ($msg = openssl_error_string())
    echo $msg . "<br />\n";
                                                                                  
if ($r = pg_query($sql)) echo "OK\n"; else echo pg_last_error($c);   


Im trying to solve this problem, possible solutions:
1 - Find a way to not share the error struct with the entire thread on PHP
2 - Clear the error struct before send querys  on libpq
3 - function ERR_peek_error() get the last error instead of the first error on OpenSSL
 [2015-02-24 03:18 UTC] william dot welter at 4linux dot com dot br
On my opinion the bug is not on PHP, is on libpq pgsecure_read()/pqsecure_write() functions that not clean error queue before IO operations as described on OpenSSL manual (https://www.openssl.org/docs/ssl/SSL_get_error.html#DESCRIPTION).

Im  already open a bug on PostgreSQL community with patch suggested (http://www.postgresql.org/message-id/20150224030956.2529.83279@wrigleys.postgresql.org)
 [2015-02-24 04:39 UTC] yohgaki@php.net
-Assigned To: +Assigned To: yohgaki
 [2015-02-24 04:39 UTC] yohgaki@php.net
Assign myself to remind me.
 [2015-02-24 04:41 UTC] yohgaki@php.net
Thank you William.
I'm not following pgsql-hackers list closely.
Will the patch be merged to PostgreSQL anytime soon?
 [2015-03-06 17:37 UTC] william dot welter at 4linux dot com dot br
The bug that i open on the libpq revealed that libpq has an internal control to not leave error on queue always "poping" the last element if an error occur after each call.

The problem is that PHP leaves error on the queue if the developer don't call openssl_error_string() for each ssl call. This behaviour makes a landmine for every lib that use OpenSSL that not clean the error queue before IO calls.

I propose a patch to clean the error queue on libpq before the IO operations, but this can't be applied because on current versions because this can break currently applications. I will propose a patch for next major release.


On my opinion PHP should not leave errors on error queue, because this can break other libs that use OpenSSL. I will work on patch to create an internal struct to store the errors to keep the OpenSSL error queue clean to other libs.
 [2015-04-02 01:08 UTC] william dot welter at 4linux dot com dot br
The following patch is my proposal to solve the problem on PHP side, by not left a landmine on openssl error queue for other applications, without broke currently "openssl_error_string" behaviour. 

This consist of create an internal ssl error context on PHP, and clean de original ssl error queue after store on the internal queue.

diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
index c64f1e92..787d513 100644
--- a/ext/openssl/openssl.c
+++ b/ext/openssl/openssl.c
@@ -498,6 +498,11 @@ static int le_x509;
 static int le_csr;
 static int ssl_stream_data_index;
 
+
+
+
+
+
 int php_openssl_get_x509_list_id(void) /* {{{ */
 {
 	return le_x509;
@@ -808,6 +813,52 @@ static int add_oid_section(struct php_x509_request * req TSRMLS_DC) /* {{{ */
 static const EVP_CIPHER * php_openssl_get_evp_cipher_from_algo(long algo);
 
 
+
+
+
+static PHP_SSL_ERROR_CONTEXT *ssl_error_context;
+
+void php_openssl_store_errors(void)
+{
+		PHP_SSL_ERROR_QUEUE *err,*tmp;
+		char buf[512];
+		unsigned long val;
+
+		//initialize error context if is null;
+		if(ssl_error_context==NULL) {
+			ssl_error_context = emalloc(sizeof(PHP_SSL_ERROR_CONTEXT));
+			ssl_error_context->current=NULL;
+		}
+
+		err = emalloc(sizeof(PHP_SSL_ERROR_QUEUE));
+		err->next=NULL;
+
+		// Retrieve error from openssl error queue
+		val = ERR_get_error();
+		if (val) {
+			ERR_error_string(val, buf);
+			err->err_str=emalloc(strlen(buf));
+			strcpy(err->err_str,buf);
+			ERR_clear_error();
+		}else{
+			return ;
+		}
+
+		//If error queue is empty, create the first
+		if(ssl_error_context->current==NULL){
+			ssl_error_context->current=err;
+		}else{
+			//Else, append error to last element
+			tmp=ssl_error_context->current;
+			while(tmp->next!=NULL)
+			{
+				tmp=tmp->next;
+			}
+			tmp->next=err;
+		}
+
+}
+
 static int php_openssl_parse_config(struct php_x509_request * req, zval * optional_args TSRMLS_DC) /* {{{ */
 {
 	char * str;
@@ -3323,6 +3374,7 @@ PHP_FUNCTION(openssl_pkey_get_public)
 	pkey = php_openssl_evp_from_zval(cert, 1, NULL, 1, &Z_LVAL_P(return_value) TSRMLS_CC);
 
 	if (pkey == NULL) {
+		php_openssl_store_errors();
 		RETURN_FALSE;
 	}
 	zend_list_addref(Z_LVAL_P(return_value));
@@ -4204,19 +4256,36 @@ PHP_FUNCTION(openssl_public_decrypt)
    Returns a description of the last error, and alters the index of the error messages. Returns false when the are no more messages */
 PHP_FUNCTION(openssl_error_string)
 {
-	char buf[512];
-	unsigned long val;
+	PHP_SSL_ERROR_QUEUE *tmp;
+	char *err_str;
 
 	if (zend_parse_parameters_none() == FAILURE) {
-		return;
+	    return;
 	}
 
-	val = ERR_get_error();
-	if (val) {
-		RETURN_STRING(ERR_error_string(val, buf), 1);
-	} else {
-		RETURN_FALSE;
+	// Return false if error queue is empty
+	if(ssl_error_context->current==NULL){
+		RETURN_FALSE;;
 	}
+
+	// Get first error
+	tmp=ssl_error_context->current;
+
+	//Store error msg
+	err_str=emalloc(strlen(tmp->err_str));
+	strcpy(err_str,tmp->err_str);
+
+	//Update references of the error queue
+	if(tmp->next!=NULL){
+		ssl_error_context->current=tmp->next;
+	}else{
+		ssl_error_context->current=NULL;
+	}
+
+	//Free memory of the current error
+	efree(tmp);
+	RETURN_STRING(err_str,1);
+
 }
 /* }}} */
 
diff --git a/ext/openssl/php_openssl.h b/ext/openssl/php_openssl.h
index 86d83b7..a3d9b67 100644
--- a/ext/openssl/php_openssl.h
+++ b/ext/openssl/php_openssl.h
@@ -86,6 +86,15 @@ PHP_FUNCTION(openssl_csr_get_public_key);
 #endif
 
 #endif
+typedef struct php_ssl_error{
+	 char *err_str;
+	 struct php_ssl_error* next;
+}PHP_SSL_ERROR_QUEUE ;
+
+
+typedef struct php_ssl_error_context{
+	struct php_ssl_error* current;
+}PHP_SSL_ERROR_CONTEXT;
 
 /*
  * Local variables:


Of course, i need to call php_openssl_store_errors on every "false" return of every function on the openssl extension.

I will put this as pull request on github.
 [2016-01-19 02:22 UTC] yohgaki@php.net
-Status: Assigned +Status: Analyzed -Package: PostgreSQL related +Package: OpenSSL related -Assigned To: yohgaki +Assigned To:
 [2016-02-12 23:53 UTC] dz at heroku dot com
We created a fix for Postgres: http://www.postgresql.org/message-id/flat/CAM3SWZSOJ1p-6jE+h8iii6WboBmyFHuJto=S2Fk==y1wLV3pSQ@mail.gmail.com

Latest point releases were two days ago, and it did not make it in, so hopefully next time...
 [2016-06-19 17:09 UTC] bukka@php.net
-Status: Analyzed +Status: Closed -Assigned To: +Assigned To: bukka
 [2016-06-19 17:09 UTC] bukka@php.net
I'm closing this as the OpenSSL error store has just been merged and there is not much more we can do in openssl ext to prevent this. If it's still issue after 7.1, please re-open.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Mar 28 19:01:29 2024 UTC