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

ssl: read pending close notify alert before closing the connection #7095

Closed
wants to merge 1 commit into from

Conversation

@mkauf
Copy link
Contributor

@mkauf mkauf commented May 18, 2021

This avoids a TCP reset (RST) if the server initiates a connection
shutdown by sending an SSL close notify alert and then closes the TCP
connection.

For SSL connections, usually the server announces that it will close the
connection with an SSL close notify alert. curl should read this alert.
If curl does not read this alert and just closes the connection, some
operating systems close the TCP connection with an RST flag.

See RFC 1122, section 4.2.2.13

If curl reads the close notify alert, the TCP connection is closed
normally with a FIN flag.

The new code is similar to existing code in the "SSL shutdown" function:
try to read an alert (non-blocking), and ignore any read errors.

This avoids a TCP reset (RST) if the server initiates a connection
shutdown by sending an SSL close notify alert and then closes the TCP
connection.

For SSL connections, usually the server announces that it will close the
connection with an SSL close notify alert. curl should read this alert.
If curl does not read this alert and just closes the connection, some
operating systems close the TCP connection with an RST flag.

See RFC 1122, section 4.2.2.13

If curl reads the close notify alert, the TCP connection is closed
normally with a FIN flag.

The new code is similar to existing code in the "SSL shutdown" function:
try to read an alert (non-blocking), and ignore any read errors.
@mkauf
Copy link
Contributor Author

@mkauf mkauf commented May 18, 2021

TLS backends

This pull request implements a fix for:

  • gtls
  • mbedtls
  • nss
  • openssl
  • wolfssl

I am not sure whether a fix is also needed for:

  • bearssl
  • gskit
  • mesalink
  • rustls
  • sectransp

Probably no fix is needed, because Curl_ssl_shutdown() is called when closing:

  • schannel

How to test (on Linux, with the openssl TLS backend)

curl -v -H "Connection: close" https://httpd.apache.org/ > /dev/null

The Apache HTTP server will send a close notify alert and close the connection after the response because of the header "Connection: close".

Without the fix:

* Closing connection 0
} [5 bytes data]
* TLSv1.3 (OUT), TLS alert, close notify (256):
} [2 bytes data]

With the fix:

* Closing connection 0
{ [5 bytes data]
* TLSv1.3 (IN), TLS alert, close notify (256):
{ [2 bytes data]
* TLSv1.3 (OUT), TLS alert, close notify (256):
} [2 bytes data]

The RST/FIN TCP flags can be observed with Wireshark.

Copy link
Contributor

@emilengler emilengler left a comment

I see that you don't use whatever is written to buf. In that case maybe try passing NULL instead of creating a 32 byte variable. Some functions allow that not sure about those though

@jay
Copy link
Member

@jay jay commented May 19, 2021

I see that you don't use whatever is written to buf. In that case maybe try passing NULL instead of creating a 32 byte variable. Some functions allow that not sure about those though

It's fine to use buf and probably safer. This still has the issue there is a race condition where the alert may be received after the read call. I think the only way to deal with this correctly is a non-blocking SSL shutdown state that times out after xx seconds. See also #6149. Though this appears to be an improvement.

@jay jay added the SSL/TLS label May 19, 2021
@mkauf
Copy link
Contributor Author

@mkauf mkauf commented May 19, 2021

I see that you don't use whatever is written to buf. In that case maybe try passing NULL instead of creating a 32 byte variable. Some functions allow that not sure about those though

The documentation of OpenSSL does not say anything about passing NULL or a buffer with zero length to SSL_read(), so I assume that this is not allowed. I have used 32 as the buffer size, but any other value > 0 should also work.

This still has the issue there is a race condition where the alert may be received after the read call. I think the only way to deal with this correctly is a non-blocking SSL shutdown state that times out after xx seconds. See also #6149. Though this appears to be an improvement.

Thank you for the link to #6149. A non-blocking SSL shutdown state would be difficult to implement. I hope that this improvement is "good enough".

@bagder
bagder approved these changes May 20, 2021
@mkauf mkauf closed this in b249592 Jun 1, 2021
@mkauf mkauf deleted the mkauf:read_ssl_close_notify_alert branch Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants