Re: [PATCH] Support bump-ssl-server-first and mimic SSL server certificates

This is a new version of the patch.
Not all of Amos comments handled please see my comments bellow.

On 07/11/2012 03:38 PM, Amos Jeffries wrote:
> On 11/07/2012 4:31 p.m., Alex Rousskov wrote:
>> Last call! Will merge into trunk tomorrow unless I hear otherwise.
>>
>> Thank you,
>>
>> Alex.
>
> Sorry. Kept forgetting to check thus when I was near a machine which
> could download and decrompress attachments.
>
> A quick check brings up a few things. I don't have time to do a proper
> deep check of this sized patch.
>
>
> cache_cf.cc
> * "ssl-bump on https_port configuration requires either tproxy or
> intercepted"
> --> "tproxy or intercept" (no 'ed'). This also applies to the debugs()
> message.
>

fixed.

> * why is is FATAL: error to have old ssl_bump allow/deny values in the
> config?
> can we auto-convert the "allow" to client-first and deny to bump-none
> with very loud ERROR: messages instead? we can calculate what the
> implicit negate would have been and add it explicitly with another loud
> warning
>

*

>
> src/cf.data.pre:
> * Please use "IF " instead of "IFDEF " for the new inline #if macro. It
> is confusing to tell whether any particular one should be IFDEF<space>
> or IFDEF<colon>.

I made this change but I have my doubts.
The IFDEF is a reserved word which already used by cf_gen parser so it
doesn't hurt to use it in a new form. The "if" is a valid statement of
squid configuration file.

>
> * Why does https_port ssl-bump depend on tproxy mode? (this contradicts
> the "either tproxy or intercepted" messages noted above)
> The destination IP:port is also available in the same place(s) on
> intercept mode from 3.2 onward.

OK fixed in documentation

> NP: this affects the fake request comment documentation "using
> tproxy-provided destination IP and port"
the comment fixed.

>
> src/client_side.h:
> * setServerBump(Ssl::ServerBump *srvBump) has no else condition
> reporting or handling when setting fails.

It should not called if the bump mode already decided. Also we have
other debug messages in other places to inform users for this
bug/problem. To print the message in setServerBump we should convert
this method to a non inline method. Does it worth?

>
> src/client_side_request.cc:
> * in doCallouts, instead of adding repeated tests of
> !calloutContext->error. Can you please wrap if(!calloutContext->error) {
> ... } around the whole section of callouts

done

>
> * Why are error pages delayed until after bumping? have you investigated
> how this interacts with deny_info redirection to 4xx and 5xx pages?
> (particularly 403 and 511 "web login required" splash pages)

I do not thing there is a problem here but we should make tests.

>
> src/errorpage.cc:
> * ErrorState::detailError(int detailCode) implementation looks like a
> GCC "warning: parameter shadows member" in the making.
> Please make that an inline function and rename parameter detailCode to
> dCode or something.

fixed

>
> src/forward.cc:
> * It seems that selectPeerForIntercepted() is permitting pinned
> destinations to pass-thru when Host header is non-validated.
> Malicious intercepted clients now only need to send www-auth
> credentials for a connection-auth scheme (triggering pinning) to be able
> to make poisoning attacks on any followup pipelined request.
> eg:
> GET / HTTP/1.1
> Host: cahoots.server
> WWW-authenticate: NTLM fake
> \r\n
> GET /poisoned-URI/ HTTP/1.1
> Host: victim.site

Inside selectPeerForIntercept there is the call:
 client->validatePinnedConnection
Which checks if the host header is the correct one and if it is not
unpins the connection.

>
> + I have not had time to investigate this re-pinning mechanism that has
> been added. Is there potential for the malicious client and cahoot
> server to trigger it and change to an unvalidated destination? or does
> it simply re-open the Comm::Connection (same destination IP)?

We are connecting to the server ip address the intercepted client
connection destined.
We are not checking any host header or URL or anything....

>
> + Please ensure the pinning is only permitted on bumped requests. And

My opinion is that the code is correct as is. Also my sense, without
making tests, is that this patch FIXES the problem you are described
above because it adds the check that the pinned connection is valid for
intercepted requests which did not existed before.

> I'm not certain they are completely safe either given that we already
> have bug reports indicating domains A and B being sent through a CONNECT
> to server for A.

SSL-bump on normal http-port work with a different way. If there are
bugs they should be fixed, but I do not believe that we have such bug is
this patch...

> * If possible a check to validate that the Host matches the CONNECT
> authority-URI would be great.
Where? If you mean inside selectPeerForIntercepted as I said already
exist...

>
> * "TODO: move into a method before merge" - please enact or remove TODO.

I fixed ... the comment, I removed the "before merge" :-)

>
>
> src/url.cc:
> * this breaks the canonical URI "cache" when URL parsing is changing the
> request URL pieces. Please revert.

This part of the patch removed but just adding a comment was enough....

>
> Amos
>
>