Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURLE_RECV_ERROR schannel: SEC_E_DECRYPT_FAILURE #9431

Closed
egorpugin opened this issue Sep 5, 2022 · 20 comments
Closed

CURLE_RECV_ERROR schannel: SEC_E_DECRYPT_FAILURE #9431

egorpugin opened this issue Sep 5, 2022 · 20 comments
Labels
TLS Windows Windows-specific

Comments

@egorpugin
Copy link

egorpugin commented Sep 5, 2022

I did this

libcurl
GET https://raw.githubusercontent.com/SoftwareNetwork/database/master//db.version

I expected the following

Everything worked ok for a long time.

curl/libcurl version

7.85.0

operating system

win11, native tls (schannel)

Output

*   Trying 185.199.110.133:443...
* Connected to raw.githubusercontent.com (185.199.110.133) port 443 (#0)
* schannel: disabled automatic use of client certificate
* ALPN: offers h2
* ALPN: offers http/1.1
* ALPN: server accepted h2
* Using HTTP2, server supports multiplexing
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* h2h3 [:method: GET]
* h2h3 [:path: /SoftwareNetwork/database/master//db.version]
* h2h3 [:scheme: https]
* h2h3 [:authority: raw.githubusercontent.com]
* h2h3 [accept: */*]
* Using Stream ID: 1 (easy handle 0x2913cc49bd0)
> GET /SoftwareNetwork/database/master//db.version HTTP/2
Host: raw.githubusercontent.com
accept: */*

* schannel: failed to decrypt data, need more data
* schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.
* Failed receiving HTTP2 data
* Connection #0 to host raw.githubusercontent.com left intact

Same for http1.1.
curl_easy_setopt(curl, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1);

*   Trying 185.199.110.133:443...
* Connected to raw.githubusercontent.com (185.199.110.133) port 443 (#0)
* schannel: disabled automatic use of client certificate
* ALPN: offers http/1.1
* ALPN: server accepted http/1.1
> GET /SoftwareNetwork/database/master//db.version HTTP/1.1
Host: raw.githubusercontent.com
Accept: */*

* schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.
* Closing connection 0
* schannel: shutting down SSL/TLS connection with raw.githubusercontent.com port 443
@bagder bagder added TLS Windows Windows-specific labels Sep 5, 2022
@bagder
Copy link
Member

bagder commented Sep 6, 2022

/cc @gvollant @DHowett @wyattoday any clues?

@jay
Copy link
Member

jay commented Sep 7, 2022

Is this reproducible with the curl tool (curl.exe)? How did you build libcurl? Please give us minimal self contained example code that can be used to reproduce. FWIW this was not reproducible for me using the curl tool in Windows 7, 8 or 10 (21H2).

@egorpugin
Copy link
Author

This is custom libcurl build. I re-check my flags. Maybe there was some new HAVE_... checks etc.

@egorpugin
Copy link
Author

Here is a repro.

  1. Download https://curl.se/windows/dl-7.85.0_1/curl-7.85.0_1-win64-mingw.zip and unpack.
  2. set CURL_SSL_BACKEND=schannel
  3. curl.exe https://raw.githubusercontent.com/SoftwareNetwork/database/master//db.version -v
>curl.exe https://raw.githubusercontent.com/SoftwareNetwork/database/master//db.version -v
*   Trying 185.199.111.133:443...
* Connected to raw.githubusercontent.com (185.199.111.133) port 443 (#0)
* schannel: disabled automatic use of client certificate
* ALPN: offers h2
* ALPN: offers http/1.1
* ALPN: server accepted h2
* Using HTTP2, server supports multiplexing
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* h2h3 [:method: GET]
* h2h3 [:path: /SoftwareNetwork/database/master//db.version]
* h2h3 [:scheme: https]
* h2h3 [:authority: raw.githubusercontent.com]
* h2h3 [user-agent: curl/7.85.0]
* h2h3 [accept: */*]
* Using Stream ID: 1 (easy handle 0x2581a4da910)
> GET /SoftwareNetwork/database/master//db.version HTTP/2
> Host: raw.githubusercontent.com
> user-agent: curl/7.85.0
> accept: */*
>
* schannel: failed to decrypt data, need more data
* schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.
* Failed receiving HTTP2 data
* Connection #0 to host raw.githubusercontent.com left intact
curl: (56) Failed receiving HTTP2 data

@jay
Copy link
Member

jay commented Sep 12, 2022

That's reproducible for me in Windows 11 and it happens regardless of HTTP version setting or TLS setting. curl 7.55.1 (packaged version that comes with Windows) seems to work though so the issue can probably be bisected. I don't have a development environment set up in Windows 11 yet, I'm still in the process of configuring a VM for it.

@jay
Copy link
Member

jay commented Sep 12, 2022

It looks like a TLS 1.3 issue, when I set --tls-max 1.2 I can't reproduce.

/cc @wyattoday

@wyattoday
Copy link
Contributor

wyattoday commented Sep 12, 2022

I agree that this is an issue with how the internals changes in SChannel when using TLS 1.3.

The actual code I wrote runs on Windows 10, 1809 (a.k.a. Windows 10 build 17763) and newer regardless of whether TLS 1.3 is used or not. The only difference is the presence of the SP_PROT_TLS1_3_CLIENT flag. So, when --tls-max 1.2 is present, the only difference in code the being executed (the code in curl itself) is the absence of the SP_PROT_TLS1_3_CLIENT flag.

So, this internal SChannel code path difference depending on whether it takes the TLS 1.2 route or the TLS 1.3 route.

I assume this is the exact same issue reported here: #9451 . Namely a bug in the ALPN code because SChannel altered the way it handled some internal code.

My guess is that if they (@egorpugin) disabled HTTP2 while still using TLS 1.3 it would work (similar to the other issue).

I don't currently have time to dig into either one of these issues, but I might have some time end of next week if no one picks it up in the meantime.

@JDepooter
Copy link
Contributor

I have been doing a bit of investigation into this problem. I am easily able to reproduce the problem on my Windows 11 machine. This is still reproducible after #9756 is merged, so it is not the same problem as reported in #9451. The problem persists regardless of HTTP version used, and whether or not ALPN is used.

Here is the command line I used in my testing:

curld.exe --verbose --no-alpn --http1.1 https://raw.githubusercontent.com/SoftwareNetwork/database/master/db.version

My full output from this command is here, I will go through this in a bit more detail below.

  • STATE: INIT => CONNECT handle 0x1e511f07a30; line 1880 (connection #-5000)
  • Added connection 0. The cache now contains 1 members
  • STATE: CONNECT => RESOLVING handle 0x1e511f07a30; line 1926 (connection #0)
  • Trying 185.199.111.133:443...
  • STATE: RESOLVING => CONNECTING handle 0x1e511f07a30; line 2010 (connection #0)
  • Connected to raw.githubusercontent.com (185.199.111.133) port 443 (#0)
  • STATE: CONNECTING => PROTOCONNECT handle 0x1e511f07a30; line 2075 (connection #0)
  • schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 1/3)
  • Didn't find Session ID in cache for host HTTPS://raw.githubusercontent.com:443
  • schannel: checking server certificate revocation
  • schannel: disabled automatic use of client certificate
  • schannel: sending initial handshake data: sending 289 bytes.
  • schannel: sent initial handshake data: sent 289 bytes
  • schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 2/3)
  • schannel: failed to receive handshake, need more data
  • STATE: PROTOCONNECT => PROTOCONNECTING handle 0x1e511f07a30; line 2093 (connection #0)
  • schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 2/3)
  • schannel: encrypted data got 4096
  • schannel: encrypted data buffer: offset 4096 length 4096
  • schannel: sending next handshake data: sending 80 bytes.
  • schannel: encrypted data length: 19
  • schannel: SSL/TLS handshake complete
  • schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 3/3)
  • Didn't find Session ID in cache for host HTTPS://raw.githubusercontent.com:443
  • Added Session ID to cache for HTTPS://raw.githubusercontent.com:443 [server]
  • schannel: stored credential handle in session cache
  • STATE: PROTOCONNECTING => DO handle 0x1e511f07a30; line 2112 (connection #0)

GET /SoftwareNetwork/database/master/db.version HTTP/1.1
Host: raw.githubusercontent.com
User-Agent: curl/7.86.0-DEV
Accept: /

  • STATE: DO => DID handle 0x1e511f07a30; line 2192 (connection #0)
  • STATE: DID => PERFORMING handle 0x1e511f07a30; line 2311 (connection #0)
  • schannel: client wants to read 102400 bytes
  • schannel: encdata_buffer resized 103424
  • schannel: encrypted data buffer: offset 19 length 103424
  • schannel: encrypted data got 903
  • schannel: encrypted data buffer: offset 922 length 103424
  • schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.
  • schannel: schannel_recv cleanup
  • multi_done: status: 56 prem: 1 done: 0
  • The cache now contains 0 members
  • Closing connection 0
  • schannel: shutting down SSL/TLS connection with raw.githubusercontent.com port 443
  • schannel: clear security context handle
  • Expire cleared (transfer 0x1e511f07a30)
    curl: (56) Failure when receiving data from the peer

I have tracked this down to a problem in "step 2" of the TLS connection. It appears that the initial traffic from the server to libcurl (ServerHello and its associated data) are not fully processed.

In step 1, libcurl calls InitializeSecurityContext to generate the ClientHello and its associated data. This is then sent to the server, as indicated by this line in the log:

  • schannel: sent initial handshake data: sent 289 bytes

We then move to step 2, where we read data from the server and pass it to another call to InitializeSecurityContext. Here are the relevant lines:

  • schannel: encrypted data got 4096
  • schannel: encrypted data buffer: offset 4096 length 4096

This is where things get weird. The call to InitializeSecurityContext returns SEC_E_OK, indicating that this stage of the handshake is complete. It populates a SECBUFFER_TOKEN buffer with the outgoing "Client Finished" message for the server. However, it also populates a buffer of type SECBUFFER_EXTRA indicating that not all of the 4096 bytes have been consumed. In the above log, there are 19 bytes which have not been consumed:

  • schannel: encrypted data length: 19

Since the return code was SEC_E_OK, libcurl moves to step 3 of the TLS handshake, and sends the "Client Finished" message:

  • schannel: sending next handshake data: sending 80 bytes.

After this libcurl calls the schannel_send function with the HTTP request, and then the schannel_recv function to read the response:

  • schannel: client wants to read 102400 bytes
  • schannel: encdata_buffer resized 103424
  • schannel: encrypted data buffer: offset 19 length 103424
  • schannel: encrypted data got 903
  • schannel: encrypted data buffer: offset 922 length 103424

Notice that the 19 bytes from the initial ServerHello message are still there. When libcurl attempts to get the decrypted data via the DecryptMessage function, this extra data causes the decryption to fail:

  • schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.

If I change the CURL_SCHANNEL_BUFFER_INIT_SIZE to a larger value (8192), the connection succeeds. In this case, the entire ServerHello message is processed at once. Here is a snippet from a successful request:

  • schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 2/3)
  • schannel: encrypted data got 4292
  • schannel: encrypted data buffer: offset 4292 length 8192
  • schannel: sending next handshake data: sending 80 bytes.
  • schannel: encrypted data length: 215
  • schannel: SSL/TLS handshake complete

So it appears that the second call to InitializeSecurityContext is returning SEC_E_OK when there is still unread handshake data in the socket. At this point it feels as though we are working around a bug in the Windows SChannel library. I think there are a few options to consider here, some of which I have already tried and ruled out.

  1. We can just increase the initial buffer size. Obviously this is less than ideal, especially for servers which have long certificate chains which may require a large ServerHello message.
  2. We can decrease the initial buffer size, trying to force the correct "incomplete message" behaviour from SChannel. This did not work, the InitializeSecurityContext function still eventually returned SEC_E_OK with an SECBUFFER_EXTRA buffer indicating unconsumed bytes.
  3. We can add some code to make sure the socket is empty once we get a SEC_E_OK return value from InitializeSecurityContext. I tried this by adding another call to Curl_read_plain after the call to InitializeSecurityContext. This also did not work, I guess those remaining bytes are actually important.
  4. We can modify the loop in "step 2", so that it continues to read data when any handshake data is not consumed, even if the InitializeSecurityContext function returns SEC_E_OK. The logic is a bit tricky, because we would need to correctly handle sending the Client Finished handshake message while still processing the ServerHello hello message. I have a working proof of concept of this, and am working on a pull request now.

@jay
Copy link
Member

jay commented Oct 26, 2022

Thanks for the excellent analysis.

This is where things get weird. The call to InitializeSecurityContext returns SEC_E_OK, indicating that this stage of the handshake is complete.

I'd agree... InitializeSecurityContext (Schannel) return value SEC_E_OK says:

"The security context was successfully initialized. There is no need for another InitializeSecurityContext (Schannel) call. If the function returns an output token, that is, if the SECBUFFER_TOKEN in pOutput is of nonzero length, that token must be sent to the server."

So it appears that the second call to InitializeSecurityContext is returning SEC_E_OK when there is still unread handshake data in the socket.

We already have logic that was added a decade ago in 72a5813 for a large handshake problem with Windows CE. Currently that logic looks like this:

curl/lib/vtls/schannel.c

Lines 1538 to 1567 in 93d0928

/* check if there was additional remaining encrypted data */
if(inbuf[1].BufferType == SECBUFFER_EXTRA && inbuf[1].cbBuffer > 0) {
DEBUGF(infof(data, "schannel: encrypted data length: %lu",
inbuf[1].cbBuffer));
/*
There are two cases where we could be getting extra data here:
1) If we're renegotiating a connection and the handshake is already
complete (from the server perspective), it can encrypted app data
(not handshake data) in an extra buffer at this point.
2) (sspi_status == SEC_I_CONTINUE_NEEDED) We are negotiating a
connection and this extra data is part of the handshake.
We should process the data immediately; waiting for the socket to
be ready may fail since the server is done sending handshake data.
*/
/* check if the remaining data is less than the total amount
and therefore begins after the already processed data */
if(backend->encdata_offset > inbuf[1].cbBuffer) {
memmove(backend->encdata_buffer,
(backend->encdata_buffer + backend->encdata_offset) -
inbuf[1].cbBuffer, inbuf[1].cbBuffer);
backend->encdata_offset = inbuf[1].cbBuffer;
if(sspi_status == SEC_I_CONTINUE_NEEDED) {
doread = FALSE;
continue;
}
}
}
else {
backend->encdata_offset = 0;
}

Perhaps that logic is too restrictive? We would need to read more data to complete the handshake, so doread would need to be set true, correct? But the comments there worry me. How can we be expected to tell if the extra data is handshake data or app data at that point?

@egorpugin
Copy link
Author

egorpugin commented Oct 26, 2022

I've found this page regarding SECBUFFER_EXTRA.
https://learn.microsoft.com/en-us/windows/win32/secauthn/extra-buffers-returned-by-schannel

It says:

When the input buffers contains too much information, Schannel does not treat this as an error. The function processes as much of the input as it can, and returns the status code for that processing activity. In addition, Schannel indicates the presence of unprocessed information in the input buffers by returning an output buffer of type SECBUFFER_EXTRA. Testing the output buffers for this type of buffer is the only way to detect this situation. The cbBuffer field of the extra buffer structure indicates how many bytes of input were not processed.

@JDepooter
Copy link
Contributor

My potential fix for this problem is available here:
https://github.com/JDepooter/curl/tree/9431_schannel_recv_error

Note that that branch includes the commit in #9807, so it is really just the final two commits which are of interest to this issue.

I have some reservations about the fix however. As @jay mentioned in his previous comment, we cannot really know whether the "extra" data reported by the InitializeSecurityContext function is further handshake data or application data. We really need to use the return value to indicate this, but it is not reliable at this point. Notably, the TLS 1.3 specification allows for the server to start sending application data with the ServerHello message:
https://datatracker.ietf.org/doc/html/rfc8446#section-2

This is unlikely with https, because the server would not know which data to serve without first receiving a request. I imagine this could be possible in other protocols (ftps maybe?).

@jay
Copy link
Member

jay commented Oct 27, 2022

@andrey-popov are we doing this wrong or is it a bug in schannel?

@andrey-popov
Copy link

@andrey-popov are we doing this wrong or is it a bug in schannel?

A mistag? 🙂

@wyattoday
Copy link
Contributor

I think @jay meant to ping @Andrei-Popov to see if this is a bug in the internals of SChannel.

@JDepooter
Copy link
Contributor

I've discovered a potential workaround for this issue. When I turn off certificate revocation checking (via --ssl-no-revoke or the equivalent libcurl option), the error no longer occurs. Using wireshark, the difference in the ClientHello message is the absence of the status_request extension when revocation checking is turned off. This leads me to believe this is a problem in the way SChannel is handling CRL or OCSP in TLS 1.3.

Due to the way that TLS 1.3 works, it is not really possible to examine the relevant parts of the ServerHello message in Wireshark. So this is a useful clue as to what is going on, but ultimately not too helpful with regards to finding a solution to the problem.

@jay
Copy link
Member

jay commented Nov 12, 2022

@JDepooter though we cannot decrypt Schannel client TLS sessions we can decrypt OpenSSL client TLS sessions so let's take a look at that:

curl 7.86.0 (x86_64-pc-linux-gnu) libcurl/7.86.0 OpenSSL/3.0.7 zlib/1.2.8 nghttp2/1.50.0 librtmp/2.3

SSLKEYLOGFILE=/mnt/hgfs/shared/sslkeylog curl https://raw.githubusercontent.com/SoftwareNetwork/database/master//db.version -v 

client:
TLSv1.3 Record Layer: Handshake Protocol: Client Hello (517 bytes)

server:
TLSv1.3 Record Layer: Handshake Protocol: Server Hello (127 bytes)
TLSv1.3 Record Layer: Change Cipher Spec Protocol: Change Cipher Spec (6 bytes)
TLSv1.3 Record Layer: Handshake Protocol: Encrypted Extensions (41 bytes)
TLSv1.3 Record Layer: Handshake Protocol: Certificate (3073 bytes)
TLSv1.3 Record Layer: Handshake Protocol: Certificate Verify (286 bytes)
TLSv1.3 Record Layer: Handshake Protocol: Finished (74 bytes)
TLSv1.3 Record Layer: Handshake Protocol: New Session Ticket (215 bytes)

client:
TLSv1.3 Record Layer: Change Cipher Spec Protocol: Change Cipher Spec (6 bytes)
TLSv1.3 Record Layer: Handshake Protocol: Finished (74 bytes)

If --cert-status is used (ie status_request extension type ocsp added to Client Hello) the only difference is the Certificate tls record is larger at 3552 bytes.

Note that server's New Session Ticket is sent after server's handshake Finished, which is allowed in TLS 1.3. We had a problem with that and FTPS in #6149, basically we did not read New Session Ticket on upload because sending on a separate socket with FTPS we didn't expect to have to read anything. Though I don't think that's the issue here it's still notable because possibly what's happening is some incomplete part of server's New Session Ticket is what can't be decrypted by Schannel. Of course Schannel and OpenSSL TLS 1.3 ClientHello are different but we can assume this is a possibility.


Back to Schannel. If I disable the prerecv buffer decryption does not fail. I wonder if you see similar results?

diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 59efe4d..3adcfbc 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -780,6 +780,7 @@ endings either CRLF or LF so 't' is appropriate.
  * wrappers for WinSock sockets. https://github.com/curl/curl/issues/657
  * Define DONT_USE_RECV_BEFORE_SEND_WORKAROUND to force disable workaround.
  */
+#define DONT_USE_RECV_BEFORE_SEND_WORKAROUND
 #if !defined(DONT_USE_RECV_BEFORE_SEND_WORKAROUND)
 #  if defined(WIN32) || defined(__CYGWIN__)
 #    define USE_RECV_BEFORE_SEND_WORKAROUND
--- fail.txt	2022-11-12 04:06:27.722117500 -0500
+++ success.txt	2022-11-12 04:06:32.284378400 -0500
@@ -1,7 +1,19 @@
 * schannel: client wants to read 102400 bytes
 * schannel: encdata_buffer resized 103424
 * schannel: encrypted data buffer: offset 19 length 103424
-* schannel: encrypted data got 716
-* schannel: encrypted data buffer: offset 735 length 103424
-* schannel: failed to read data from server: SEC_E_DECRYPT_FAILURE (0x80090330) - The specified data could not be decrypted.
-* schannel: schannel_recv cleanup
+* schannel: encrypted data got 913
+* schannel: encrypted data buffer: offset 932 length 103424
+* schannel: decrypted data length: 0
+* schannel: decrypted data added: 0
+* schannel: decrypted cached: offset 0 length 102400
+* schannel: encrypted data length: 932
+* schannel: encrypted cached: offset 932 length 103424
+* schannel: remote party requests renegotiation
+* schannel: renegotiating SSL/TLS connection
+* schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 2/3)
+* schannel: encrypted data buffer: offset 932 length 103424
+* schannel: encrypted data length: 717
+* schannel: SSL/TLS handshake complete
+* schannel: SSL/TLS connection with raw.githubusercontent.com port 443 (step 3/3)
+* Found Session ID in cache for host HTTPS://raw.githubusercontent.com:443
+* schannel: SSL/TLS connection renegotiated

What prerecv is for:

curl/lib/sendf.c

Lines 154 to 158 in cd95ee9

/* WinSock will destroy unread received data if send() is
failed.
To avoid lossage of received data, recv() must be
performed before every send() if any incoming data is
available. However, skip this, if buffer is already full. */

Interestingly, Curl_read_plain does not fetch prerecv data but Curl_recv_plain (which looks functionally the same) does. Guess which one schannel_step2 and schannel_recv calls...

curl/lib/vtls/schannel.c

Lines 2200 to 2204 in cd95ee9

/* read encrypted data from socket */
*err = Curl_read_plain(conn->sock[sockindex],
(char *)(backend->encdata_buffer +
backend->encdata_offset),
size, &nread);

So here's a theory, possibly some part of the New Session Ticket record has been squirreled away in the prerecv buffer and so it is missed by schannel code. Only socks and schannel call Curl_read_plain instead of Curl_recv_plain and I don't know why.

jay added a commit to jay/curl that referenced this issue Nov 14, 2022
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).

Ref: curl#9431 (comment)

Fixes curl#9431
Closes #xxxx
@jay
Copy link
Member

jay commented Nov 14, 2022

Please try #9904

jay added a commit to jay/curl that referenced this issue Nov 15, 2022
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).

