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

tls: fix bugs of double TLS #48796

Closed
wants to merge 4 commits into from

Conversation

ywave620
Copy link
Contributor

@ywave620 ywave620 commented Jul 16, 2023

Fixs two issues in TLSWrap, one of them is reported in #30896 (a 3 years log bug ...).

  1. TLSWrap has exactly one StreamListener, however, that StreamListener can be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successor StreamListener.

  2. A TLSWrap does not allow more than one active write, as checked in the assertion about current_write in TLSWrap::DoWrite().

However, when users make use of an existing tls.TLSSocket to establish double TLS, by
either

  tls.connect({socket: tlssock})

or

  tlsServer.emit('connection', tlssock)

we have both of the user provided tls.TLSSocket, tlssock and a brand new created TLSWrap writing to the TLSWrap bound to tlssock, which easily violates the constranint because two writers have no idea of each other.

The design of the fix is:
when a TLSWrap is created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired.

Fixs two issues in `TLSWrap`, one of them is reported in
nodejs#30896.

1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.

2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.

However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
  tls.connect({socket: tlssock})
or
  tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.

The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 16, 2023
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2023
@nodejs-github-bot
Copy link
Collaborator

lib/_tls_wrap.js Outdated Show resolved Hide resolved
Remove unnecessary saving of `this` for arrow function.
@ywave620
Copy link
Contributor Author

leftover processes found in the github hooked CI Test ASan / test-asan

ps awwx | grep Release/node | grep -v grep | cat
 105758 ?        tl     0:00 /home/runner/work/node/node/out/Release/node /home/runner/work/node/node/test/parallel/test-cluster-primary-error.js cluster
 105791 ?        S      0:00 /home/runner/work/node/node/out/Release/node /home/runner/work/node/node/test/parallel/test-cluster-primary-error.js cluster
make[1]: *** [Makefile:554: test-ci] Error 1
make: *** [Makefile:579: run-ci] Error 2
Error: Process completed with exit code 2.

But it seems like these failures have nothing to do with this PR. Could somebody rerun the CI?

@ywave620
Copy link
Contributor Author

cc @nodejs/crypto

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -136,7 +136,8 @@ class TLSWrap : public AsyncWrap,
v8::Local<v8::Object> obj,
Kind kind,
StreamBase* stream,
SecureContext* sc);
SecureContext* sc,
bool stream_has_active_write);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit: I'm generally not a fan of bool arguments to methods, largely because just seeing the true or false a the callsite tends not to be descriptive enough to let folks know what the impact is. I certainly won't block this change because of it, but I'd prefer a enum arg here instead.

@mhdawson
Copy link
Member

Landed in 662fe0be9d8e

@mhdawson mhdawson closed this Jul 28, 2023
@mhdawson
Copy link
Member

arg, just noticed there was a suggestion on the PR after I landed it. @jasnell sorry about that.

@ywave620 any chance you could open a new PR to address that.

@ywave620
Copy link
Contributor Author

arg, just noticed there was a suggestion on the PR after I landed it. @jasnell sorry about that.

@ywave620 any chance you could open a new PR to address that.

Totally ok

@ywave620
Copy link
Contributor Author

@ywave620 any chance you could open a new PR to address that.

#48969. @mhdawson PTAL

@mhdawson
Copy link
Member

mhdawson commented Aug 2, 2023

@ywave620 many thanks

@mhdawson
Copy link
Member

mhdawson commented Aug 2, 2023

@ywave620 I had landed your original version in this PR. What I'd hope for was a PR that just addressed the remaining suggestion from @jasnell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants