Skip to content

Commit

Permalink
Fix ConnStateData::sslServerBump creation and existance checks
Browse files Browse the repository at this point in the history 
 - Currently when client first bumping mode selected at step1
   we are not creating an Ssl::ServerBump object for ConnStateData
   object. But we are creating when we have to serve an error page
   using the client-first bumping mode.
   This patch fixes the ConnStateData::switchToHttps to always builds
   a Ssl::ServerBump mode for all bumping modes including client-first
   mode.

 - Inside ConnStateData::switchToHttps fix the check for
   ConnStateData::sslServerBump. If this member is already set here
   means that we are serving an error using client-first
   bumping mode.

 - Inside Ssl::PeekingPeerConnector replace any check for
   ConnStateData::sslServerBump existance with must clauses. This is
   must be always set when the Ssl::PeekingPeerConnector is called.
  • Loading branch information
@chtsanti
chtsanti committed May 11, 2021
1 parent e917002 commit ab757cf
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 47 deletions.
23 changes: 14 additions & 9 deletions src/client_side.cc
View file
Expand Up @@ -2861,14 +2861,18 @@ ConnStateData::switchToHttps(ClientHttpRequest *http, Ssl::BumpMode bumpServerMo
// but we are now performing the HTTPS handshake traffic
transferProtocol.protocol = AnyP::PROTO_HTTPS;

// If sslServerBump is set, then we have decided to deny CONNECT
// and now want to switch to SSL to send the error to the client
// without even peeking at the origin server certificate.
if (bumpServerMode == Ssl::bumpServerFirst && !sslServerBump) {
request->flags.sslPeek = true;
sslServerBump = new Ssl::ServerBump(http);
} else if (bumpServerMode == Ssl::bumpPeek || bumpServerMode == Ssl::bumpStare) {
request->flags.sslPeek = true;
if (sslServerBump) {
// If sslServerBump is set, then we have decided to deny CONNECT
// and now want to switch to SSL to send the error to the client
// without even peeking at the origin server certificate.
Must(bumpServerMode == Ssl::bumpClientFirst);
Must(sslServerBump->at(XactionStep::tlsBump1));
Must(!sslServerBump->connectedOk());
} else {
Must(Ssl::isBumpMode(bumpServerMode));
if (bumpServerMode == Ssl::bumpServerFirst || bumpServerMode == Ssl::bumpPeek || bumpServerMode == Ssl::bumpStare)
request->flags.sslPeek = true;

sslServerBump = new Ssl::ServerBump(http, nullptr, bumpServerMode);
}

Expand Down Expand Up @@ -2945,7 +2949,8 @@ ConnStateData::parseTlsHandshake()
return;
}

if (!sslServerBump || sslServerBump->act.step1 == Ssl::bumpClientFirst) { // Either means client-first.
Must(sslServerBump);
if (sslServerBump->act.step1 == Ssl::bumpClientFirst) {
getSslContextStart();
return;
} else if (sslServerBump->act.step1 == Ssl::bumpServerFirst) {
Expand Down
2 changes: 1 addition & 1 deletion src/client_side_request.h
View file
Expand Up @@ -200,7 +200,7 @@ class ClientHttpRequest
/// returns raw sslBump mode value
Ssl::BumpMode sslBumpNeed() const { return sslBumpNeed_; }
/// returns true if and only if the request needs to be bumped
bool sslBumpNeeded() const { return sslBumpNeed_ == Ssl::bumpServerFirst || sslBumpNeed_ == Ssl::bumpClientFirst || sslBumpNeed_ == Ssl::bumpBump || sslBumpNeed_ == Ssl::bumpPeek || sslBumpNeed_ == Ssl::bumpStare; }
bool sslBumpNeeded() const { return Ssl::isBumpMode(sslBumpNeed_); }
/// set the sslBumpNeeded state
void sslBumpNeed(Ssl::BumpMode mode);
void sslBumpStart();
Expand Down
74 changes: 37 additions & 37 deletions src/ssl/PeekingPeerConnector.cc
View file
Expand Up @@ -47,9 +47,9 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice()
{
// Mark Step3 of bumping
if (request->clientConnectionManager.valid()) {
if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) {
serverBump->step = XactionStep::tlsBump3;
}
auto serverBump = request->clientConnectionManager->serverBump();
Must(serverBump);
serverBump->step = XactionStep::tlsBump3;
}

handleServerCertificate();
Expand Down Expand Up @@ -169,10 +169,12 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
if (hostName)
SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);

auto serverBump = csd->serverBump();
Must(serverBump);
// XXX: The serverBump step should never be tlsBump1 here however
// for now the serverBump->step is not update correctly when
// bump is decided at step1.
Must(!csd->serverBump() || csd->serverBump()->at(XactionStep::tlsBump1, XactionStep::tlsBump3));
Must(serverBump->at(XactionStep::tlsBump1, XactionStep::tlsBump3));
if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
auto clientSession = fd_table[clientConn->fd].ssl.get();
Must(clientSession);
Expand Down Expand Up @@ -201,13 +203,11 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
setClientSNI(serverSession.get(), sniServer);
}

if (Ssl::ServerBump *serverBump = csd->serverBump()) {
serverBump->attachServerSession(serverSession);
// store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE
if (X509 *peeked_cert = serverBump->serverCert.get()) {
X509_up_ref(peeked_cert);
SSL_set_ex_data(serverSession.get(), ssl_ex_index_ssl_peeked_cert, peeked_cert);
}
serverBump->attachServerSession(serverSession);
// store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE
if (X509 *peeked_cert = serverBump->serverCert.get()) {
X509_up_ref(peeked_cert);
SSL_set_ex_data(serverSession.get(), ssl_ex_index_ssl_peeked_cert, peeked_cert);
}
}

Expand All @@ -221,34 +221,32 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error)
if (!request->clientConnectionManager.valid() || !fd_table[serverConnection()->fd].ssl)
return;