Ref: curl#9431 (comment)

Fixes curl#9431
Closes #xxxx
@JDepooter
Copy link
Contributor

Awesome, this works for me. I had a feeling that pre-recv buffer was involved some how.

@jay jay closed this as completed in 12e1def Nov 18, 2022
bagder added a commit that referenced this issue Nov 18, 2022
This reverts commit 12e1def.

It introduced SOCKS proxy fails, like test 700 never ending.

Reopens #9431
@bagder
Copy link
Member

bagder commented Nov 18, 2022

Reopened by 18383fb

@bagder bagder reopened this Nov 18, 2022
jay added a commit to jay/curl that referenced this issue Nov 18, 2022
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.

Ref: curl#9431 (comment)

Assisted-by: Joel Depooter
Reported-by: Egor Pugin

Fixes curl#9431
Closes curl#9904
@jay
Copy link
Member

jay commented Nov 18, 2022

Reopened by 18383fb

Take 2 in #9949.

@jay jay closed this as completed in 4f42150 Nov 20, 2022
rforge pushed a commit to r-devel/r-dev-web that referenced this issue Dec 19, 2022
erengy added a commit to erengy/taiga that referenced this issue Jan 2, 2023
jay added a commit to jay/curl that referenced this issue Feb 3, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this behavior, though it could possibly
be handled more correctly at a higher level (eg transfer handling checks
for server error response before it has completed sending its request).

