From d20cf1863de1c4b3e070c1781d091eea1054ce9f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 22 Apr 2016 22:46:12 -0600 Subject: [PATCH] Removed HandshakeParser::parseError and hid/renamed its parseDone. The presence of two "persistent" parsing outcomes (done and error) was confusing to the callers: Which one do I check and when? The adjusted interface uses exceptions for errors and a false parseHandshake() return value to signal "need more data". This simplifies the API and untangles the calling code quite a bit. --- src/client_side.cc | 53 ++++++++++++++++++++++----------------- src/client_side.h | 7 +++--- src/security/Handshake.cc | 33 +++++++++++------------- src/security/Handshake.h | 14 ++++++----- src/ssl/bio.cc | 36 ++++++++++++++++++++------ src/ssl/bio.h | 4 ++- 6 files changed, 88 insertions(+), 59 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index 14d1795dac..917471ed02 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2187,19 +2187,12 @@ void ConnStateData::afterClientRead() { #if USE_OPENSSL - if (atTlsPeek) { - assert(!inBuf.isEmpty()); - if (!tlsParser.parseHello(inBuf)) { - if (!tlsParser.parseError) { - readSomeData(); - return; - } - } - atTlsPeek = false; - afterClientHelloPeeked(); + if (parsingTlsHandshake) { + parseTlsHandshake(); return; } #endif + /* Process next request */ if (pipeline.empty()) fd_note(clientConnection->fd, "Reading next request"); @@ -2432,9 +2425,9 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) : needProxyProtocolHeader_(false), #if USE_OPENSSL switchedToHttps_(false), + parsingTlsHandshake(false), sslServerBump(NULL), signAlgorithm(Ssl::algSignTrusted), - atTlsPeek(false), #endif stoppedSending_(NULL), stoppedReceiving_(NULL) @@ -3150,23 +3143,37 @@ ConnStateData::switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode) // a bumbed "connect" request on non transparent port. receivedFirstByte_ = false; // Get more data to peek at Tls - atTlsPeek = true; + parsingTlsHandshake = true; readSomeData(); } void -ConnStateData::afterClientHelloPeeked() +ConnStateData::parseTlsHandshake() { + Must(parsingTlsHandshake); + + assert(!inBuf.isEmpty()); receivedFirstByte(); + fd_note(clientConnection->fd, "Parsing TLS handshake"); + + bool unsupportedProtocol = false; + try { + if (!tlsParser.parseHello(inBuf)) { + // need more data to finish parsing + readSomeData(); + return; + } + } + catch (const std::exception &ex) { + debugs(83, 2, "error on FD " << clientConnection->fd << ": " << ex.what()); + unsupportedProtocol = true; + } - // Avoid the check for tlsParser error, in the case of bug in our tls parser. - // The bumpServerFirst and bumpClientFirst should not depend on tlsParser - // result. For the peek-and-splice mode, the tlsParser.parseError checked and - // handled inside startPeekAndsSplice method. + parsingTlsHandshake = false; - // Record parsed info - if (!tlsParser.parseError) - clientConnection->tlsNegotiations()->retrieveParsedInfo(tlsParser.details); + // Even if the parser failed, each TLS detail should either be set + // correctly or still be "unknown"; copying unknown detail is a no-op. + clientConnection->tlsNegotiations()->retrieveParsedInfo(tlsParser.details); // We should disable read/write handlers Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0); @@ -3180,7 +3187,7 @@ ConnStateData::afterClientHelloPeeked() FwdState::fwdStart(clientConnection, sslServerBump->entry, sslServerBump->request.getRaw()); } else { Must(sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare); - startPeekAndSplice(); + startPeekAndSplice(unsupportedProtocol); } } @@ -3202,9 +3209,9 @@ ConnStateData::spliceOnError(const err_type err) } -void ConnStateData::startPeekAndSplice() +void ConnStateData::startPeekAndSplice(const bool unsupportedProtocol) { - if (tlsParser.parseError) { + if (unsupportedProtocol) { if (!spliceOnError(ERR_PROTOCOL_UNKNOWN)) clientConnection->close(); return; diff --git a/src/client_side.h b/src/client_side.h index ea83290ac3..34d7e58b99 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -203,7 +203,7 @@ public: void postHttpsAccept(); /// Initializes and starts a peek-and-splice negotiation with the SSL client - void startPeekAndSplice(); + void startPeekAndSplice(const bool unknownProtocol); /// Called when the initialization of peek-and-splice negotiation finidhed void startPeekAndSpliceDone(); /// Called when a peek-and-splice step finished. For example after @@ -235,7 +235,7 @@ public: void sslCrtdHandleReply(const Helper::Reply &reply); void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode); - void afterClientHelloPeeked(); + void parseTlsHandshake(); bool switchedToHttps() const { return switchedToHttps_; } Ssl::ServerBump *serverBump() {return sslServerBump;} inline void setServerBump(Ssl::ServerBump *srvBump) { @@ -352,6 +352,8 @@ private: #if USE_OPENSSL bool switchedToHttps_; + bool parsingTlsHandshake; ///< whether we are getting/parsing TLS Hello bytes + /// The SSL server host name appears in CONNECT request or the server ip address for the intercepted requests String sslConnectHostOrIp; ///< The SSL server host name as passed in the CONNECT request SBuf sslCommonName_; ///< CN name for SSL certificate generation @@ -360,7 +362,6 @@ private: /// HTTPS server cert. fetching state for bump-ssl-server-first Ssl::ServerBump *sslServerBump; Ssl::CertSignAlgorithm signAlgorithm; ///< The signing algorithm to use - bool atTlsPeek; #endif /// the reason why we no longer write the response or nil diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc index fa3cda2067..21ce9ce9f1 100644 --- a/src/security/Handshake.cc +++ b/src/security/Handshake.cc @@ -147,9 +147,8 @@ operator <<(std::ostream &os, const DebugFrame &frame) Security::HandshakeParser::HandshakeParser(): state(atHelloNone), ressumingSession(false), - parseDone(false), - parseError(false), currentContentType(0), + done(nullptr), expectingModernRecords(false) { } @@ -162,7 +161,7 @@ Security::HandshakeParser::parseVersion2Record() details->tlsVersion = record.version; parseVersion2HandshakeMessage(record.fragment); state = atHelloReceived; - parseDone = true; + done = "SSL v2 Hello"; } /// RFC 5246. Appendix E.2. Compatibility with SSL 2.0 @@ -240,12 +239,13 @@ void Security::HandshakeParser::parseChangeCipherCpecMessage() { Must(currentContentType == ContentType::ctChangeCipherSpec); - // we are currently ignoring Change Cipher Spec Protocol messages - // Everything after this message may be is encrypted - // The continuing parsing is pointless, abort here and set parseDone + // We are currently ignoring Change Cipher Spec Protocol messages. skipMessage("ChangeCipherCpec msg"); + + // Everything after the ChangeCipherCpec message may be encrypted. + // Continuing parsing is pointless. Stop here. ressumingSession = true; - parseDone = true; + done = "ChangeCipherCpec"; } void @@ -269,7 +269,7 @@ Security::HandshakeParser::parseHandshakeMessage() Must(state < atHelloReceived); Security::HandshakeParser::parseClientHelloHandshakeMessage(message.body); state = atHelloReceived; - parseDone = true; + done = "ClientHello"; return; case HandshakeType::hskServerHello: Must(state < atHelloReceived); @@ -285,7 +285,7 @@ Security::HandshakeParser::parseHandshakeMessage() Must(state < atHelloDoneReceived); // zero-length state = atHelloDoneReceived; - parseDone = true; + done = "ServerHelloDone"; return; } debugs(83, 5, "ignoring " << @@ -447,20 +447,17 @@ Security::HandshakeParser::parseHello(const SBuf &data) // data contains everything read so far, but we may read more later tkRecords.reinput(data, true); tkRecords.rollback(); - while (!tkRecords.atEnd() && !parseDone) + while (!done) parseRecord(); - debugs(83, 7, "success; done: " << parseDone); - return parseDone; + debugs(83, 7, "success; got: " << done); + // we are done; tkRecords may have leftovers we are not interested in + return true; } catch (const BinaryTokenizer::InsufficientInput &) { debugs(83, 5, "need more data"); - Must(!parseError); + return false; } - catch (const std::exception &ex) { - debugs(83, 2, "parsing error: " << ex.what()); - parseError = true; - } - return false; + return false; // unreached } #if USE_OPENSSL diff --git a/src/security/Handshake.h b/src/security/Handshake.h index ac2d71da4a..4364bf49a0 100644 --- a/src/security/Handshake.h +++ b/src/security/Handshake.h @@ -184,11 +184,13 @@ public: HandshakeParser(); /// Parses the initial sequence of raw bytes sent by the SSL agent. - /// Returns true upon successful completion (HelloDone or Finished received). - /// Otherwise, returns false (and sets parseError to true on errors). + /// Returns true upon successful completion (e.g., got HelloDone). + /// Returns false if more data is needed. + /// Throws on errors. bool parseHello(const SBuf &data); - TlsDetails::Pointer details; + TlsDetails::Pointer details; ///< TLS handshake meta info or nil. + #if USE_OPENSSL Ssl::X509_STACK_Pointer serverCertificates; ///< parsed certificates chain #endif @@ -197,9 +199,6 @@ public: bool ressumingSession; ///< True if this is a resumming session - bool parseDone; ///< The parser finishes its job - bool parseError; ///< Set to tru by parse on parse error. - private: bool isSslv2Record(const SBuf &raw) const; void parseRecord(); @@ -228,6 +227,9 @@ private: #endif unsigned int currentContentType; ///< The current SSL record content type + + const char *done; ///< not nil iff we got what we were looking for + /// concatenated TLSPlaintext.fragments of TLSPlaintext.type SBuf fragments; diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index 09bc5873dc..34faf322d8 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -198,6 +198,22 @@ Ssl::ClientBio::read(char *buf, int size, BIO *table) return -1; } +/* ServerBio */ + +Ssl::ServerBio::ServerBio(const int anFd): + Bio(anFd), + helloMsgSize(0), + helloBuild(false), + allowSplice(false), + allowBump(false), + holdWrite_(false), + record_(false), + parsedHandshake(false), + bumpMode_(bumpNone), + rbufConsumePos(0) +{ +} + void Ssl::ServerBio::stateChanged(const SSL *ssl, int where, int ret) { @@ -214,10 +230,10 @@ Ssl::ServerBio::setClientFeatures(Security::TlsDetails::Pointer const &details, int Ssl::ServerBio::read(char *buf, int size, BIO *table) { - if (!parser_.parseDone && !parser_.parseError) // not done parsing yet - return readAndParse(buf, size, table); - else + if (parsedHandshake) // done parsing TLS Hello return readAndGive(buf, size, table); + else + return readAndParse(buf, size, table); } /// Read and give everything to OpenSSL. @@ -240,7 +256,7 @@ Ssl::ServerBio::readAndGive(char *buf, const int size, BIO *table) } /// Read and give everything to our parser. -/// When/if parsing is done, start giving to OpenSSL. +/// When/if parsing is finished (successfully or not), start giving to OpenSSL. int Ssl::ServerBio::readAndParse(char *buf, const int size, BIO *table) { @@ -248,15 +264,19 @@ Ssl::ServerBio::readAndParse(char *buf, const int size, BIO *table) if (result <= 0) return result; - if (!parser_.parseHello(rbuf)) { - if (!parser_.parseError) { + try { + if (!parser_.parseHello(rbuf)) { + // need more data to finish parsing BIO_set_retry_read(table); return -1; } - debugs(83, DBG_IMPORTANT, "ERROR: Failed to parse SSL Server Hello Message"); // XXX: move to catch{} + parsedHandshake = true; // done parsing (successfully) + } + catch (const std::exception &ex) { + debugs(83, 2, "parsing error on FD " << fd_ << ": " << ex.what()); + parsedHandshake = true; // done parsing (due to an error) } - Must(parser_.parseDone || parser_.parseError); return giveBuffered(buf, size); } diff --git a/src/ssl/bio.h b/src/ssl/bio.h index 4b05235bb5..5df82116e0 100644 --- a/src/ssl/bio.h +++ b/src/ssl/bio.h @@ -111,7 +111,8 @@ private: class ServerBio: public Bio { public: - explicit ServerBio(const int anFd): Bio(anFd), helloMsgSize(0), helloBuild(false), allowSplice(false), allowBump(false), holdWrite_(false), record_(false), bumpMode_(bumpNone), rbufConsumePos(0) {} + explicit ServerBio(const int anFd); + /// The ServerBio version of the Ssl::Bio::stateChanged method virtual void stateChanged(const SSL *ssl, int where, int ret); /// The ServerBio version of the Ssl::Bio::write method @@ -166,6 +167,7 @@ private: bool allowBump; ///< True if the SSL stream can be bumped bool holdWrite_; ///< The write hold state of the bio. bool record_; ///< If true the input data recorded to rbuf for internal use + bool parsedHandshake; ///< whether we are done parsing TLS Hello Ssl::BumpMode bumpMode_; ///< The size of data stored in rbuf which passed to the openSSL -- 2.47.2