From 570861fc55ba38c1092fac1d555111bab4577b49 Mon Sep 17 00:00:00 2001 From: Faidon Liambotis Date: Wed, 23 Dec 2020 01:23:22 +0200 Subject: basehttp: also consider Access when a Server's URI ServerState->Comp() is used by the HTTP methods main loop to check whether a connection can be reused, or whether a new one is needed. Unfortunately, the currently implementation only compares the Host and Port between the ServerState's internal URI, with a new URI. However these are URIs, and therefore Port is 0 when a URI port is not specificied, i.e. in the most common configurations. As a result, a ServerState for http://example.org/... will be reused for URIs of the form https://example.org/..., as both Host (example.org) and Port (0) match. In turn this means that GET requests will happen over port 80, in cleartext, even for those https URLs(!). URI Acquires for an http URI and subsequently for an https one, in the same aptmethod session, do not typically happen with apt as the frontend, as apt opens a new pipe with the "https" aptmethod binary (nowadays a symlink to http), which is why this hasn't been a problem in practice and has eluded detection so far. It does happen in the wild with other frontends (e.g. reprepro), plus is legitimately an odd and surprising behavior on apt's end. Therefore add a comparison for the URI's "Access" (= the scheme) in addition to Host and Port, to ensure that we're not reusing the same state for multiple different schemes. --- methods/basehttp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/methods/basehttp.h b/methods/basehttp.h index 4a83f319c..62c9963ea 100644 --- a/methods/basehttp.h +++ b/methods/basehttp.h @@ -94,7 +94,7 @@ struct ServerState /** \brief Get the headers before the data */ RunHeadersResult RunHeaders(RequestState &Req, const std::string &Uri); - bool Comp(URI Other) const {return Other.Host == ServerName.Host && Other.Port == ServerName.Port;}; + bool Comp(URI Other) const {return Other.Access == ServerName.Access && Other.Host == ServerName.Host && Other.Port == ServerName.Port;}; virtual void Reset(); virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 8d4b3a4fcead0ca534b5d1c5a99ae2a4c95eee21 Mon Sep 17 00:00:00 2001 From: Faidon Liambotis Date: Wed, 23 Dec 2020 01:51:50 +0200 Subject: connect: convert a C-style string to std::string Convert the fixed-size (300) char array "ServStr" to a std::string, and simplify the code by removing snprintfs in the process. While at it, rename to the more aptly named "ServiceNameOrPort" and update the comment to reflect what this variable is meant to be. --- methods/connect.cc | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/methods/connect.cc b/methods/connect.cc index 57dfb6299..bb7fba85d 100644 --- a/methods/connect.cc +++ b/methods/connect.cc @@ -349,12 +349,9 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, { if (ConnectionAllowed(Service, Host) == false) return ResultState::FATAL_ERROR; - // Convert the port name/number - char ServStr[300]; - if (Port != 0) - snprintf(ServStr,sizeof(ServStr),"%i", Port); - else - snprintf(ServStr,sizeof(ServStr),"%s", Service); + + // Used by getaddrinfo(); prefer port if given, else fallback to service + std::string ServiceNameOrPort = Port != 0 ? std::to_string(Port) : Service; /* We used a cached address record.. Yes this is against the spec but the way we have setup our rotating dns suggests that this is more @@ -405,14 +402,14 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, while (1) { int Res; - if ((Res = getaddrinfo(Host.c_str(),ServStr,&Hints,&LastHostAddr)) != 0 || + if ((Res = getaddrinfo(Host.c_str(), ServiceNameOrPort.c_str(), &Hints, &LastHostAddr)) != 0 || LastHostAddr == 0) { if (Res == EAI_NONAME || Res == EAI_SERVICE) { if (DefPort != 0) { - snprintf(ServStr, sizeof(ServStr), "%i", DefPort); + ServiceNameOrPort = std::to_string(DefPort); DefPort = 0; continue; } @@ -431,10 +428,10 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, } if (Res == EAI_SYSTEM) _error->Errno("getaddrinfo", _("System error resolving '%s:%s'"), - Host.c_str(), ServStr); + Host.c_str(), ServiceNameOrPort.c_str()); else _error->Error(_("Something wicked happened resolving '%s:%s' (%i - %s)"), - Host.c_str(), ServStr, Res, gai_strerror(Res)); + Host.c_str(), ServiceNameOrPort.c_str(), Res, gai_strerror(Res)); return ResultState::TRANSIENT_ERROR; } break; @@ -469,7 +466,7 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, return Result; if (_error->PendingError() == true) return ResultState::FATAL_ERROR; - _error->Error(_("Unable to connect to %s:%s:"), Host.c_str(), ServStr); + _error->Error(_("Unable to connect to %s:%s:"), Host.c_str(), ServiceNameOrPort.c_str()); return ResultState::TRANSIENT_ERROR; } /*}}}*/ -- cgit v1.2.3 From 1663774bf309fbd196fd2b9c5c2afdd7a25fd288 Mon Sep 17 00:00:00 2001 From: Faidon Liambotis Date: Wed, 23 Dec 2020 01:54:14 +0200 Subject: connect: use ServiceNameOrPort, not Port, as the cache key The "last connection" cache is currently being stored and looked up on the combination of (LastHost, LastPort). However, these are not what the arguments to getaddrinfo() were on the first try: the call is to getaddrinfo(Host, ServiceNameOrPort, ...), i.e. with the port *or if 0, the service name* (e.g. http). Effectively this means that the connection cache lookup for: https://example.org/... i.e. Host = example.org, Port = 0, Service = http would end up matching the "last" connection of (if existed): https://example.org/... i.e. Host = example.org, Port = 0, Service = https ...and thus performing a TLS request over an (unrelated) port 80 connection. Therefore, an HTTP request, followed up by an (unrelated) HTTPS request to the same server, would always fail. Address this by using as the cache key the ServiceNameOrPort, rather than Port. --- methods/connect.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/methods/connect.cc b/methods/connect.cc index bb7fba85d..d513a4540 100644 --- a/methods/connect.cc +++ b/methods/connect.cc @@ -45,7 +45,7 @@ /*}}}*/ static std::string LastHost; -static int LastPort = 0; +static std::string LastService; static struct addrinfo *LastHostAddr = 0; static struct addrinfo *LastUsed = 0; @@ -356,7 +356,7 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, /* We used a cached address record.. Yes this is against the spec but the way we have setup our rotating dns suggests that this is more sensible */ - if (LastHost != Host || LastPort != Port) + if (LastHost != Host || LastService != ServiceNameOrPort) { Owner->Status(_("Connecting to %s"),Host.c_str()); @@ -438,7 +438,7 @@ static ResultState ConnectToHostname(std::string const &Host, int const Port, } LastHost = Host; - LastPort = Port; + LastService = ServiceNameOrPort; } // When we have an IP rotation stay with the last IP. @@ -483,7 +483,10 @@ ResultState Connect(std::string Host, int Port, const char *Service, if (ConnectionAllowed(Service, Host) == false) return ResultState::FATAL_ERROR; - if(LastHost != Host || LastPort != Port) + // Used by getaddrinfo(); prefer port if given, else fallback to service + std::string ServiceNameOrPort = Port != 0 ? std::to_string(Port) : Service; + + if(LastHost != Host || LastService != ServiceNameOrPort) { SrvRecords.clear(); if (_config->FindB("Acquire::EnableSrvRecords", true) == true) -- cgit v1.2.3