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

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.

* 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>.

* 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.
  NP: this affects the fake request comment documentation "using
tproxy-provided destination IP and port"

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

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

* 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)

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.

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

  + 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)?

  + Please ensure the pinning is only permitted on bumped requests. And
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.
  * If possible a check to validate that the Host matches the CONNECT
authority-URI would be great.

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

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

Amos
Received on Wed Jul 11 2012 - 12:38:51 MDT