Sanitize headers in Proxy Authentication Required responses

net/http/http_proxy_client_socket.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_proxy_client_socket.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 465 matching lines @@
       // attacker can force us into this state by masquerading as the proxy.
       // The only safe thing to do here is to fail the connection because our
       // client is expecting an SSL protected response.
       // See http://crbug.com/7338.
 
     case 302:  // Found / Moved Temporarily
       // Attempt to follow redirects from HTTPS proxies, but only if we can
       // sanitize the response.  This still allows a rogue HTTPS proxy to
       // redirect an HTTPS site load to a similar-looking site, but no longer
       // allows it to impersonate the site the user requested.
-      if (is_https_proxy_ && SanitizeProxyRedirect(&response_, request_.url)) {
+      if (!is_https_proxy_ || !SanitizeProxyRedirect(&response_)) {
+        // We're not using an HTTPS proxy, or we couldn't sanitize the redirect.

[Ryan Sleevi, 2014/12/08 22:04:39]

nit: "Pronouns in Comments considered harmful" -
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...

That said, this comment describes the code, which violates
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio...

(That is, the if conditional says exactly what your plain english comment is
saying). So delete this comment.

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

Done.

+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
+
+      {
         bool is_connection_reused = http_stream_parser_->IsConnectionReused();
         redirect_has_load_timing_info_ =
             transport_->GetLoadTimingInfo(
                 is_connection_reused, &redirect_load_timing_info_);
-        transport_.reset();
-        http_stream_parser_.reset();
-        return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
       }
-
-      // We're not using an HTTPS proxy, or we couldn't sanitize the redirect.
-      LogBlockedTunnelResponse();
-      return ERR_TUNNEL_CONNECTION_FAILED;
+      transport_.reset();
+      http_stream_parser_.reset();
+      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;

[Ryan Hamilton, 2014/12/08 22:51:59]

I think you've tried to make use of an early-return of the error here. Yay! I
love early-return. But I think you need to nuke the {}s at 492/497 and
refrormat?

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

What's wrong with the {}s? They're to prevent "jumping past assignment" errors
from the switch.

[Ryan Hamilton, 2014/12/10 22:09:21]

They look ugly :>
Ugh. I see. In that case, I would suggest one of two approaches:
1) simply inline the call to http_stream_parser_->IsConnectionReused()
2) put the {}s around the entire case 407: block, which I think is more common
than simply putting braces around two lines in the middle of a block.

[Ryan Hamilton, 2014/12/19 21:26:58]

ping.

[Deprecated (see juliatuttle), 2014/12/19 21:50:46]

Done.

 
     case 407:  // Proxy Authentication Required
       // We need this status code to allow proxy authentication.  Our
       // authentication code is smart enough to avoid being tricked by an
       // active network attacker.
       // The next state is intentionally not set as it should be STATE_NONE;
+      if (!SanitizeProxyAuth(&response_)) {
+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
       return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_);
 
     default:
       // Ignore response to avoid letting the proxy impersonate the target
       // server.  (See http://crbug.com/137891.)
       // We lose something by doing this.  We have seen proxy 403, 404, and
       // 501 response bodies that contain a useful error message.  For
       // example, Squid uses a 404 response to report the DNS error: "The
       // domain name does not exist."
       LogBlockedTunnelResponse();
@@ skipping 29 matching lines @@
 
 int HttpProxyClientSocket::DoTCPRestartComplete(int result) {
   if (result != OK)
     return result;
 
   next_state_ = STATE_GENERATE_AUTH_TOKEN;
   return result;
 }
 
 }  // namespace net