Chrome Participated Tamper Detect

components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc

Index: components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc
diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc b/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc
new file mode 100644
index 0000000000000000000000000000000000000000..254316b28b79014f3d97226d9f2d3c57986c59e1
--- /dev/null
+++ b/components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.cc
@@ -0,0 +1,387 @@
+// Copyright 2014 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 "components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h"
+
+#include <algorithm>
+#include <cstring>
+
+#include "base/base64.h"
+#include "base/md5.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "components/data_reduction_proxy/common/data_reduction_proxy_headers.h"
+#include "net/android/network_library.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+
+// Macro for UMA reporting. First reports to either |https_histogram| or

[bolian, 2014/07/19 00:15:24]

Can we rewrite the comment shorter. Reading the comments, I am confused. The
code itself is clearer. 

[xingx, 2014/07/21 18:51:44]

Done.

+// |http_histogram| depending on |scheme_is_https|, with |carrier_id| as
+// bucket, which counts for each carrier, how many responses have been tampered
+// with. Then reports to |http(s)_histogram|_Total, which counts the total
+// number of responses that have been tampered with across carriers.
+#define REPORT_TAMPER_DETECTION_UMA(scheme_is_https, http_histogram, https_histogram, carrier_id) \
+  do { \
+    if (scheme_is_https) { \
+      UMA_HISTOGRAM_SPARSE_SLOWLY(https_histogram, carrier_id); \
+      UMA_HISTOGRAM_COUNTS(https_histogram "_Total", 1); \
+    } else { \
+      UMA_HISTOGRAM_SPARSE_SLOWLY(http_histogram, carrier_id); \
+      UMA_HISTOGRAM_COUNTS(http_histogram "_Total", 1); \
+    }\
+  } while (0)
+
+namespace data_reduction_proxy {
+
+// static
+void DataReductionProxyTamperDetection::CheckResponseFingerprint(

[bolian, 2014/07/19 00:15:24]

can we rename this to DetectAndReport?

[xingx, 2014/07/21 18:51:44]

Done.

+    const net::HttpResponseHeaders* headers,
+    const bool is_secure_scheme) {
+  DCHECK(headers);
+  if (!headers)
+    return;
+
+  // If fingerprint of Chrome-Proxy header is absent, abort tamper detection.

[bengr, 2014/07/18 22:47:54]

If the fingerprint of the Chrome-Proxy

[xingx, 2014/07/21 18:51:45]

Done.

+  std::string chrome_proxy_fingerprint;
+  if (!GetDataReductionProxyActionValue(
+      headers,
+      tamper_detection_fingerprint_names::kFingerprintChromeProxy,

[bolian, 2014/07/19 00:15:24]

why add a new namespace just for this?

[xingx, 2014/07/21 18:51:45]

Done.

+      &chrome_proxy_fingerprint))
+    return;
+
+  // Gets Chrome-Proxy header values.
+  std::vector<std::string> chrome_proxy_header_values =
+      GetHeaderValues(headers, "Chrome-Proxy");
+
+  // Removes Chrome-Proxy header's fingerprint for generating the fingerprint

[bengr, 2014/07/18 22:47:54]

Remove the Chrome-Proxy

[xingx, 2014/07/21 18:51:45]

Done.

+  // of received Chrome-Proxy header later.

[bengr, 2014/07/18 22:47:54]

I don't understand this comment. Do you mean:

// The Chrome-Proxy header's fingerprint is computed over all instructions
within the header except the fingerprint, so remove that fingerprint before
verifying the Chrome-Proxy header's integrity.

[bolian, 2014/07/19 00:15:24]

+1.
Better document why than what.

+  RemoveChromeProxyFingerprint(&chrome_proxy_header_values);
+
+  // Get carrier ID.
+  unsigned carrier_id = 0;
+#if defined(OS_ANDROID)
+  base::StringToUint(net::android::GetTelephonyNetworkOperator(), &carrier_id);
+#endif
+
+  DataReductionProxyTamperDetection tamper_detection(
+      headers,
+      is_secure_scheme,
+      carrier_id,
+      &chrome_proxy_header_values);
+
+  // Checks if Chrome-Proxy header has been tampered with.
+  if (tamper_detection.IsChromeProxyHeaderTampered(chrome_proxy_fingerprint)) {
+    tamper_detection.ReportChromeProxyHeaderTamperedUMA();
+    return;
+  } else {

[bolian, 2014/07/19 00:15:24]

get rid of else. you have returned in 'if'.

[xingx, 2014/07/21 18:51:44]

Done.

+    // Chrome-Proxy header has not been tampered with, reports the number of
+    // responses that other fingerprints will be checked.
+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme,
+        "DataReductionProxy.HTTPSHeaderTamperDetection",
+        "DataReductionProxy.HTTPHeaderTamperDetection",
+        carrier_id);
+  }
+
+  // Checks other fingerprints.
+  const char* fingerprint_names[] = {

[bolian, 2014/07/19 00:15:24]

You don't need this array. Just iterate the name->code map that you already
have.

[xingx, 2014/07/21 18:51:44]

Done. Good point.

+    data_reduction_proxy::tamper_detection_fingerprint_names::

[bengr, 2014/07/18 22:47:54]

Call this kChromeProxyActionFingerprintVia. Don't create a separate namespace
for this.

[xingx, 2014/07/21 18:51:45]

Done.

+        kFingerprintVia,
+    data_reduction_proxy::tamper_detection_fingerprint_names::
+        kFingerprintOtherHeaders,
+    data_reduction_proxy::tamper_detection_fingerprint_names::
+        kFingerprintContentLength
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(fingerprint_names); ++i) {
+    std::string fingerprint;
+    if (!GetDataReductionProxyActionValue(
+        headers, fingerprint_names[i], &fingerprint)) {
+      continue;
+    }
+
+    FingerprintCode fingerprint_code = tamper_detection.

[bengr, 2014/07/18 22:47:54]

move tamper_detection to the next line.

[xingx, 2014/07/21 18:51:45]

Removed.

+        GetFingerprintCode(fingerprint_names[i]);
+    switch (fingerprint_code) {
+    case VIA:
+      if (tamper_detection.IsViaHeaderTampered(fingerprint))
+        tamper_detection.ReportViaHeaderTamperedUMA();
+      break;
+    case OTHERHEADERS:
+      if (tamper_detection.AreOtherHeadersTampered(fingerprint))
+        tamper_detection.ReportOtherHeadersTamperedUMA();
+      break;
+    case CONTENTLENGTH:
+      if (tamper_detection.IsContentLengthHeaderTampered(fingerprint))
+        tamper_detection.ReportContentLengthHeaderTamperedUMA();
+      break;
+    default:
+      NOTREACHED();
+      break;
+    }
+  }
+  return;

[bolian, 2014/07/19 00:15:24]

rm

[xingx, 2014/07/21 18:51:45]

Done.

+}
+
+// Constructor initializes fingerprint name to code map.

