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
