Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: nodejs/node
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: ef8aa888669f96c5d091a0057964e3e0efae74f4
Choose a base ref
...
head repository: nodejs/node
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 8b89d4d42156046326dffb32c51e256ee7045a53
Choose a head ref
  • 2 commits
  • 8 files changed
  • 1 contributor

Commits on Dec 12, 2022

  1. tls: fix re-entrancy issue with TLS close_notify 

    Like errno, OpenSSL's API requires SSL_get_error and error queue be
checked immediately after the failing operation, otherwise the error
queue or SSL object may have changed state and no longer report
information about the operation the caller wanted.

TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read
picks up a closing alert (detected by checking SSL_get_shutdown), Node
calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to
dispatch on the error. But, by this point, Node has already re-entered
JS, which may change the error.

In particular, I've observed that, on close_notify, JS seems to
sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I
think this comes from onStreamRead in stream_base_commons.js?)

Instead, SSL_get_error and the error queue should be sampled earlier.
Back in #1661, Node needed to account for GetSSLError being called after
ssl_ was destroyed. This was the real cause. With this fixed, there's no
need to account for this. (Any case where ssl_ may be destroyed before
SSL_get_error is a case where ssl_ or the error queue could change
state, so it's a bug either way.)

This is the first of two fixes in error-handling here. The
EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the
peer. Some of the ECONNRESET expectations in the tests aren't actually
correct. The next commit will fix this as well.

PR-URL: #44563
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @davidben @nodejs-github-bot
    davidben authored and nodejs-github-bot committed Dec 12, 2022
    Configuration menu
    Copy the full SHA
    7c74514 View commit details
    Browse the repository at this point in the history

  2. tls: don't treat fatal TLS alerts as EOF 

    SSL_RECEIVED_SHUTDOWN means not just close_notify or fatal alert. From
what I can tell, this was just a mistake? OnStreamRead's comment
suggests eof_ was intended to be for close_notify.

This fixes a bug in TLSSocket error reporting that seems to have made it
into existing tests. If we receive a fatal alert, EmitRead(UV_EOF)
would, via onConnectEnd in _tls_wrap.js, synthesize an ECONNRESET before
the alert itself is surfaced. As a result, TLS alerts received during
the handshake are misreported by Node.

See the tests that had to be updated as part of this.

PR-URL: #44563
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    @davidben @nodejs-github-bot
    davidben authored and nodejs-github-bot committed Dec 12, 2022
    Configuration menu
    Copy the full SHA
    8b89d4d View commit details
    Browse the repository at this point in the history
Loading