Bug #68276 [Com]: Reproducible memory corruption: pgsql conflicts with openssl extension

Edit report at https://bugs.php.net/bug.php?id=68276&edit=1 ID: 68276 Comment by: william dot welter at 4linux dot com dot br Reported by: dmitry dot koterov at gmail dot com Summary: Reproducible memory corruption: pgsql conflicts with openssl extension Status: Assigned Type: Bug Package: PostgreSQL related Operating System: Ubuntu 14 PHP Version: 5.5.18 Assigned To: yohgaki Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [2015-03-06 17:37:37] 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-02-24 04:41:56] yohgaki@php.net Thank you William. I'm not following pgsql-hackers list closely. Will the patch be merged to PostgreSQL anytime soon? ------------------------------------------------------------------------ [2015-02-24 04:39:06] yohgaki@php.net Assign myself to remind me. ------------------------------------------------------------------------ [2015-02-24 03:18:54] 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-22 04:35:36] 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 ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=68276 -- Edit this bug report at https://bugs.php.net/bug.php?id=68276&edit=1