Repository navigation
Integer overflow/UB when writing large buffers to TLS streams #38049
Description
Activity
Here's half of a fix that switches from
SSL_writetoSSL_write_ex:diff
diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 398509bc5c..0569aa725c 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -774,20 +774,21 @@ void TLSWrap::ClearOut() { MarkPopErrorOnReturn mark_pop_error_on_return; char out[kClearOutChunkSize]; - int read; + size_t read; + int ok; for (;;) { - read = SSL_read(ssl_.get(), out, sizeof(out)); - Debug(this, "Read %d bytes of cleartext output", read); + ok = SSL_read_ex(ssl_.get(), out, sizeof(out), &read); + Debug(this, "Read %zu bytes of cleartext output", ok ? read : 0); - if (read <= 0) + if (!ok) break; char* current = out; while (read > 0) { - int avail = read; + size_t avail = read; uv_buf_t buf = EmitAlloc(avail); - if (static_cast<int>(buf.len) < avail) + if (buf.len < avail) avail = buf.len; memcpy(buf.base, current, avail); EmitRead(avail, buf); @@ -811,14 +812,11 @@ void TLSWrap::ClearOut() { EmitRead(UV_EOF); } - // We need to check whether an error occurred or the connection was - // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0. - // See node#1642 and SSL_read(3SSL) for details. - if (read <= 0) { + if (!ok) { HandleScope handle_scope(env()->isolate()); int err; - Local<Value> arg = GetSSLError(read, &err, nullptr) + Local<Value> arg = GetSSLError(ok, &err, nullptr) .FromMaybe(Local<Value>()); // Ignore ZERO_RETURN after EOF, it is basically not a error @@ -859,12 +857,13 @@ void TLSWrap::ClearIn() { MarkPopErrorOnReturn mark_pop_error_on_return; NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(data.size()); - int written = SSL_write(ssl_.get(), data.data(), data.size()); - Debug(this, "Writing %zu bytes, written = %d", data.size(), written); - CHECK(written == -1 || written == static_cast<int>(data.size())); + size_t written; + int ok = SSL_write_ex(ssl_.get(), data.data(), data.size(), &written); + Debug(this, "Writing %zu bytes, written = %zu", data.size(), ok ? written : 0); + CHECK(!ok || written == data.size()); // All written - if (written != -1) { + if (ok) { Debug(this, "Successfully wrote all data to SSL"); return; } @@ -875,7 +874,7 @@ void TLSWrap::ClearIn() { int err; std::string error_str; - MaybeLocal<Value> arg = GetSSLError(written, &err, &error_str); + MaybeLocal<Value> arg = GetSSLError(ok, &err, &error_str); if (!arg.IsEmpty()) { Debug(this, "Got SSL error (%d)", err); write_callback_scheduled_ = true; @@ -1008,7 +1007,8 @@ int TLSWrap::DoWrite(WriteWrap* w, AllocatedBuffer data; MarkPopErrorOnReturn mark_pop_error_on_return; - int written = 0; + size_t written; + int ok; // It is common for zero length buffers to be written, // don't copy data if there there is one buffer with data @@ -1027,25 +1027,25 @@ int TLSWrap::DoWrite(WriteWrap* w, } NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length); - written = SSL_write(ssl_.get(), data.data(), length); + ok = SSL_write_ex(ssl_.get(), data.data(), length, &written); } else { // Only one buffer: try to write directly, only store if it fails uv_buf_t* buf = &bufs[nonempty_i]; NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len); - written = SSL_write(ssl_.get(), buf->base, buf->len); + ok = SSL_write_ex(ssl_.get(), buf->base, buf->len, &written); - if (written == -1) { + if (!ok) { data = AllocatedBuffer::AllocateManaged(env(), length); memcpy(data.data(), buf->base, buf->len); } } - CHECK(written == -1 || written == static_cast<int>(length)); - Debug(this, "Writing %zu bytes, written = %d", length, written); + CHECK(!ok || written == length); + Debug(this, "Writing %zu bytes, written = %zu", length, ok ? written : 0); - if (written == -1) { + if (!ok) { int err; - MaybeLocal<Value> arg = GetSSLError(written, &err, &error_); + MaybeLocal<Value> arg = GetSSLError(ok, &err, &error_); // If we stopped writing because of an error, it's fatal, discard the data. if (!arg.IsEmpty()) {
However,
uv_try_writereturns an int that is the number of bytes written (>0) or the error (<0), so there's overflow again there.diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 78d20f912b..42de5f8831 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -342,6 +342,7 @@ int LibuvStreamWrap::DoTryWrite(uv_buf_t** bufs, size_t* count) { uv_buf_t* vbufs = *bufs; size_t vcount = *count; + // FIXME err is an int but the buffers can be larger than INT_MAX. err = uv_try_write(stream(), vbufs, vcount); if (err == UV_ENOSYS || err == UV_EAGAIN) return 0;
Fixes I can think of:
-
Change the return type of
uv_try_writetolong intorssize_t. The unsigned->signed conversion behavior here is implementation-defined: https://github.com/libuv/libuv/blob/285a5ea819035ff777b8b7c6a367f3f5b55d8809/src/unix/stream.c#L1551I'm struggling to find info on how big the buffer passed to
sendmsg(2)orWSASendfor a TCP socket can practically be -- I don't know if there's another limit I'd run into after making this change. -
Slice the buffers into ~1GB chunks, maybe in
LibuvStreamWrap::DoTryWrite()or inTLSWrap::EncOut()-- sort of a refinement to tls: group chunks into TLS segments #27861. -
Make no changes to Node.js and do the slicing in my code instead. (Will do regardless, at least temporarily.)
-
- changed the title
[-]Integer overflow/UB when writing large buffers to SSL streams[/-][+]Integer overflow/UB when writing large buffers to TLS streams[/+]on Apr 3, 2021 - added a commit that references this issue
on Aug 29, 2022 This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
Consistently.
What is the expected behavior?
Either a nice error saying that stream chunks can only be up to
INT_MAX, or for the stream to automatically slice up large chunks.edit the limit isn't
INT_MAXactually ... in the above test case it's around 2,147,483,540 bytes, which I'm guessing is body + about 107 bytes of some overhead that sums toINT_MAX.What do you see instead?
Additional information
The error claims to be from the
writesyscall, but it looks to be in openssl upstream of the syscall:node/deps/openssl/openssl/ssl/ssl_lib.c
Lines 2000 to 2006 in e46c680
For comparison, an
fs.WriteStreamwill properly write files exceeding Linux'swrite(2)limit of 2,147,479,552 bytes.#27861 looks like it would make this possible to hit in other scenarios also, maybe there should be a limit on the concatenated segment size.