auto serverBump = request->clientConnectionManager->serverBump();
Must(serverBump);
// remember the server certificate from the ErrorDetail object
if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) {
if (!serverBump->serverCert.get()) {
// remember the server certificate from the ErrorDetail object
if (error && error->detail && error->detail->peerCert())
serverBump->serverCert.resetAndLock(error->detail->peerCert());
else {
handleServerCertificate();
}
if (!serverBump->serverCert.get()) {
// remember the server certificate from the ErrorDetail object
if (error && error->detail && error->detail->peerCert())
serverBump->serverCert.resetAndLock(error->detail->peerCert());
else {
handleServerCertificate();
}
}

if (error) {
// For intercepted connections, set the host name to the server
// certificate CN. Otherwise, we just hope that CONNECT is using
// a user-entered address (a host name or a user-entered IP).
const bool isConnectRequest = !request->clientConnectionManager->port->flags.isIntercepted();
if (request->flags.sslPeek && !isConnectRequest) {
if (X509 *srvX509 = serverBump->serverCert.get()) {
if (const char *name = Ssl::CommonHostName(srvX509)) {
request->url.host(name);
debugs(83, 3, "reset request host: " << name);
}
if (error) {
// For intercepted connections, set the host name to the server
// certificate CN. Otherwise, we just hope that CONNECT is using
// a user-entered address (a host name or a user-entered IP).
const bool isConnectRequest = !request->clientConnectionManager->port->flags.isIntercepted();
if (request->flags.sslPeek && !isConnectRequest) {
if (X509 *srvX509 = serverBump->serverCert.get()) {
if (const char *name = Ssl::CommonHostName(srvX509)) {
request->url.host(name);
debugs(83, 3, "reset request host: " << name);
}
}
}
}

if (!error) {
} else {
serverCertificateVerified();
if (splice) {
if (!Comm::IsConnOpen(clientConn)) {
Expand Down Expand Up @@ -369,9 +367,9 @@ Ssl::PeekingPeerConnector::handleServerCertificate()
serverCertificateHandled = true;

// remember the server certificate for later use
if (Ssl::ServerBump *serverBump = csd->serverBump()) {
serverBump->serverCert = std::move(serverCert);
}
auto serverBump = csd->serverBump();
Must(serverBump);
serverBump->serverCert = std::move(serverCert);
}
}

Expand All @@ -380,7 +378,9 @@ Ssl::PeekingPeerConnector::serverCertificateVerified()
{
if (ConnStateData *csd = request->clientConnectionManager.valid()) {
Security::CertPointer serverCert;
if(Ssl::ServerBump *serverBump = csd->serverBump())
auto serverBump = csd->serverBump();
Must(serverBump);
if(serverBump->serverCert.get())
serverCert.resetAndLock(serverBump->serverCert.get());
else {
const int fd = serverConnection()->fd;
Expand Down
9 changes: 9 additions & 0 deletions src/ssl/support.h
View file
Expand Up @@ -150,6 +150,15 @@ inline const char *bumpMode(int bm)
return (0 <= bm && bm < Ssl::bumpEnd) ? Ssl::BumpModeStr.at(bm) : NULL;
}

/**
\ingroup ServerProtocolSSLAPI
\return true if bm is one of the bumping modes
*/
inline bool isBumpMode(int bm)
{
return bm == Ssl::bumpServerFirst || bm == Ssl::bumpClientFirst || bm == Ssl::bumpBump || bm == Ssl::bumpPeek || bm == Ssl::bumpStare;
}

/// certificates indexed by issuer name
typedef std::multimap<SBuf, X509 *> CertsIndexedList;

Expand Down

0 comments on commit ab757cf

Please sign in to comment.