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 07d21903d600ceae1a1e12bf6292f34b322c52da..b22dcdb1145902c32545822c8f5fbc5669294905 100644 (file)
--- 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 52bd47c5c5e24c77c100f9a28285f6739aa1bfaa..18724e062dfbb4e2ef4a2b2b7c269eb2c16452af 100644 (file)
--- 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 7c4c1280a0746de4611eb84c18c4f1b3a98d9de0..8af5c460850a098489658aa635a803d46eaa968b 100644 (file)
--- 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 40cd13e0a0af4b4616dcc3fdd489a0396e01c79e..e1f6dbbb274fd14a87223ed5bb309e9fe691a6f4 100644 (file)
--- 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 6b5f3b592215676fe1cb5dcd4a1b37f07531dd9e..6b35efdce954657da4d7b7936aa983e2585b8aee 100644 (file)
--- 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);