Ref: curl#9431
Ref: curl#10253

Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 3, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this behavior, though it could possibly
be handled more correctly at a higher level (eg transfer handling checks
for server error response before it has completed sending its request).

Ref: curl#9431
Ref: curl#10253

Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 7, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 7, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 8, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 8, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 8, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 8, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit to jay/curl that referenced this issue Feb 8, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
jay added a commit that referenced this issue Feb 9, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (#9431)
- HTTP/2 arbitrary hangs (#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: #657
Ref: #668
Ref: #720

Ref: #9431
Ref: #10253

Closes #10409
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
Prior to this change a workaround for Windows to recv before every send
was enabled by default. The way it works is a recv is called before
every send and saves the received data, in case send fails because in
Windows apparently that can wipe out the socket's internal received
data buffer.

This feature has led to several bugs because the way libcurl operates
it waits on a socket to read or to write, and may not at all times
check for buffered receive data.

Two recent significant bugs this workaround caused:
- Broken Schannel TLS 1.3 connections (curl#9431)
- HTTP/2 arbitrary hangs (curl#10253)

The actual code remains though it is disabled by default. Though future
changes to connection filter buffering could improve the situation IMO
it's just not tenable to manage this workaround.

Ref: curl#657
Ref: curl#668
Ref: curl#720

Ref: curl#9431
Ref: curl#10253

Closes curl#10409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TLS Windows Windows-specific
Development

Successfully merging a pull request may close this issue.

6 participants