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.

diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc
index 8d01e4426ad1aa245da1537fcf1eb2902226bda2..c01bb42ac48396e446a6d4cf2d6ad62a571cf90f 100644 (file)
--- 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());