Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7c7d4fd

Browse files
committed
Distrust external OpenSSL clients; clear err queue
OpenSSL has an unfortunate tendency to mix per-session state error handling with per-thread error handling. This can cause problems when programs that link to libpq with OpenSSL enabled have some other use of OpenSSL; without care, one caller of OpenSSL may cause problems for the other caller. Backend code might similarly be affected, for example when a third party extension independently uses OpenSSL without taking the appropriate precautions. To fix, don't trust other users of OpenSSL to clear the per-thread error queue. Instead, clear the entire per-thread queue ahead of certain I/O operations when it appears that there might be trouble (these I/O operations mostly need to call SSL_get_error() to check for success, which relies on the queue being empty). This is slightly aggressive, but it's pretty clear that the other callers have a very dubious claim to ownership of the per-thread queue. Do this is both frontend and backend code. Finally, be more careful about clearing our own error queue, so as to not cause these problems ourself. It's possibly that control previously did not always reach SSLerrmessage(), where ERR_get_error() was supposed to be called to clear the queue's earliest code. Make sure ERR_get_error() is always called, so as to spare other users of OpenSSL the possibility of similar problems caused by libpq (as opposed to problems caused by a third party OpenSSL library like PHP's OpenSSL extension). Again, do this is both frontend and backend code. See bug #12799 and https://bugs.php.net/bug.php?id=68276 Based on patches by Dave Vitek and Peter Eisentraut. From: Peter Geoghegan <pg@bowt.ie>
1 parent 34c33a1 commit 7c7d4fd

File tree

2 files changed

+102
-45
lines changed

2 files changed

+102
-45
lines changed

src/backend/libpq/be-secure-openssl.c

+49-21
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ static DH *tmp_dh_cb(SSL *s, int is_export, int keylength);
7878
static int verify_cb(int, X509_STORE_CTX *);
7979
static void info_cb(const SSL *ssl, int type, int args);
8080
static void initialize_ecdh(void);
81-
static const char *SSLerrmessage(void);
81+
static const char *SSLerrmessage(unsigned long ecode);
8282

8383
static char *X509_NAME_to_cstring(X509_NAME *name);
8484

@@ -182,7 +182,7 @@ be_tls_init(void)
182182
if (!SSL_context)
183183
ereport(FATAL,
184184
(errmsg("could not create SSL context: %s",
185-
SSLerrmessage())));
185+
SSLerrmessage(ERR_get_error()))));
186186

187187
/*
188188
* Disable OpenSSL's moving-write-buffer sanity check, because it
@@ -198,7 +198,7 @@ be_tls_init(void)
198198
ereport(FATAL,
199199
(errcode(ERRCODE_CONFIG_FILE_ERROR),
200200
errmsg("could not load server certificate file \"%s\": %s",
201-
ssl_cert_file, SSLerrmessage())));
201+
ssl_cert_file, SSLerrmessage(ERR_get_error()))));
202202

203203
if (stat(ssl_key_file, &buf) != 0)
204204
ereport(FATAL,
@@ -251,12 +251,12 @@ be_tls_init(void)
251251
SSL_FILETYPE_PEM) != 1)
252252
ereport(FATAL,
253253
(errmsg("could not load private key file \"%s\": %s",
254-
ssl_key_file, SSLerrmessage())));
254+
ssl_key_file, SSLerrmessage(ERR_get_error()))));
255255

256256
if (SSL_CTX_check_private_key(SSL_context) != 1)
257257
ereport(FATAL,
258258
(errmsg("check of private key failed: %s",
259-
SSLerrmessage())));
259+
SSLerrmessage(ERR_get_error()))));
260260
}
261261

262262
/* set up ephemeral DH keys, and disallow SSL v2/v3 while at it */
@@ -285,7 +285,7 @@ be_tls_init(void)
285285
(root_cert_list = SSL_load_client_CA_file(ssl_ca_file)) == NULL)
286286
ereport(FATAL,
287287
(errmsg("could not load root certificate file \"%s\": %s",
288-
ssl_ca_file, SSLerrmessage())));
288+
ssl_ca_file, SSLerrmessage(ERR_get_error()))));
289289
}
290290

291291
/*----------
@@ -316,7 +316,7 @@ be_tls_init(void)
316316
else
317317
ereport(FATAL,
318318
(errmsg("could not load SSL certificate revocation list file \"%s\": %s",
319-
ssl_crl_file, SSLerrmessage())));
319+
ssl_crl_file, SSLerrmessage(ERR_get_error()))));
320320
}
321321
}
322322

@@ -353,6 +353,7 @@ be_tls_open_server(Port *port)
353353
int r;
354354
int err;
355355
int waitfor;
356+
unsigned long ecode;
356357

357358
Assert(!port->ssl);
358359
Assert(!port->peer);
@@ -362,24 +363,44 @@ be_tls_open_server(Port *port)
362363
ereport(COMMERROR,
363364
(errcode(ERRCODE_PROTOCOL_VIOLATION),
364365
errmsg("could not initialize SSL connection: %s",
365-
SSLerrmessage())));
366+
SSLerrmessage(ERR_get_error()))));
366367
return -1;
367368
}
368369
if (!my_SSL_set_fd(port, port->sock))
369370
{
370371
ereport(COMMERROR,
371372
(errcode(ERRCODE_PROTOCOL_VIOLATION),
372373
errmsg("could not set SSL socket: %s",
373-
SSLerrmessage())));
374+
SSLerrmessage(ERR_get_error()))));
374375
return -1;
375376
}
376377
port->ssl_in_use = true;
377378

378379
aloop:
380+
/*
381+
* Prepare to call SSL_get_error() by clearing thread's OpenSSL error
382+
* queue. In general, the current thread's error queue must be empty
383+
* before the TLS/SSL I/O operation is attempted, or SSL_get_error()
384+
* will not work reliably. An extension may have failed to clear the
385+
* per-thread error queue following another call to an OpenSSL I/O
386+
* routine.
387+
*/
388+
ERR_clear_error();
379389
r = SSL_accept(port->ssl);
380390
if (r <= 0)
381391
{
382392
err = SSL_get_error(port->ssl, r);
393+
394+
/*
395+
* Other clients of OpenSSL in the backend may fail to call
396+
* ERR_get_error(), but we always do, so as to not cause problems
397+
* for OpenSSL clients that don't call ERR_clear_error()
398+
* defensively. Be sure that this happens by calling now.
399+
* SSL_get_error() relies on the OpenSSL per-thread error queue
400+
* being intact, so this is the earliest possible point
401+
* ERR_get_error() may be called.
402+
*/
403+
ecode = ERR_get_error();
383404
switch (err)
384405
{
385406
case SSL_ERROR_WANT_READ:
@@ -413,7 +434,7 @@ be_tls_open_server(Port *port)
413434
ereport(COMMERROR,
414435
(errcode(ERRCODE_PROTOCOL_VIOLATION),
415436
errmsg("could not accept SSL connection: %s",
416-
SSLerrmessage())));
437+
SSLerrmessage(ecode))));
417438
break;
418439
case SSL_ERROR_ZERO_RETURN:
419440
ereport(COMMERROR,
@@ -522,10 +543,13 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
522543
{
523544
ssize_t n;
524545
int err;
546+
unsigned long ecode;
525547

526548
errno = 0;
549+
ERR_clear_error();
527550
n = SSL_read(port->ssl, ptr, len);
528551
err = SSL_get_error(port->ssl, n);
552+
ecode = (err != SSL_ERROR_NONE || n < 0) ? ERR_get_error() : 0;
529553
switch (err)
530554
{
531555
case SSL_ERROR_NONE:
@@ -552,7 +576,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
552576
case SSL_ERROR_SSL:
553577
ereport(COMMERROR,
554578
(errcode(ERRCODE_PROTOCOL_VIOLATION),
555-
errmsg("SSL error: %s", SSLerrmessage())));
579+
errmsg("SSL error: %s", SSLerrmessage(ecode))));
556580
/* fall through */
557581
case SSL_ERROR_ZERO_RETURN:
558582
errno = ECONNRESET;
@@ -579,10 +603,13 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
579603
{
580604
ssize_t n;
581605
int err;
606+
unsigned long ecode;
582607

583608
errno = 0;
609+
ERR_clear_error();
584610
n = SSL_write(port->ssl, ptr, len);
585611
err = SSL_get_error(port->ssl, n);
612+
ecode = (err != SSL_ERROR_NONE || n < 0) ? ERR_get_error() : 0;
586613
switch (err)
587614
{
588615
case SSL_ERROR_NONE:
@@ -609,7 +636,7 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
609636
case SSL_ERROR_SSL:
610637
ereport(COMMERROR,
611638
(errcode(ERRCODE_PROTOCOL_VIOLATION),
612-
errmsg("SSL error: %s", SSLerrmessage())));
639+
errmsg("SSL error: %s", SSLerrmessage(ecode))));
613640
/* fall through */
614641
case SSL_ERROR_ZERO_RETURN:
615642
errno = ECONNRESET;
@@ -765,7 +792,8 @@ load_dh_file(int keylength)
765792
{
766793
if (DH_check(dh, &codes) == 0)
767794
{
768-
elog(LOG, "DH_check error (%s): %s", fnbuf, SSLerrmessage());
795+
elog(LOG, "DH_check error (%s): %s", fnbuf,
796+
SSLerrmessage(ERR_get_error()));
769797
return NULL;
770798
}
771799
if (codes & DH_CHECK_P_NOT_PRIME)
@@ -805,7 +833,7 @@ load_dh_buffer(const char *buffer, size_t len)
805833
if (dh == NULL)
806834
ereport(DEBUG2,
807835
(errmsg_internal("DH load buffer: %s",
808-
SSLerrmessage())));
836+
SSLerrmessage(ERR_get_error()))));
809837
BIO_free(bio);
810838

811839
return dh;
@@ -971,26 +999,26 @@ initialize_ecdh(void)
971999
}
9721000

9731001
/*
974-
* Obtain reason string for last SSL error
1002+
* Obtain reason string for passed SSL errcode
1003+
*
1004+
* ERR_get_error() is used by caller to get errcode to pass here.
9751005
*
9761006
* Some caution is needed here since ERR_reason_error_string will
9771007
* return NULL if it doesn't recognize the error code. We don't
9781008
* want to return NULL ever.
9791009
*/
9801010
static const char *
981-
SSLerrmessage(void)
1011+
SSLerrmessage(unsigned long ecode)
9821012
{
983-
unsigned long errcode;
9841013
const char *errreason;
9851014
static char errbuf[32];
9861015

987-
errcode = ERR_get_error();
988-
if (errcode == 0)
1016+
if (ecode == 0)
9891017
return _("no SSL error reported");
990-
errreason = ERR_reason_error_string(errcode);
1018+
errreason = ERR_reason_error_string(ecode);
9911019
if (errreason != NULL)
9921020
return errreason;
993-
snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), errcode);
1021+
snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), ecode);
9941022
return errbuf;
9951023
}
9961024

0 commit comments

Comments
 (0)