sendf: change Curl_read_plain to wrap Curl_recv_plain #9904
Conversation
|
This pull request introduces 1 alert and fixes 1 when merging b59a5fc into 184fc6f - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert and fixes 1 when merging 0d2ae02 into 184fc6f - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert and fixes 1 when merging d95b276 into 184fc6f - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert and fixes 1 when merging f711912 into f491240 - view on LGTM.com new alerts:
fixed alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
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
force-pushed
the
fix_schannel_decrypt_failure
branch
from
f711912
to
a65fda1
Compare
|
This pull request introduces 1 alert and fixes 1 when merging a65fda1 into a8e6351 - view on LGTM.com new alerts:
fixed alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
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
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: #9431 (comment)
Fixes #9431
Closes #xxxx
The pre-receive behavior for Windows was designed to call recv() before every send(), enough to fill a small buffer, because Winsock destroys unread received data in its own buffers when send() fails. Pre-receive is also designed to only be used with HTTP(S) and when the connection has "plain" sockets, eg TLS and HTTP/2 haven't yet been negotiated.
The bug has only been observed during Schannel TLS 1.3 handshakes where the server may send a record immediately after the server's handshake
Finishedrecord. In #9431 it is observed that the server sendsNew Session Ticketimmediately afterFinished. The client (us) reply to the server'sFinishedwith our ownFinishedusing Curl_write_plain, which could then pre-receive some part of theNew Session Ticketreceived before ourFinishedwas sent.