Re: [PATCH] Squid host rewrite for intercepted https requests

On 8/06/2012 9:23 p.m., Alexander Komyagin wrote:
> Hello Alex!
>
> On Thu, 2012-06-07 at 17:22 -0600, Alex Rousskov wrote:
>> On 06/07/2012 10:02 AM, Alexander Komyagin wrote:
>>> Hi! I've found that Squid (as of 3.2.0.16) is still overwriting clients
>>> requests to HTTP if they are intercepted:
>>>
>>> src/client_side.cc:prepareTransparentURL():
>>>
>>> snprintf(http->uri, url_sz, "http://%s%s", /*conn->port->protocol,*/
>>> host, url);
>>>
>>> When I want to intercept https traffic this one really breaks the things
>>> down. I have not deepened into the details, but it seems that everything
>>> works fine (simultaneous http/https) when I use conn->port->protocol and
>>> force protocol to http if it is NULL.
>>>
>>> Patch is attached.
>> Hello Alexander,
>>
>> It looks like your patch is changing more than just the lines that
>> format the URL. We made a similar but more compact change for the
>> bump-server-first project:
>>
>>> static void
>>> prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, char *url,
>>> const char *req_hdr)
>>> {
>>> char *host;
>>> char ipbuf[MAX_IPSTRLEN];
>>>
>>> if (*url != '/')
>>> return; /* already in good shape */
>>>
>>> /* BUG: Squid cannot deal with '*' URLs (RFC2616 5.1.2) */
>>> // BUG 2976: Squid only accepts intercepted HTTP.
>>>
>>> if ((host = mime_get_header(req_hdr, "Host")) != NULL) {
>>> int url_sz = strlen(url) + 32 + Config.appendDomainLen +
>>> strlen(host);
>>> http->uri = (char *)xcalloc(url_sz, 1);
>>> - snprintf(http->uri, url_sz, "http://%s%s", /*conn->port->protocol,*/ host, url);
>>> + snprintf(http->uri, url_sz, "%s://%s%s", conn->port->protocol, host, url);
>>> debugs(33, 5, "TRANSPARENT HOST REWRITE: '"<< http->uri<<"'");
>>> } else {
>>> /* Put the local socket IP address as the hostname. */
>>> int url_sz = strlen(url) + 32 + Config.appendDomainLen;
>>> http->uri = (char *)xcalloc(url_sz, 1);
>>> http->getConn()->clientConnection->local.ToHostname(ipbuf,MAX_IPSTRLEN),
>>> - snprintf(http->uri, url_sz, "http://%s:%d%s",
>>> - // http->getConn()->port->protocol,
>>> + snprintf(http->uri, url_sz, "%s://%s:%d%s",
>>> + http->getConn()->port->protocol,
>>> ipbuf, http->getConn()->clientConnection->local.GetPort(), url);
>>> debugs(33, 5, "TRANSPARENT REWRITE: '"<< http->uri<< "'");
>>> }
>>> }
>>
>> Have you seen the port protocol being NULL? It feels like that should
>> not be possible. If it is never NULL, I think the above changes are
>> preferred.
> Nope. Never seen. But the guy who reported *BUG 2976* certainly did. See
> http://www.squid-cache.org/mail-archive/squid-dev/201103/0020.html

As did several other people around that period. Enough to make me add
that workaround to stable releases. It happens whenever Squid is
reconfigured. The "conn->port" is a pointer to the SquidConfig
http(s)_port state which gets erasesd and rebuilt, problem is the object
is reallocated in a different location and the old object being pointed
to by this fixed pointer has its string unset. NOTE: when MemPools is
disabled this is an invalid pointer dereference crash rather than a
silent "NULL".

The easiest proper fix is to copy the http_port protocol= string into
ConnStateData member for use. ie conn->transportProtocol.

The harder one I was working towards is to RefCount the new
AnyP::PortCfg objects at which point they will persist as long as that
ConnStateData needs to use them. Which is tricky since they are
apparently CBDATA which is not compatible with RefCounting.

Amos
Received on Fri Jun 08 2012 - 12:00:23 MDT