Bug #68276 [Ana->Csd]: Reproducible memory corruption: pgsql conflicts with openssl extension

Edit report at https://bugs.php.net/bug.php?id=68276&edit=1 ID: 68276 Updated by: bukka@php.net Reported by: dmitry dot koterov at gmail dot com Summary: Reproducible memory corruption: pgsql conflicts with openssl extension -Status: Analyzed +Status: Closed Type: Bug Package: OpenSSL related Operating System: Ubuntu 14 PHP Version: 5.5.18 -Assigned To: +Assigned To: bukka Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [2016-02-12 23:53:32] 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... ------------------------------------------------------------------------ [2015-04-02 01:08:02] 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. ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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