[bengr, 2014/07/18 22:47:54]

// Constructor initializes the map of fingerprint name to code.

[xingx, 2014/07/21 18:51:45]

Done.

+DataReductionProxyTamperDetection::DataReductionProxyTamperDetection(
+    const net::HttpResponseHeaders* headers,
+    const bool is_secure,
+    const unsigned carrier_id,
+    std::vector<std::string>* values)
+    : response_headers_(headers),
+      is_secure_scheme_(is_secure),
+      carrier_id_(carrier_id),
+      clean_chrome_proxy_header_values_(values) {
+  DCHECK(headers);
+  fingerprint_name_code_map_ = std::map<std::string, FingerprintCode>();
+  fingerprint_name_code_map_
+      [tamper_detection_fingerprint_names::kFingerprintVia] = VIA;
+  fingerprint_name_code_map_
+      [tamper_detection_fingerprint_names::kFingerprintOtherHeaders] =
+      OTHERHEADERS;
+  fingerprint_name_code_map_
+      [tamper_detection_fingerprint_names::kFingerprintContentLength] =
+      CONTENTLENGTH;
+};
+
+DataReductionProxyTamperDetection::~DataReductionProxyTamperDetection() {};
+
+// Checks whether the Chrome-Proxy header has been tampered with. |fingerprint|
+// is the fingerprint received from the data reduction proxy, which is Base64
+// encoded. Decodes it first. Then calculates the fingerprint of received
+// Chrome-Proxy header, and compares the two to see whether they are equal or
+// not. Note that |clean_chrome_proxy_header_values_| holds the values of
+// Chrome-Proxy header with its own fingerprint removed, so it's the correct
+// values to calculate fingerprint of received Chrome-Proxy header.
+bool DataReductionProxyTamperDetection::IsChromeProxyHeaderTampered(
+    const std::string& fingerprint) const {
+  std::string received_fingerprint;
+  if (!base::Base64Decode(fingerprint, &received_fingerprint))
+    return true;
+  // Calculates the MD5 hash value of Chrome-Proxy.
+  std::string actual_fingerprint = GetMD5(
+      ValuesToSortedString(clean_chrome_proxy_header_values_));
+
+  return received_fingerprint != actual_fingerprint;
+}
+
+void DataReductionProxyTamperDetection::ReportChromeProxyHeaderTamperedUMA()
+    const {
+  REPORT_TAMPER_DETECTION_UMA(
+      is_secure_scheme_,
+      "DataReductionProxy.HTTPSHeaderTampered_ChromeProxy",
+      "DataReductionProxy.HTTPHeaderTampered_ChromeProxy",
+      carrier_id_);
+}
+
+// Checks whether there are other proxies/middleboxes' name after the data
+// reduction proxy's name in Via header.
+bool DataReductionProxyTamperDetection::IsViaHeaderTampered(
+    const std::string& fingerprint) const {
+  bool has_chrome_proxy, has_intermediary;
+  has_chrome_proxy = HasDataReductionProxyViaHeader(response_headers_,
+                                                    &has_intermediary);
+  // Via header of the data reduction proxy is missing.
+  if (!has_chrome_proxy) {

[bolian, 2014/07/19 00:15:24]

I would move this to the report function and return the missing case in a param.
This will also allow you unit test such a case.

[xingx, 2014/07/21 18:51:44]

Done.

+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme_,
+        "DataReductionProxy.HTTPSHeaderTampered_Via_Missing",
+        "DataReductionProxy.HTTPHeaderTampered_Via_Missing",
+        carrier_id_);
+    return false;
+  }
+  return !has_intermediary;
+}
+
+void DataReductionProxyTamperDetection::ReportViaHeaderTamperedUMA() const {
+  REPORT_TAMPER_DETECTION_UMA(is_secure_scheme_,
+             "DataReductionProxy.HTTPSHeaderTampered_Via",
+             "DataReductionProxy.HTTPHeaderTampered_Via",
+             carrier_id_);
+}
+
+// Checks whether values of a predefined list of headers have been modified.
+// The format for |fingerprint| is:

[bengr, 2014/07/18 22:47:53]

I'm confused by this. I would expect an explanation like the following:

We construct a canonical representation of the headers so that reordered headers
will produce the same fingerprint. The fingerprint is constructed as follows:
1) sort...
2) merge into a string with a ':' delimeter.
3) etc

[xingx, 2014/07/21 18:51:45]

Done.

the function to generate "canonical representation of A header" is
ValuesToSortedString.
Here for each header, we need to call ValuesToSortedString to generate a string,
concatenates the string and then calculate hash value on it.

+//     [base64encoded_fingerprint]:header_name1:header_namer2:...

[bengr, 2014/07/18 22:47:54]

Verify that the delimiter is legal according to RFC2616, and that it does not
appear in a base64 encoding, so '+' and '/' are out.

[xingx, 2014/07/21 18:51:44]

Done.

+// Firstly extracts the header names in the |fingerprint|. For each header,
+// generates a string of all the values of such header. Concatenates the
+// strings for all the headers (with delimiter ";") and calculates the MD5 hash
+// value on it. Compares such hash value to the fingerprint received from the
+// data reduction proxy.
+bool DataReductionProxyTamperDetection::AreOtherHeadersTampered(
+    const std::string& fingerprint) const {
+  std::string received_fingerprint;
+  DCHECK(fingerprint.size());
+
+  // ":" delimiter would not occur in base64 as well as header names.
+  net::HttpUtil::ValuesIterator it(fingerprint.begin(),
+                                   fingerprint.end(), ':');
+
+  // The first value from fingerprint is the base64 encoded fingerprint; the
+  // following values are the header names included in fingerprint calculation.
+  // Make sure there is [base64fingerprint] and it can be decoded.
+  if (!(it.GetNext() &&
+        base::Base64Decode(it.value(), &received_fingerprint))) {
+    NOTREACHED();
+    return true;
+  }
+
+  std::string header_values;
+  // Enumerates the list of headers.
+  while (it.GetNext()) {
+    // Gets values of one header.
+    std::vector<std::string> response_header_values =
+        GetHeaderValues(response_headers_, it.value());
+    // Sorts the values and concatenate them, with delimiter ";". ";" would not
+    // occur in header values,
+    header_values += ValuesToSortedString(&response_header_values)  + ";";

[bengr, 2014/07/18 22:47:54]

The end of the list will be a ';'. Is that ok?

[xingx, 2014/07/21 18:51:45]

That's expected, so a non-exist header is different to a header contains an
empty value:
a header contains an empty value: ","
a header does not exist:          ""

+  }
+
+  // Calculates MD5 hash value on the concatenated string.

[bengr, 2014/07/18 22:47:54]

the MD5 hash of

[xingx, 2014/07/21 18:51:45]

Done.

+  std::string actual_fingerprint = GetMD5(header_values);
+
+  return received_fingerprint != actual_fingerprint;
+}
+
+void DataReductionProxyTamperDetection::ReportOtherHeadersTamperedUMA() const {
+  REPORT_TAMPER_DETECTION_UMA(
+      is_secure_scheme_,
+      "DataReductionProxy.HTTPSHeaderTampered_OtherHeaders",
+      "DataReductionProxy.HTTPHeaderTampered_OtherHeaders",
+      carrier_id_);
+}
+
+// Checks whether the Content-Length value is different from what the data
+// reduction proxy sends. Reports it as modified only if Content-Length can be
+// decoded as an integer at both ends and such two numbers are not equal.
+bool DataReductionProxyTamperDetection::IsContentLengthHeaderTampered(
+    const std::string& fingerprint) const {
+  int received_content_length_fingerprint, actual_content_length;
+  // If Content-Length value from data reduction proxy does not exist or it
+  // cannot be converted to an integer, abort.
+  if (base::StringToInt(fingerprint, &received_content_length_fingerprint)) {
+    std::string actual_content_length_string;
+    // If there is no Content-Length header received, abort.
+    if (response_headers_->GetNormalizedHeader("Content-Length",
+        &actual_content_length_string)) {
+      // If the Content-Length value cannot be converted to integer, abort.
+      if (!base::StringToInt(actual_content_length_string,
+                             &actual_content_length)) {
+        return false;
+      }
+
+      return received_content_length_fingerprint != actual_content_length;
+    }
+  }
+  return false;
+}
+
+void DataReductionProxyTamperDetection::ReportContentLengthHeaderTamperedUMA()
+    const {
+  // Gets MIME type of the response and reports to UMA histograms separately.
+  // Divides MIME types into 4 groups: JavaScript, CSS, Images, and others.
+  REPORT_TAMPER_DETECTION_UMA(
+      is_secure_scheme_,
+      "DataReductionProxy.HTTPSHeaderTampered_ContentLength",
+      "DataReductionProxy.HTTPHeaderTampered_ContentLength",
+      carrier_id_);
+
+  // Gets MIME type.
+  std::string mime_type;
+  response_headers_->GetMimeType(&mime_type);
+
+  // Reports tampered JavaScript.
+  if (mime_type.compare("text/javascript") == 0 ||
+      mime_type.compare("application/x-javascript") == 0 ||
+      mime_type.compare("application/javascript") == 0) {
+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme_,
+        "DataReductionProxy.HTTPSHeaderTampered_ContentLength_JS",
+        "DataReductionProxy.HTTPHeaderTampered_ContentLength_JS",
+        carrier_id_);
+  }
+  // Reports tampered CSSs.
+  else if (mime_type.compare("text/css") == 0) {
+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme_,
+        "DataReductionProxy.HTTPSHeaderTampered_ContentLength_CSS",
+        "DataReductionProxy.HTTPHeaderTampered_ContentLength_CSS",
+        carrier_id_);
+  }
+  // Reports tampered images.
+  else if (mime_type.find("image") == 0) {

[bolian, 2014/07/19 00:15:24]

image/

[xingx, 2014/07/21 18:51:45]

Done.

+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme_,
+        "DataReductionProxy.HTTPSHeaderTampered_ContentLength_Image",
+        "DataReductionProxy.HTTPHeaderTampered_ContentLength_Image",
+        carrier_id_);
+  }
+  // Reports tampered other MIME types.
+  else {
+    REPORT_TAMPER_DETECTION_UMA(
+        is_secure_scheme_,
+        "DataReductionProxy.HTTPSHeaderTampered_ContentLength_Other",
+        "DataReductionProxy.HTTPHeaderTampered_ContentLength_Other",
+        carrier_id_);
+  }
+}
+
+DataReductionProxyTamperDetection::FingerprintCode
+    DataReductionProxyTamperDetection::GetFingerprintCode(
+    const std::string& fingerprint_name) {
+  std::map<std::string, FingerprintCode>::iterator it =
+      fingerprint_name_code_map_.find(fingerprint_name);
+
+  return it == fingerprint_name_code_map_.end() ? NONEXIST : it->second;

[bengr, 2014/07/18 22:47:53]

This is confusing. How about:
if (!fingerprint_name_code_map_.end())
  return it->second;
return NONEXIST;

[xingx, 2014/07/21 18:51:45]

Done.

+}
+
+// Removes Chrome-Proxy header's fingerprint (action name
+// |kFingerprintChromeProxy|) from its values vector.
+void DataReductionProxyTamperDetection::RemoveChromeProxyFingerprint(
+    std::vector<std::string>* values) {

[bengr, 2014/07/18 22:47:54]

Check that values is not NULL

[xingx, 2014/07/21 18:51:44]

Done.

+  std::string chrome_proxy_fingerprint_prefix = std::string(
+      tamper_detection_fingerprint_names::kFingerprintChromeProxy) + "=";
+
+  size_t size = values->size();

[bengr, 2014/07/18 22:47:54]

Move values->size() into the loop declaration.

[xingx, 2014/07/21 18:51:44]

Done.

+  for (size_t i = 0; i < size; ++i) {
+    if ((*values)[i].find(chrome_proxy_fingerprint_prefix) == 0) {
+      values->erase(values->begin() + i);
+      break;
+    }
+  }
+}
+
+std::string DataReductionProxyTamperDetection::ValuesToSortedString(
+    std::vector<std::string>* values) {
+  std::string concatenated_values;
+
+  std::sort(values->begin(), values->end());
+  size_t size = values->size();

[bengr, 2014/07/18 22:47:53]

Move values->size() into the loop declaration.

[xingx, 2014/07/21 18:51:45]

Done.

+  for (size_t i = 0; i < size; ++i)

[bengr, 2014/07/18 22:47:54]

add curly brace

[xingx, 2014/07/21 18:51:45]

Done.

+    // Concatenates with delimiter ",".
+    concatenated_values += (*values)[i] + ",";
+  return concatenated_values;
+}
+
+std::string DataReductionProxyTamperDetection::GetMD5(
+    const std::string &input) {
+  base::MD5Digest digest;
+  base::MD5Sum(input.c_str(), input.size(), &digest);
+  return std::string((char*)digest.a, ARRAYSIZE_UNSAFE(digest.a));
+}
+
+std::vector<std::string> DataReductionProxyTamperDetection::GetHeaderValues(
+    const net::HttpResponseHeaders* headers, const std::string& header_name) {
+  std::vector<std::string> values;
+  std::string value;
+  void* iter = NULL;
+  while (headers->EnumerateHeader(&iter, header_name, &value)) {
+    values.push_back(value);
+  }
+  return values;
+}
+
+}  // namespace data_reduction_proxy