From: Jay Satiro Date: Mon, 14 Nov 2022 08:30:30 +0000 (-0500) Subject: sendf: change Curl_read_plain to wrap Curl_recv_plain (take 2) X-Git-Tag: curl-7_87_0~129 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4f42150d04d1b8eef136f23518c53d0128998a62;p=thirdparty%2Fcurl.git sendf: change Curl_read_plain to wrap Curl_recv_plain (take 2) Prior to this change Curl_read_plain would attempt to read the socket directly. On Windows that's a problem because recv data may be cached by libcurl and that data is only drained using Curl_recv_plain. Rather than rewrite Curl_read_plain to handle cached recv data, I changed it to wrap Curl_recv_plain, in much the same way that Curl_write_plain already wraps Curl_send_plain. Curl_read_plain -> Curl_recv_plain Curl_write_plain -> Curl_send_plain This fixes a bug in the schannel backend where decryption of arbitrary TLS records fails because cached recv data is never drained. We send data (TLS records formed by Schannel) using Curl_write_plain, which calls Curl_send_plain, and that may do a recv-before-send ("pre-receive") to cache received data. The code calls Curl_read_plain to read data (TLS records from the server), which prior to this change did not call Curl_recv_plain and therefore cached recv data wasn't retrieved, resulting in malformed TLS records and decryption failure (SEC_E_DECRYPT_FAILURE). The bug has only been observed during Schannel TLS 1.3 handshakes. Refer to the issue and PR for more information. -- This is take 2 of the original fix. It preserves the original behavior of Curl_read_plain to write 0 to the bytes read parameter on error, since apparently some callers expect that (SOCKS tests were hanging). The original fix which landed in 12e1def5 and was later reverted in 18383fbf failed to work properly because it did not do that. Also, it changes Curl_write_plain the same way to complement Curl_read_plain, and it changes Curl_send_plain to return -1 instead of 0 on CURLE_AGAIN to complement Curl_recv_plain. Behavior on error with these changes: Curl_recv_plain returns -1 and *code receives error code. Curl_send_plain returns -1 and *code receives error code. Curl_read_plain returns error code and *n (bytes read) receives 0. Curl_write_plain returns error code and *written receives 0. -- Ref: https://github.com/curl/curl/issues/9431#issuecomment-1312420361 Assisted-by: Joel Depooter Reported-by: Egor Pugin Fixes https://github.com/curl/curl/issues/9431 Closes https://github.com/curl/curl/pull/9949 --- diff --git a/lib/krb5.c b/lib/krb5.c index 07d21903d6..b22dcdb114 100644 --- a/lib/krb5.c +++ b/lib/krb5.c @@ -454,14 +454,14 @@ static int ftp_send_command(struct Curl_easy *data, const char *message, ...) /* Read |len| from the socket |fd| and store it in |to|. Return a CURLcode saying whether an error occurred or CURLE_OK if |len| was read. */ static CURLcode -socket_read(curl_socket_t fd, void *to, size_t len) +socket_read(struct Curl_easy *data, curl_socket_t fd, void *to, size_t len) { char *to_p = to; CURLcode result; ssize_t nread = 0; while(len > 0) { - result = Curl_read_plain(fd, to_p, len, &nread); + result = Curl_read_plain(data, fd, to_p, len, &nread); if(!result) { len -= nread; to_p += nread; @@ -502,15 +502,15 @@ socket_write(struct Curl_easy *data, curl_socket_t fd, const void *to, return CURLE_OK; } -static CURLcode read_data(struct connectdata *conn, - curl_socket_t fd, +static CURLcode read_data(struct Curl_easy *data, curl_socket_t fd, struct krb5buffer *buf) { + struct connectdata *conn = data->conn; int len; CURLcode result; int nread; - result = socket_read(fd, &len, sizeof(len)); + result = socket_read(data, fd, &len, sizeof(len)); if(result) return result; @@ -525,7 +525,7 @@ static CURLcode read_data(struct connectdata *conn, if(!len || !buf->data) return CURLE_OUT_OF_MEMORY; - result = socket_read(fd, buf->data, len); + result = socket_read(data, fd, buf->data, len); if(result) return result; nread = conn->mech->decode(conn->app_data, buf->data, len, @@ -560,7 +560,7 @@ static ssize_t sec_recv(struct Curl_easy *data, int sockindex, /* Handle clear text response. */ if(conn->sec_complete == 0 || conn->data_prot == PROT_CLEAR) - return sread(fd, buffer, len); + return Curl_recv_plain(data, sockindex, buffer, len, err); if(conn->in_buffer.eof_flag) { conn->in_buffer.eof_flag = 0; @@ -573,7 +573,7 @@ static ssize_t sec_recv(struct Curl_easy *data, int sockindex, buffer += bytes_read; while(len > 0) { - if(read_data(conn, fd, &conn->in_buffer)) + if(read_data(data, fd, &conn->in_buffer)) return -1; if(conn->in_buffer.size == 0) { if(bytes_read > 0) diff --git a/lib/sendf.c b/lib/sendf.c index 52bd47c5c5..18724e062d 100644 --- a/lib/sendf.c +++ b/lib/sendf.c @@ -338,6 +338,7 @@ CURLcode Curl_write(struct Curl_easy *data, } } +/* Curl_send_plain sends raw data without a size restriction on 'len'. */ ssize_t Curl_send_plain(struct Curl_easy *data, int num, const void *mem, size_t len, CURLcode *code) { @@ -386,7 +387,6 @@ ssize_t Curl_send_plain(struct Curl_easy *data, int num, #endif ) { /* this is just a case of EWOULDBLOCK */ - bytes_written = 0; *code = CURLE_AGAIN; } else { @@ -403,7 +403,12 @@ ssize_t Curl_send_plain(struct Curl_easy *data, int num, /* * Curl_write_plain() is an internal write function that sends data to the * server using plain sockets only. Otherwise meant to have the exact same - * proto as Curl_write() + * proto as Curl_write(). + * + * This function wraps Curl_send_plain(). The only difference besides the + * prototype is '*written' (bytes written) is set to 0 on error. + * 'sockfd' must be one of the connection's two main sockets and the value of + * 'len' must not be changed. */ CURLcode Curl_write_plain(struct Curl_easy *data, curl_socket_t sockfd, @@ -415,13 +420,22 @@ CURLcode Curl_write_plain(struct Curl_easy *data, struct connectdata *conn = data->conn; int num; DEBUGASSERT(conn); + DEBUGASSERT(sockfd == conn->sock[FIRSTSOCKET] || + sockfd == conn->sock[SECONDARYSOCKET]); + if(sockfd != conn->sock[FIRSTSOCKET] && + sockfd != conn->sock[SECONDARYSOCKET]) + return CURLE_BAD_FUNCTION_ARGUMENT; + num = (sockfd == conn->sock[SECONDARYSOCKET]); *written = Curl_send_plain(data, num, mem, len, &result); + if(*written == -1) + *written = 0; return result; } +/* Curl_recv_plain receives raw data without a size restriction on 'len'. */ ssize_t Curl_recv_plain(struct Curl_easy *data, int num, char *buf, size_t len, CURLcode *code) { @@ -663,30 +677,39 @@ CURLcode Curl_client_write(struct Curl_easy *data, return chop_write(data, type, ptr, len); } -CURLcode Curl_read_plain(curl_socket_t sockfd, - char *buf, - size_t bytesfromsocket, - ssize_t *n) +/* + * Curl_read_plain() is an internal read function that reads data from the + * server using plain sockets only. Otherwise meant to have the exact same + * proto as Curl_read(). + * + * This function wraps Curl_recv_plain(). The only difference besides the + * prototype is '*n' (bytes read) is set to 0 on error. + * 'sockfd' must be one of the connection's two main sockets and the value of + * 'sizerequested' must not be changed. + */ +CURLcode Curl_read_plain(struct Curl_easy *data, /* transfer */ + curl_socket_t sockfd, /* read from this socket */ + char *buf, /* store read data here */ + size_t sizerequested, /* max amount to read */ + ssize_t *n) /* amount bytes read */ { - ssize_t nread = sread(sockfd, buf, bytesfromsocket); + CURLcode result; + struct connectdata *conn = data->conn; + int num; + DEBUGASSERT(conn); + DEBUGASSERT(sockfd == conn->sock[FIRSTSOCKET] || + sockfd == conn->sock[SECONDARYSOCKET]); + if(sockfd != conn->sock[FIRSTSOCKET] && + sockfd != conn->sock[SECONDARYSOCKET]) + return CURLE_BAD_FUNCTION_ARGUMENT; - if(-1 == nread) { - const int err = SOCKERRNO; - const bool return_error = -#ifdef USE_WINSOCK - WSAEWOULDBLOCK == err -#else - EWOULDBLOCK == err || EAGAIN == err || EINTR == err -#endif - ; - *n = 0; /* no data returned */ - if(return_error) - return CURLE_AGAIN; - return CURLE_RECV_ERROR; - } + num = (sockfd == conn->sock[SECONDARYSOCKET]); - *n = nread; - return CURLE_OK; + *n = Curl_recv_plain(data, num, buf, sizerequested, &result); + if(*n == -1) + *n = 0; + + return result; } /* diff --git a/lib/sendf.h b/lib/sendf.h index 7c4c1280a0..8af5c46085 100644 --- a/lib/sendf.h +++ b/lib/sendf.h @@ -61,9 +61,10 @@ CURLcode Curl_client_write(struct Curl_easy *data, int type, char *ptr, bool Curl_recv_has_postponed_data(struct connectdata *conn, int sockindex); /* internal read-function, does plain socket only */ -CURLcode Curl_read_plain(curl_socket_t sockfd, +CURLcode Curl_read_plain(struct Curl_easy *data, + curl_socket_t sockfd, char *buf, - size_t bytesfromsocket, + size_t sizerequested, ssize_t *n); ssize_t Curl_recv_plain(struct Curl_easy *data, int num, char *buf, diff --git a/lib/socks.c b/lib/socks.c index 40cd13e0a0..e1f6dbbb27 100644 --- a/lib/socks.c +++ b/lib/socks.c @@ -112,7 +112,7 @@ int Curl_blockread_all(struct Curl_easy *data, /* transfer */ result = ~CURLE_OK; break; } - result = Curl_read_plain(sockfd, buf, buffersize, &nread); + result = Curl_read_plain(data, sockfd, buf, buffersize, &nread); if(CURLE_AGAIN == result) continue; if(result) @@ -396,7 +396,7 @@ static CURLproxycode do_SOCKS4(struct Curl_cfilter *cf, /* FALLTHROUGH */ case CONNECT_SOCKS_READ: /* Receive response */ - result = Curl_read_plain(sockfd, (char *)sx->outp, + result = Curl_read_plain(data, sockfd, (char *)sx->outp, sx->outstanding, &actualread); if(result && (CURLE_AGAIN != result)) { failf(data, "SOCKS4: Failed receiving connect request ack: %s", @@ -599,7 +599,7 @@ static CURLproxycode do_SOCKS5(struct Curl_cfilter *cf, sx->outp = socksreq; /* store it here */ /* FALLTHROUGH */ case CONNECT_SOCKS_READ: - result = Curl_read_plain(sockfd, (char *)sx->outp, + result = Curl_read_plain(data, sockfd, (char *)sx->outp, sx->outstanding, &actualread); if(result && (CURLE_AGAIN != result)) { failf(data, "Unable to receive initial SOCKS5 response."); @@ -729,7 +729,7 @@ static CURLproxycode do_SOCKS5(struct Curl_cfilter *cf, sxstate(sx, data, CONNECT_AUTH_READ); /* FALLTHROUGH */ case CONNECT_AUTH_READ: - result = Curl_read_plain(sockfd, (char *)sx->outp, + result = Curl_read_plain(data, sockfd, (char *)sx->outp, sx->outstanding, &actualread); if(result && (CURLE_AGAIN != result)) { failf(data, "Unable to receive SOCKS5 sub-negotiation response."); @@ -931,7 +931,7 @@ static CURLproxycode do_SOCKS5(struct Curl_cfilter *cf, sxstate(sx, data, CONNECT_REQ_READ); /* FALLTHROUGH */ case CONNECT_REQ_READ: - result = Curl_read_plain(sockfd, (char *)sx->outp, + result = Curl_read_plain(data, sockfd, (char *)sx->outp, sx->outstanding, &actualread); if(result && (CURLE_AGAIN != result)) { failf(data, "Failed to receive SOCKS5 connect request ack."); @@ -1030,7 +1030,7 @@ static CURLproxycode do_SOCKS5(struct Curl_cfilter *cf, #endif /* FALLTHROUGH */ case CONNECT_REQ_READ_MORE: - result = Curl_read_plain(sockfd, (char *)sx->outp, + result = Curl_read_plain(data, sockfd, (char *)sx->outp, sx->outstanding, &actualread); if(result && (CURLE_AGAIN != result)) { failf(data, "Failed to receive SOCKS5 connect request ack."); diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c index 6b5f3b5922..6b35efdce9 100644 --- a/lib/vtls/schannel.c +++ b/lib/vtls/schannel.c @@ -1413,7 +1413,7 @@ schannel_connect_step2(struct Curl_easy *data, struct connectdata *conn, for(;;) { if(doread) { /* read encrypted handshake data from socket */ - result = Curl_read_plain(conn->sock[sockindex], + result = Curl_read_plain(data, conn->sock[sockindex], (char *) (backend->encdata_buffer + backend->encdata_offset), backend->encdata_length - @@ -2190,7 +2190,7 @@ schannel_recv(struct Curl_easy *data, int sockindex, backend->encdata_offset, backend->encdata_length)); /* read encrypted data from socket */ - *err = Curl_read_plain(conn->sock[sockindex], + *err = Curl_read_plain(data, conn->sock[sockindex], (char *)(backend->encdata_buffer + backend->encdata_offset), size, &nread);