forked from msys2/MINGW-packages
-
Notifications
You must be signed in to change notification settings - Fork 48
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
curl: work around
SSL_read: No error problems
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
- Loading branch information
Showing
2 changed files
with
160 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| From 78cef068479d85f7af29b5ddf21ced5288a50f37 Mon Sep 17 00:00:00 2001 | ||
| From: Jay Satiro <raysatiro@yahoo.com> | ||
| Date: Wed, 20 Nov 2019 18:44:18 -0500 | ||
| Subject: [PATCH] openssl: Revert to less sensitivity for SYSCALL errors | ||
|
|
||
| - Disable the extra sensitivity except in debug builds (--enable-debug). | ||
|
|
||
| - Improve SYSCALL error message logic in ossl_send and ossl_recv so that | ||
| "No error" / "Success" socket error text isn't shown on SYSCALL error. | ||
|
|
||
| Prior to this change 0ab38f5 (precedes 7.67.0) increased the sensitivity | ||
| of OpenSSL's SSL_ERROR_SYSCALL error so that abrupt server closures were | ||
| also considered errors. For example, a server that does not send a known | ||
| protocol termination point (eg HTTP content length or chunked encoding) | ||
| _and_ does not send a TLS termination point (close_notify alert) would | ||
| cause an error if it closed the connection. | ||
|
|
||
| To be clear that behavior made it into release build 7.67.0 | ||
| unintentionally. Several users have reported it as an issue. | ||
|
|
||
| Ultimately the idea is a good one, since it can help prevent against a | ||
| truncation attack. Other SSL backends may already behave similarly (such | ||
| as Windows native OS SSL Schannel). However much more of our user base | ||
| is using OpenSSL and there is a mass of legacy users in that space, so I | ||
| think that behavior should be partially reverted and then rolled out | ||
| slowly. | ||
|
|
||
| This commit changes the behavior so that the increased sensitivity is | ||
| disabled in all curl builds except curl debug builds (DEBUGBUILD). If | ||
| after a period of time there are no major issues then it can be enabled | ||
| in dev and release builds with the newest OpenSSL (1.1.1+), since users | ||
| using the newest OpenSSL are the least likely to have legacy problems. | ||
|
|
||
| Bug: https://github.com/curl/curl/issues/4409#issuecomment-555955794 | ||
| Reported-by: Bjoern Franke | ||
|
|
||
| Fixes https://github.com/curl/curl/issues/4624 | ||
| Closes https://github.com/curl/curl/pull/4623 | ||
| --- | ||
| lib/vtls/openssl.c | 72 +++++++++++++++++++++++++++++++++++++--------- | ||
| 1 file changed, 59 insertions(+), 13 deletions(-) | ||
|
|
||
| diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c | ||
| index 6b24c0cf4e..e5bd9d604f 100644 | ||
| --- a/lib/vtls/openssl.c | ||
| +++ b/lib/vtls/openssl.c | ||
| @@ -392,11 +392,20 @@ static const char *SSL_ERROR_to_str(int err) | ||
| */ | ||
| static char *ossl_strerror(unsigned long error, char *buf, size_t size) | ||
| { | ||
| + if(size) | ||
| + *buf = '\0'; | ||
| + | ||
| #ifdef OPENSSL_IS_BORINGSSL | ||
| ERR_error_string_n((uint32_t)error, buf, size); | ||
| #else | ||
| ERR_error_string_n(error, buf, size); | ||
| #endif | ||
| + | ||
| + if(size > 1 && !*buf) { | ||
| + strncpy(buf, (error ? "Unknown error" : "No error"), size); | ||
| + buf[size - 1] = '\0'; | ||
| + } | ||
| + | ||
| return buf; | ||
| } | ||
|
|
||
| @@ -3833,10 +3842,22 @@ static ssize_t ossl_send(struct connectdata *conn, | ||
| *curlcode = CURLE_AGAIN; | ||
| return -1; | ||
| case SSL_ERROR_SYSCALL: | ||
| - Curl_strerror(SOCKERRNO, error_buffer, sizeof(error_buffer)); | ||
| - failf(conn->data, OSSL_PACKAGE " SSL_write: %s", error_buffer); | ||
| - *curlcode = CURLE_SEND_ERROR; | ||
| - return -1; | ||
| + { | ||
| + int sockerr = SOCKERRNO; | ||
| + sslerror = ERR_get_error(); | ||
| + if(sslerror) | ||
| + ossl_strerror(sslerror, error_buffer, sizeof(error_buffer)); | ||
| + else if(sockerr) | ||
| + Curl_strerror(sockerr, error_buffer, sizeof(error_buffer)); | ||
| + else { | ||
| + strncpy(error_buffer, SSL_ERROR_to_str(err), sizeof(error_buffer)); | ||
| + error_buffer[sizeof(error_buffer) - 1] = '\0'; | ||
| + } | ||
| + failf(conn->data, OSSL_PACKAGE " SSL_write: %s, errno %d", | ||
| + error_buffer, sockerr); | ||
| + *curlcode = CURLE_SEND_ERROR; | ||
| + return -1; | ||
| + } | ||
| case SSL_ERROR_SSL: | ||
| /* A failure in the SSL library occurred, usually a protocol error. | ||
| The OpenSSL error queue contains more information on the error. */ | ||
| @@ -3901,11 +3922,6 @@ static ssize_t ossl_recv(struct connectdata *conn, /* connection data */ | ||
| /* there's data pending, re-invoke SSL_read() */ | ||
| *curlcode = CURLE_AGAIN; | ||
| return -1; | ||
| - case SSL_ERROR_SYSCALL: | ||
| - Curl_strerror(SOCKERRNO, error_buffer, sizeof(error_buffer)); | ||
| - failf(conn->data, OSSL_PACKAGE " SSL_read: %s", error_buffer); | ||
| - *curlcode = CURLE_RECV_ERROR; | ||
| - return -1; | ||
| default: | ||
| /* openssl/ssl.h for SSL_ERROR_SYSCALL says "look at error stack/return | ||
| value/errno" */ | ||
| @@ -3914,14 +3930,44 @@ static ssize_t ossl_recv(struct connectdata *conn, /* connection data */ | ||
| if((nread < 0) || sslerror) { | ||
| /* If the return code was negative or there actually is an error in the | ||
| queue */ | ||
| + int sockerr = SOCKERRNO; | ||
| + if(sslerror) | ||
| + ossl_strerror(sslerror, error_buffer, sizeof(error_buffer)); | ||
| + else if(sockerr && err == SSL_ERROR_SYSCALL) | ||
| + Curl_strerror(sockerr, error_buffer, sizeof(error_buffer)); | ||
| + else { | ||
| + strncpy(error_buffer, SSL_ERROR_to_str(err), sizeof(error_buffer)); | ||
| + error_buffer[sizeof(error_buffer) - 1] = '\0'; | ||
| + } | ||
| failf(conn->data, OSSL_PACKAGE " SSL_read: %s, errno %d", | ||
| - (sslerror ? | ||
| - ossl_strerror(sslerror, error_buffer, sizeof(error_buffer)) : | ||
| - SSL_ERROR_to_str(err)), | ||
| - SOCKERRNO); | ||
| + error_buffer, sockerr); | ||
| + *curlcode = CURLE_RECV_ERROR; | ||
| + return -1; | ||
| + } | ||
| + /* For debug builds be a little stricter and error on any | ||
| + SSL_ERROR_SYSCALL. For example a server may have closed the connection | ||
| + abruptly without a close_notify alert. For compatibility with older | ||
| + peers we don't do this by default. #4624 | ||
| + | ||
| + We can use this to gauge how many users may be affected, and | ||
| + if it goes ok eventually transition to allow in dev and release with | ||
| + the newest OpenSSL: #if (OPENSSL_VERSION_NUMBER >= 0x10101000L) */ | ||
| +#ifdef DEBUGBUILD | ||
| + if(err == SSL_ERROR_SYSCALL) { | ||
| + int sockerr = SOCKERRNO; | ||
| + if(sockerr) | ||
| + Curl_strerror(sockerr, error_buffer, sizeof(error_buffer)); | ||
| + else { | ||
| + msnprintf(error_buffer, sizeof(error_buffer), | ||
| + "Connection closed abruptly"); | ||
| + } | ||
| + failf(conn->data, OSSL_PACKAGE " SSL_read: %s, errno %d" | ||
| + " (Fatal because this is a curl debug build)", | ||
| + error_buffer, sockerr); | ||
| *curlcode = CURLE_RECV_ERROR; | ||
| return -1; | ||
| } | ||
| +#endif | ||
| } | ||
| } | ||
| return nread; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters