8000 Distrust external OpenSSL clients; clear err queue · dpirotte/postgres@9b676fd · GitHub
[go: up one dir, main page]
Skip to content

Commit 9b676fd

Browse files
peterepetere
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 7bad282 commit 9b676fd

File tree

2 files changed

+103
-44
lines changed

2 files changed

+103
-44
lines changed

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

Lines changed: 49 additions & 20 deletions
< 6D4E td data-grid-cell-id="diff-0202c94a7fdd10c35397465259ce2c5c03c64f6d55047a485486939a25b12d88-752-759-1" data-selected="false" role="gridcell" style="background-color:var(--bgColor-default);text-align:center" tabindex="-1" valign="top" class="focusable-grid-cell diff-line-number position-relative diff-line-number-neutral left-side">759
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ static void info_cb(const SSL *ssl, int type, int args);
9191
static void initialize_SSL(void);
9292
static int open_server_SSL(Port *);
9393
static void close_SSL(Port *);
94-
static const char *SSLerrmessage(void);
94+
static const char *SSLerrmessage(unsigned long ecode);
9595
#endif
9696

9797
/*
@@ -245,11 +245,14 @@ secure_read(Port *port, void *ptr, size_t len)
245245
if (port->ssl)
246246
{
247247
int err;
248+
unsigned long ecode;
248249

249250
rloop:
250251
errno = 0;
252+
ERR_clear_error();
251253
n = SSL_read(port->ssl, ptr, len);
252254
err = SSL_get_error(port->ssl, n);
255+
ecode = (err != SSL_ERROR_NONE || n < 0) ? ERR_get_error() : 0;
253256
switch (err)
254257
{
255258
case SSL_ERROR_NONE:
@@ -281,7 +284,7 @@ secure_read(Port *port, void *ptr, size_t len)
281284
case SSL_ERROR_SSL:
282285
ereport(COMMERROR,
283286
(errcode(ERRCODE_PROTOCOL_VIOLATION),
284-
errmsg("SSL error: %s", SSLerrmessage())));
287+
errmsg("SSL error: %s", SSLerrmessage(ecode))));
285288
/* fall through */
286289
case SSL_ERROR_ZERO_RETURN:
287290
errno = ECONNRESET;
@@ -321,6 +324,7 @@ secure_write(Port *port, void *ptr, size_t len)
321324
if (port->ssl)
322325
{
323326
int err;
327+
unsigned long ecode;
324328

325329
if (ssl_renegotiation_limit && port->count > ssl_renegotiation_limit * 1024L)
326330
{
@@ -349,8 +353,10 @@ secure_write(Port *port, void *ptr, size_t len)
349353

350354
wloop:
351355
errno = 0;
356+
ERR_clear_error();
352357
n = SSL_write(port->ssl, ptr, len);
353358
err = SSL_get_error(port->ssl, n);
359+
ecode = (err != SSL_ERROR_NONE || n < 0) ? ERR_get_error() : 0;
354360
switch (err)
355361
{
356362
case SSL_ERROR_NONE:
@@ -376,7 +382,7 @@ secure_write(Port *port, void *ptr, size_t len)
376382
case SSL_ERROR_SSL:
377383
ereport(COMMERROR,
378384
(errcode(ERRCODE_PROTOCOL_VIOLATION),
379-
errmsg("SSL error: %s", SSLerrmessage())));
385+
errmsg("SSL error: %s", SSLerrmessage(ecode))));
380386
/* fall through */
381387
case SSL_ERROR_ZERO_RETURN:
382388
errno = ECONNRESET;
@@ -536,7 +542,8 @@ load_dh_file(int keylength)
536542
{
537543
if (DH_check(dh, &codes) == 0)
538544
{
539-
elog(LOG, "DH_check error (%s): %s", fnbuf, SSLerrmessage());
545+
elog(LOG, "DH_check error (%s): %s", fnbuf,
546+
SSLerrmessage(ERR_get_error()));
540547
return NULL;
541548
}
542549
if (codes & DH_CHECK_P_NOT_PRIME)
@@ -576,7 +583,7 @@ load_dh_buffer(const char *buffer, size_t len)
576583
if (dh == NULL)
577584
ereport(DEBUG2,
578585
(errmsg_internal("DH load buffer: %s",
579-
SSLerrmessage())));
586+
SSLerrmessage(ERR_get_error()))));
580587
BIO_free(bio);
581588

582589
return dh;
@@ -746,7 +753,7 @@ initialize_SSL(void)
746753
if (!SSL_context)
747754
ereport(FATAL,
748755
(errmsg("could not create SSL context: %s",
749-
SSLerrmessage())));
756+
SSLerrmessage(ERR_get_error()))));
750757

