From 4ba1501c72416423182c8691c1f01138988058d0 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 2 Nov 2021 18:48:02 +0000 Subject: [PATCH] BUG: Unexpected state while connecting to ... server, part 1 (#916) These BUG messages (discussed and removed in a recent commit 2b6b1bc) exposed several bugs. This change fixes a case where a BUG message was correctly triggered by a Must() violation in Ssl::ServerBio::write(): check failed: buf[0] == 22 exception location: bio.cc(478) The code expectations reflected in that Must() were wrong: Instead of sending ClientHello, OpenSSL may also send a TLS Alert (Level: Fatal, Description: Internal Error), at least. We believe that alert is sent when SslBump configures OpenSSL to negotiate using unsupported ciphers or something like that. This change relaxes ServerBio code expectations, preventing the Must() violation. The Must() violation was causing OpenSSL-related memory leaks. A more comprehensive solution is needed to avoid similar leaks, but this small fix helps in a specific (and a fairly common) case. This is a Measurement Factory project. --- src/ssl/bio.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index 8d01e4426a..c01bb42ac4 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -361,13 +361,15 @@ Ssl::ServerBio::write(const char *buf, int size, BIO *table) } if (!helloBuild && (bumpMode_ == Ssl::bumpPeek || bumpMode_ == Ssl::bumpStare)) { - // buf contains OpenSSL-generated ClientHello. We assume it has a - // complete ClientHello and nothing else, but cannot fully verify - // that quickly. We only verify that buf starts with a v3+ record - // containing ClientHello. + // We have not seen any bytes, so the buffer must start with an + // OpenSSL-generated TLSPlaintext record containing, for example, a + // ClientHello or an alert message. We check these assumptions before we + // substitute that record/message with clientSentHello. + // TODO: Move these checks to where we actually rely on them. + debugs(83, 7, "to-server" << Raw("TLSPlaintext", buf, size).hex()); Must(size >= 2); // enough for version and content_type checks below Must(buf[1] >= 3); // record's version.major; determines buf[0] meaning - Must(buf[0] == 22); // TLSPlaintext.content_type == handshake in v3+ + Must(20 <= buf[0] && buf[0] <= 23); // valid TLSPlaintext.content_type //Hello message is the first message we write to server assert(helloMsg.isEmpty()); -- 2.47.2