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

[v10.x] backport more TLS changes #27967

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
src: in-source comments and minor TLS cleanups
Renamed some internal C++ methods and properties for consistency, and
commented SSL I/O.

- Rename waiting_new_session_ after is_waiting_new_session(), instead of
  using reverse naming (new_session_wait_), and change "waiting" to
  "awaiting".
- Make TLSWrap::ClearIn() return void, the value is never used.
- Fix a getTicketKeys() cut-n-paste error. Since it doesn't use the
  arguments, remove them from the js wrapper.
- Remove call of setTicketKeys(getTicketKeys()), its a no-op.

PR-URL: #25713
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
sam-github authored and addaleax committed May 30, 2019
commit b800d297e17253f21bb0b2dce95e72e56f68a4b8
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
@@ -929,8 +929,8 @@ Server.prototype._setServerData = function(data) {
};


Server.prototype.getTicketKeys = function getTicketKeys(keys) {
return this._sharedCreds.context.getTicketKeys(keys);
Server.prototype.getTicketKeys = function getTicketKeys() {
return this._sharedCreds.context.getTicketKeys();
};


5 changes: 3 additions & 2 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
@@ -1475,7 +1475,7 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
reinterpret_cast<const char*>(session_id),
session_id_length).ToLocalChecked();
Local<Value> argv[] = { session, buff };
w->new_session_wait_ = true;
w->awaiting_new_session_ = true;
w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);

return 0;
@@ -2003,6 +2003,7 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {

ClearErrorOnReturn clear_error_on_return;

// XXX(sam) Return/throw an error, don't discard the SSL error reason.
bool yes = SSL_renegotiate(w->ssl_.get()) == 1;
args.GetReturnValue().Set(yes);
}
@@ -2036,7 +2037,7 @@ template <class Base>
void SSLWrap<Base>::NewSessionDone(const FunctionCallbackInfo<Value>& args) {
Base* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
w->new_session_wait_ = false;
w->awaiting_new_session_ = false;
w->NewSessionDoneCb();
}

6 changes: 3 additions & 3 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
@@ -218,7 +218,7 @@ class SSLWrap {
kind_(kind),
next_sess_(nullptr),
session_callbacks_(false),
new_session_wait_(false),
awaiting_new_session_(false),
cert_cb_(nullptr),
cert_cb_arg_(nullptr),
cert_cb_running_(false) {
@@ -234,7 +234,7 @@ class SSLWrap {
inline void enable_session_callbacks() { session_callbacks_ = true; }
inline bool is_server() const { return kind_ == kServer; }
inline bool is_client() const { return kind_ == kClient; }
inline bool is_waiting_new_session() const { return new_session_wait_; }
inline bool is_awaiting_new_session() const { return awaiting_new_session_; }
inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; }

protected:
@@ -324,7 +324,7 @@ class SSLWrap {
SSLSessionPointer next_sess_;
SSLPointer ssl_;
bool session_callbacks_;
bool new_session_wait_;
bool awaiting_new_session_;

// SSL_set_cert_cb
CertCb cert_cb_;
11 changes: 6 additions & 5 deletions src/node_crypto_bio.h
Original file line number Diff line number Diff line change
@@ -34,8 +34,8 @@ namespace node {
namespace crypto {

// This class represents buffers for OpenSSL I/O, implemented as a singly-linked
// list of chunks. It can be used both for writing data from Node to OpenSSL
// and back, but only one direction per instance.
// list of chunks. It can be used either for writing data from Node to OpenSSL,
// or for reading data back, but not both.
// The structure is only accessed, and owned by, the OpenSSL BIOPointer
// (a.k.a. std::unique_ptr<BIO>).
class NodeBIO : public MemoryRetainer {
@@ -80,11 +80,12 @@ class NodeBIO : public MemoryRetainer {
// Put `len` bytes from `data` into buffer
void Write(const char* data, size_t size);

// Return pointer to internal data and amount of
// contiguous data available for future writes
// Return pointer to contiguous block of reserved data and the size available
// for future writes. Call Commit() once the write is complete.
char* PeekWritable(size_t* size);

// Commit reserved data
// Specify how much data was written into the block returned by
// PeekWritable().
void Commit(size_t size);


3 changes: 3 additions & 0 deletions src/node_crypto_clienthello.h
Original file line number Diff line number Diff line change
@@ -30,6 +30,9 @@
namespace node {
namespace crypto {

// Parse the client hello so we can do async session resumption. OpenSSL's
// session resumption uses synchronous callbacks, see SSL_CTX_sess_set_get_cb
// and get_session_cb.
class ClientHelloParser {
public:
inline ClientHelloParser();
2 changes: 1 addition & 1 deletion src/stream_wrap.cc
Original file line number Diff line number Diff line change
@@ -222,7 +222,7 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) {
type = uv_pipe_pending_type(reinterpret_cast<uv_pipe_t*>(stream()));
}

// We should not be getting this callback if someone as already called
// We should not be getting this callback if someone has already called
// uv_close() on the handle.
CHECK_EQ(persistent().IsEmpty(), false);

60 changes: 43 additions & 17 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
@@ -116,6 +116,11 @@ void TLSWrap::InitSSL() {
#endif // SSL_MODE_RELEASE_BUFFERS

SSL_set_app_data(ssl_.get(), this);
// Using InfoCallback isn't how we are supposed to check handshake progress:
// https://github.com/openssl/openssl/issues/7199#issuecomment-420915993
//
// Note on when this gets called on various openssl versions:
// https://github.com/openssl/openssl/issues/7199#issuecomment-420670544
SSL_set_info_callback(ssl_.get(), SSLInfoCallback);

if (is_server()) {
@@ -194,6 +199,9 @@ void TLSWrap::Start(const FunctionCallbackInfo<Value>& args) {

// Send ClientHello handshake
CHECK(wrap->is_client());
// Seems odd to read when when we want to send, but SSL_read() triggers a
// handshake if a session isn't established, and handshake will cause
// encrypted data to become available for output.
wrap->ClearOut();
wrap->EncOut();
}
@@ -243,7 +251,7 @@ void TLSWrap::EncOut() {
return;

// Wait for `newSession` callback to be invoked
if (is_waiting_new_session())
if (is_awaiting_new_session())
return;

// Split-off queue
@@ -253,7 +261,7 @@ void TLSWrap::EncOut() {
if (ssl_ == nullptr)
return;

// No data to write
// No encrypted output ready to write to the underlying stream.
if (BIO_pending(enc_out_) == 0) {
if (pending_cleartext_input_.empty())
InvokeQueued(0);
@@ -442,13 +450,13 @@ void TLSWrap::ClearOut() {
}


bool TLSWrap::ClearIn() {
void TLSWrap::ClearIn() {
// Ignore cycling data if ClientHello wasn't yet parsed
if (!hello_parser_.IsEnded())
return false;
return;

if (ssl_ == nullptr)
return false;
return;

std::vector<uv_buf_t> buffers;
buffers.swap(pending_cleartext_input_);
@@ -468,8 +476,9 @@ bool TLSWrap::ClearIn() {

// All written
if (i == buffers.size()) {
// We wrote all the buffers, so no writes failed (written < 0 on failure).
CHECK_GE(written, 0);
return true;
return;
}

// Error or partial write
@@ -481,6 +490,8 @@ bool TLSWrap::ClearIn() {
Local<Value> arg = GetSSLError(written, &err, &error_str);
if (!arg.IsEmpty()) {
write_callback_scheduled_ = true;
// XXX(sam) Should forward an error object with .code/.function/.etc, if
// possible.
InvokeQueued(UV_EPROTO, error_str.c_str());
} else {
// Push back the not-yet-written pending buffers into their queue.
@@ -491,7 +502,7 @@ bool TLSWrap::ClearIn() {
buffers.end());
}

return false;
return;
}


@@ -547,6 +558,7 @@ void TLSWrap::ClearError() {
}


// Called by StreamBase::Write() to request async write of clear text into SSL.
int TLSWrap::DoWrite(WriteWrap* w,
uv_buf_t* bufs,
size_t count,
@@ -560,18 +572,26 @@ int TLSWrap::DoWrite(WriteWrap* w,
}

bool empty = true;

// Empty writes should not go through encryption process
size_t i;
for (i = 0; i < count; i++)
for (i = 0; i < count; i++) {
if (bufs[i].len > 0) {
empty = false;
break;
}
}

// We want to trigger a Write() on the underlying stream to drive the stream
// system, but don't want to encrypt empty buffers into a TLS frame, so see
// if we can find something to Write().
// First, call ClearOut(). It does an SSL_read(), which might cause handshake
// or other internal messages to be encrypted. If it does, write them later
// with EncOut().
// If there is still no encrypted output, call Write(bufs) on the underlying
// stream. Since the bufs are empty, it won't actually write non-TLS data
// onto the socket, we just want the side-effects. After, make sure the
// WriteWrap was accepted by the stream, or that we call Done() on it.
if (empty) {
ClearOut();
// However, if there is any data that should be written to the socket,
// the callback should not be invoked immediately
if (BIO_pending(enc_out_) == 0) {
CHECK_NULL(current_empty_write_);
current_empty_write_ = w;
@@ -591,7 +611,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
CHECK_NULL(current_write_);
current_write_ = w;

// Write queued data
// Write encrypted data to underlying stream and call Done().
if (empty) {
EncOut();
return 0;
@@ -610,17 +630,20 @@ int TLSWrap::DoWrite(WriteWrap* w,
if (i != count) {
int err;
Local<Value> arg = GetSSLError(written, &err, &error_);

// If we stopped writing because of an error, it's fatal, discard the data.
if (!arg.IsEmpty()) {
current_write_ = nullptr;
return UV_EPROTO;
}

// Otherwise, save unwritten data so it can be written later by ClearIn().
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
&bufs[i],
&bufs[count]);
}

// Try writing data immediately
// Write any encrypted/handshake output that may be ready.
EncOut();

return 0;
@@ -652,17 +675,20 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
return;
}

// Only client connections can receive data
if (ssl_ == nullptr) {
EmitRead(UV_EPROTO);
return;
}

// Commit read data
// Commit the amount of data actually read into the peeked/allocated buffer
// from the underlying stream.
crypto::NodeBIO* enc_in = crypto::NodeBIO::FromBIO(enc_in_);
enc_in->Commit(nread);

// Parse ClientHello first
// Parse ClientHello first, if we need to. It's only parsed if session event
// listeners are used on the server side. "ended" is the initial state, so
// can mean parsing was never started, or that parsing is finished. Either
// way, ended means we can give the buffered data to SSL.
if (!hello_parser_.IsEnded()) {
size_t avail = 0;
uint8_t* data = reinterpret_cast<uint8_t*>(enc_in->Peek(&avail));
30 changes: 23 additions & 7 deletions src/tls_wrap.h
Original file line number Diff line number Diff line change
@@ -71,7 +71,9 @@ class TLSWrap : public AsyncWrap,
uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle) override;
// Return error_ string or nullptr if it's empty.
const char* Error() const override;
// Reset error_ string to empty. Not related to "clear text".
void ClearError() override;

void NewSessionDoneCb();
@@ -104,11 +106,22 @@ class TLSWrap : public AsyncWrap,

static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
void InitSSL();
void EncOut();
bool ClearIn();
void ClearOut();
// SSL has a "clear" text (unencrypted) side (to/from the node API) and
// encrypted ("enc") text side (to/from the underlying socket/stream).
// On each side data flows "in" or "out" of SSL context.
//
// EncIn() doesn't exist. Encrypted data is pushed from underlying stream into
// enc_in_ via the stream listener's OnStreamAlloc()/OnStreamRead() interface.
void EncOut(); // Write encrypted data from enc_out_ to underlying stream.
void ClearIn(); // SSL_write() clear data "in" to SSL.
void ClearOut(); // SSL_read() clear text "out" from SSL.

// Call Done() on outstanding WriteWrap request.
bool InvokeQueued(int status, const char* error_str = nullptr);

// Drive the SSL state machine by attempting to SSL_read() and SSL_write() to
// it. Transparent handshakes mean SSL_read() might trigger I/O on the
// underlying stream even if there is no clear text to read or write.
inline void Cycle() {
// Prevent recursion
if (++cycle_depth_ > 1)
@@ -117,6 +130,7 @@ class TLSWrap : public AsyncWrap,
for (; cycle_depth_ > 0; cycle_depth_--) {
ClearIn();
ClearOut();
// EncIn() doesn't exist, it happens via stream listener callbacks.
EncOut();
}
}
@@ -138,16 +152,18 @@ class TLSWrap : public AsyncWrap,
static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableSessionCallbacks(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableCertCb(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableTrace(const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableCertCb(const v8::FunctionCallbackInfo<v8::Value>& args);
static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
static int SelectSNIContextCallback(SSL* s, int* ad, void* arg);

crypto::SecureContext* sc_;
BIO* enc_in_ = nullptr;
BIO* enc_out_ = nullptr;
// BIO buffers hold encrypted data.
BIO* enc_in_ = nullptr; // StreamListener fills this for SSL_read().
BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut().
// Waiting for ClearIn() to pass to SSL_write().
std::vector<uv_buf_t> pending_cleartext_input_;
size_t write_size_ = 0;
WriteWrap* current_write_ = nullptr;