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

On 9/06/2012 2:52 a.m., Alex Rousskov wrote:
> On 06/08/2012 06:00 AM, Amos Jeffries wrote:
>> 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.
>
> If they are cbdata, let's just lock/release them properly, without ever
> freeing/deleting them explicitly. My understanding is that the following
> is necessary to fix the underlying problem:
>
> 1. add_http_port() needs to lock the new port pointer before linking it.
> 2. parsePortCfg() needs to lock the new port pointer before linking it.
> 3. free_PortCfg() needs to unlock the old port pointer before unlinking
> it. It should not delete the old pointer.
>
> Anything else?

ConnStateData already seems to set a reference on the details. So yes,
that looks like the quickest fix.

Amos
Received on Sat Jun 09 2012 - 00:48:06 MDT