751758
/*
752
* Disable OpenSSL's moving-write-buffer sanity check, because it
@@ -762,7 +769,7 @@ initialize_SSL(void)
762769
ereport(FATAL,
763770
(errcode(ERRCODE_CONFIG_FILE_ERROR),
764771
errmsg("could not load server certificate file \"%s\": %s",
765-
SERVER_CERT_FILE, SSLerrmessage())));
772+
SERVER_CERT_FILE, SSLerrmessage(ERR_get_error()))));
766773

767774
if (stat(SERVER_PRIVATE_KEY_FILE, &buf) != 0)
768775
ereport(FATAL,
@@ -792,12 +799,13 @@ initialize_SSL(void)
792799
SSL_FILETYPE_PEM) != 1)
793800
ereport(FATAL,
794801
(errmsg("could not load private key file \"%s\": %s",
795-
SERVER_PRIVATE_KEY_FILE, SSLerrmessage())));
802+
SERVER_PRIVATE_KEY_FILE,
803+
SSLerrmessage(ERR_get_error()))));
796804

797805
if (SSL_CTX_check_private_key(SSL_context) != 1)
798806
ereport(FATAL,
799807
(errmsg("check of private key failed: %s",
800-
SSLerrmessage())));
808+
SSLerrmessage(ERR_get_error()))));
801809
}
802810

803811
/* set up ephemeral DH keys, and disallow SSL v2 while at it */
@@ -836,7 +844,7 @@ initialize_SSL(void)
836844
*/
837845
ereport(FATAL,
838846
(errmsg("could not load root certificate file \"%s\": %s",
839-
ROOT_CERT_FILE, SSLerrmessage())));
847+
ROOT_CERT_FILE, SSLerrmessage(ERR_get_error()))));
840848
}
841849
else
842850
{
@@ -868,7 +876,7 @@ initialize_SSL(void)
868876
/* Not fatal - we do not require CRL */
869877
ereport(LOG,
870878
(errmsg("SSL certificate revocation list file \"%s\" not found, skipping: %s",
871-
ROOT_CRL_FILE, SSLerrmessage()),
879+
ROOT_CRL_FILE, SSLerrmessage(ERR_get_error())),
872880
errdetail("Certificates will not be checked against revocation list.")));
873881
}
874882

@@ -903,6 +911,7 @@ open_server_SSL(Port *port)
903911
{
904912
int r;
905913
int err;
914+
unsigned long ecode;
906915

907916
Assert(!port->ssl);
908917
Assert(!port->peer);
@@ -912,23 +921,43 @@ open_server_SSL(Port *port)
912921
ereport(COMMERROR,
913922
(errcode(ERRCODE_PROTOCOL_VIOLATION),
914923
errmsg("could not initialize SSL connection: %s",
915-
SSLerrmessage())));
924+
SSLerrmessage(ERR_get_error()))));
916925
return -1;
917926
}
918927
if (!my_SSL_set_fd(port->ssl, port->sock))
919928
{
920929
ereport(COMMERROR,
921930
(errcode(ERRCODE_PROTOCOL_VIOLATION),
922931
errmsg("could not set SSL socket: %s",
923-
SSLerrmessage())));
932+
SSLerrmessage(ERR_get_error()))));
924933
return -1;
925934
}
926935

927936
aloop:
937+
/*
938+
* Prepare to call SSL_get_error() by clearing thread's OpenSSL error
939+
* queue. In general, the current thread's error queue must be empty
940+
* before the TLS/SSL I/O operation is attempted, or SSL_get_error()
941+
* will not work reliably. An extension may have failed to clear the
942+
* per-thread error queue following another call to an OpenSSL I/O
943+
* routine.
944+
*/
945+
ERR_clear_error();
928946
r = SSL_accept(port->ssl);
929947
if (r <= 0)
930948
{
931949
err = SSL_get_error(port->ssl, r);
950+
951+
/*
952+
* Other clients of OpenSSL in the backend may fail to call
953+
* ERR_get_error(), but we always do, so as to not cause problems
954+
* for OpenSSL clients that don't call ERR_clear_error()
955+
* defensively. Be sure that this happens by calling now.
956+
* SSL_get_error() relies on the OpenSSL per-thread error queue
957+
* being intact, so this is the earliest possible point
958+
* ERR_get_error() may be called.
959+
*/
960+
ecode = ERR_get_error();
932961
switch (err)
933962
{
934963
case SSL_ERROR_WANT_READ:
@@ -954,7 +983,7 @@ open_server_SSL(Port *port)
954983
ereport(COMMERROR,
955984
(errcode(ERRCODE_PROTOCOL_VIOLATION),
956985
errmsg("could not accept SSL connection: %s",
957-
SSLerrmessage())));
986+
SSLerrmessage(ecode))));
958987
break;
959988
case SSL_ERROR_ZERO_RETURN:
960989
ereport(COMMERROR,
@@ -1055,24 +1084,24 @@ close_SSL(Port *port)
10551084
/*
10561085
* Obtain reason string for last SSL error
10571086
*
1087+
* ERR_get_error() is used by caller to get errcode to pass here.
1088+
*
10581089
* Some caution is needed here since ERR_reason_error_string will
10591090
* return NULL if it doesn't recognize the error code. We don't
10601091
* want to return NULL ever.
10611092
*/
10621093
static const char *
1063-
SSLerrmessage(void)
1094+
SSLerrmessage(unsigned long ecode)
10641095
{
1065-
unsigned long errcode;
10661096
const char *errreason;
10671097
static char errbuf[32];
10681098

1069-
errcode = ERR_get_error();
1070-
if (errcode == 0)
1099+
if (ecode == 0)
10711100
return _("no SSL error reported");
1072-
errreason = ERR_reason_error_string(errcode);
1101+
errreason = ERR_reason_error_string(ecode);
10731102
if (errreason != NULL)
10741103
return errreason;
1075-
snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), errcode);
1104+
snprintf(errbuf, sizeof(errbuf), _("SSL error code %lu"), ecode);
10761105
return errbuf;
10771106
}
10781107

0 commit comments

Comments
 (0)
0