Chrome Participated Tamper Detect

components/data_reduction_proxy/browser/data_reduction_proxy_tamper_detect.h

+// 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.
+
+// This file implements the tamper detection logic, which detects whether
+// there are middleboxes and whether they are tampering with the response
+// which maybe break correct communication and data transfer between the

[bengr, 2014/07/28 21:56:49]

maybe --> may

[xingx1, 2014/07/30 03:44:20]

Done.

+// Chromium client and the data reduction proxy.
+//
+// A high-level description of the tamper detection process works in two steps:

[bengr, 2014/07/28 21:56:50]

Reword:

"At a high level, the tamper detection process works in two steps:"

[xingx1, 2014/07/30 03:44:20]

Done.

+// 1. The data reduction proxy selects a fraction of responses to analyze;

[bengr, 2014/07/28 21:56:51]

1.  The data reduction proxy selects a fraction of responses to analyze,
generates a series of fingerprints for each, and appends them to Chrome-Proxy
response headers.

2. The client re-generate the fingerprints using the same method as the proxy,
compares them to the fingerprints in the response, and generates UMA. A response
is considered to have been tampered with if the fingerprints do not match.

[xingx1, 2014/07/30 03:44:21]

Done.

+//    for the selected ones, the data reduction proxy generates a series of
+//    fingerprints for the responses, and appends them to the Chrome-Proxy
+//    header;
+// 2. At the Chromium client side, when the Chromium client sees such
+//    fingerprints, it uses the same method as the data reduction proxy to
+//    re-generate the fingerprints, and compares them to the fingerprints in
+//    the response, to see if the response has been tampered with and report to
+//    UMA.
+//
+// Four fingerprints are generated at the data reduction proxy side:

[bengr, 2014/07/28 21:56:49]

Four fingerprints are generated by the proxy:

[xingx1, 2014/07/30 03:44:21]

Done.

+// 1. Fingerprint of the Chrome-Proxy header, which is designed to check
+//    whether the Chrome-Proxy header has been modified or not;
+// 2. Fingerprint of the Via header, which is designed to check whether there
+//    are middleboxes between the Chromium client and the data reduction proxy;
+// 3. Fingerprint of a list of headers, which is designed to check whether the
+//    values of a list of headers (list is defined by the data reduction proxy)
+//    have been modified or deleted;
+// 4. Fingerprint of the Content-Length header, which is designed to check
+//    whether the response body has been modified or not (assume that different

[bengr, 2014/07/28 21:56:49]

assume --> the code assumes
value indicates --> values indicate
body -> bodies

[xingx1, 2014/07/30 03:44:20]

Done.

+//    Content-Length value indicates different response body).
+//
+// At the Chromium client side, the fingerprint of the Chrome-Proxy header will

[bengr, 2014/07/28 21:56:51]

At the Chromium client side -> On the client

[xingx1, 2014/07/30 03:44:20]

Done.

+// be checked first. If the fingerprint indicates that the Chrome-Proxy header
+// has not been modified, then the other fingerprints will be considered to be
+// reliable and will be checked next; if not, then it's possible that the other
+// fingerprints have been tampered with and thus they will not be checked.
+//
+// Detected tampering information will be reported to UMA. In general, for each

[bengr, 2014/07/28 21:56:50]

Do you also report when response have not been tampered with? That would be
useful as a baseline.

[xingx1, 2014/07/30 03:44:20]

Right now, no. But we have total number of "checked" responses, so we know the
number of responses that have not been tampered with.

+// fingerprint, the Chromium client reports the number of responses that have

[bengr, 2014/07/28 21:56:49]

Chromium client -> client

[xingx1, 2014/07/30 03:44:19]

Done.

+// been tampered with for different carriers. For the fingerprint of the
+// Content-Length header, which indicates whether the response body has been
+// modified or not, the reports of tampering are separated by MIME type of the
+// response body.
+
+#ifndef COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECTION_H_
+#define COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECTION_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "net/proxy/proxy_service.h"
+
+namespace net {
+class HttpResponseHeaders;
+}
+
+namespace data_reduction_proxy {
+
+// This class detects if the response sent by the data reduction proxy has been

[bengr, 2014/07/28 21:56:51]

This class detects -> Detects
the intermediaries -> intermediaries

[xingx1, 2014/07/30 03:44:20]

Done.

+// modified by the intermediaries on the Web.
+class DataReductionProxyTamperDetection {
+ public:
+  // Checks if the response contains tamper detection fingerprints added by the
+  // data reduction proxy, and determines if the response had been tampered
+  // with if so. Results are reported to UMA. HTTP and HTTPS traffic are
+  // reported separately, specified by |is_secure_scheme|. Returns true if
+  // the response has been tampered with.
+  static bool DetectAndReport(const net::HttpResponseHeaders* header,

[bengr, 2014/07/28 21:56:51]

header -> headers

[xingx1, 2014/07/30 03:44:19]

Done.

+                                       bool is_secure_scheme);

[bengr, 2014/07/28 21:56:49]

broken identation. This should be aligned with the "const" above. 

[xingx1, 2014/07/30 03:44:21]

Done.

+
+  // Tamper detection checks |response_headers|. Histogram events are reported
+  // by |carrier_id|; |is_secure_scheme| determines which histogram to report
+  // (HTTP and HTTPS are reported separately). |chrome_proxy_header_values|
+  // points to the vector contains the values of the Chrome-Proxy header, but

[bengr, 2014/07/28 21:56:50]

contains -> containing

[xingx1, 2014/07/30 03:44:20]

Done.

+  // with the Chrome-Proxy header's fingerprint removed, which is a temporary
+  // result saved to use later to avoid parsing the header again.
+  DataReductionProxyTamperDetection(
+      const net::HttpResponseHeaders* response_headers,
+      bool is_secure_scheme,
+      unsigned carrier_id,

[bengr, 2014/07/28 21:56:50]

Explain what a carrier id is.

[xingx1, 2014/07/30 03:44:20]

Done.

+      std::vector<std::string>* chrome_proxy_header_values);
+
+  virtual ~DataReductionProxyTamperDetection();

[bengr, 2014/07/28 21:56:50]

Constructors and destructors should come before other public methods.

+
+ private:
+  friend class DataReductionProxyTamperDetectionTest;

[bengr, 2014/07/28 21:56:50]

Why do you need this? Why isn't friending the specific tests enough?

[xingx1, 2014/07/30 03:44:20]

Previously I have a common function for testing all fingerprints-checking
functions. So I need to "friend" that function.

Right now I removed that function, each fingerprint checking unittest becomes
longer (since I removed common checking function).

Done.

+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           TestFingerprintCommon);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           ChromeProxy);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           Via);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           OtherHeaders);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           ContentLength);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           HeaderRemoving);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           ValuesToSortedString);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           GetHeaderValues);
+  FRIEND_TEST_ALL_PREFIXES(DataReductionProxyTamperDetectionTest,
+                           Completed);
+
+  // Enum for fingerprint type.
+  enum FingerprintCode {
+      CHROMEPROXY   = 1, /* Code of fingerprint of the Chrome-Proxy header */

[bengr, 2014/07/28 21:56:51]

prefix all of these so there is no confusion. E.g., FINGERPRINT_CHROMEPROXY.
Also remove "Code of fingerprint of" from all comments.

[xingx1, 2014/07/30 03:44:20]

Done.

+      VIA           = 2, /* Code of fingerprint of the Via header */
+      OTHERHEADERS  = 3, /* Code of fingerprint of a list of headers */
+      CONTENTLENGTH = 4, /* Code of fingerprint of the Content-Length header */
+      NONEXIST      = 5,

[bengr, 2014/07/28 21:56:50]

What does NONEEXIST mean?  Also, rename these as FINGERPRINT_CHROME_PROXY,
FINGERPRINT_VIA, FINGERPRINT_OTHER_HEADERS, FINGERPRINT_CONTENT_LENGTH, and
FINGERPRINT_NONE_EXIST.

[xingx1, 2014/07/30 03:44:21]

Done.

+  };
+
+  // Returns true if the Chrome-Proxy header has been tampered with.

[bengr, 2014/07/28 21:56:50]

Change the comment here and below if you change the name. It should be something
like:

Returns the result of validating the Chrome-Proxy header.

[xingx1, 2014/07/30 03:44:19]

Done.

+  bool IsChromeProxyHeaderTampered(const std::string& fingerprint) const;

[bengr, 2014/07/28 21:56:50]

This function and all similar ones need new names. One option is to have a
function like:

TamperDetectionResult ValidateChromeProxyHeader();

This is better if you expect returns to have more than two values. E.g., the
return can be a status and you could pass in a pointer to write more information
for deteected tampering.

[xingx1, 2014/07/30 03:44:19]

Done.

+  // Reports UMA for tampering of the Chrome-Proxy header.
+  void ReportChromeProxyHeaderTamperedUMA() const;

[bengr, 2014/07/28 21:56:50]

rename as ReportUMAforChromeProxyHeaderValidation(); Do the same below.

[xingx1, 2014/07/30 03:44:19]

Done.

+
+  // Returns true if the Via header has been tampered with. |has_chrome_proxy|
+  // indicates that the data reduction proxy's Via header occurs or not.
+  bool IsViaHeaderTampered(const std::string& fingerprint,
+                           bool* has_chrome_proxy_via_header) const;
+  // Reports UMA for tampering of the Via header.
+  void ReportViaHeaderTamperedUMA(bool has_chrome_proxy_via_header) const;
+
+  // Returns true if a list of headers have been tampered with.
+  bool AreOtherHeadersTampered(const std::string& fingerprint) const;
+  // Reports UMA for tampering of values of the list of headers.
+  void ReportOtherHeadersTamperedUMA() const;
+
+  // Returns true if the Content-Length header has been tampered with.
+  bool IsContentLengthHeaderTampered(const std::string& fingerprint) const;
+  // Reports UMA for tampering of the Content-Length header.
+  void ReportContentLengthHeaderTamperedUMA() const;
+
+  // Returns the fingerprint code (enum) for the given fingerprint name.
+  FingerprintCode GetFingerprintCode(const std::string& fingerprint_name);
+
+  // Removes the fingerprint of the Chrome-Proxy header from the Chrome-Proxy
+  // header's |values| vector. The data reduction proxy calculates the
+  // fingerprint for the Chrome-Proxy header and then appends calculated
+  // fingerprint to the Chrome-Proxy header, so at the Chromium client side,

[bengr, 2014/07/28 21:56:49]

at the Chromium client --> on the client

[xingx1, 2014/07/30 03:44:20]

Done.

+  // to re-generate the fingerprint, the Chrome-Proxy header's fingerprint value
+  // needs to be removed from the Chrome-Proxy header first.
+  static void RemoveChromeProxyFingerprint(std::vector<std::string>* values);

[bengr, 2014/07/28 21:56:51]

can this method be const?

[xingx1, 2014/07/30 03:44:21]

Acknowledged.

+
+  // Returns a string representation of |values|.
+  static std::string ValuesToSortedString(std::vector<std::string>* values);

[bengr, 2014/07/28 21:56:50]

can this method be const?

[xingx1, 2014/07/30 03:44:20]

Acknowledged.

+
+  // Returns raw MD5 hash value for a given string |input|. It is different to
+  // base::MD5String which is base16 encoded.
+  static std::string GetMD5(const std::string& input);

[bengr, 2014/07/28 21:56:50]

can this method be const?

[xingx1, 2014/07/30 03:44:21]

Acknowledged.

+
+  // Returns all the values of |header_name| of the response |headers| as a
+  // vector. This function is used for values that need to be sorted later.
+  static std::vector<std::string> GetHeaderValues(
+      const net::HttpResponseHeaders* headers,
+      const std::string& header_name);
+
+  // Pointer to response headers.
+  const net::HttpResponseHeaders* response_headers_;
+
+  // If true, the connection to the data reduction proxy is over HTTPS;
+  const bool is_secure_scheme_;

[bengr, 2014/07/28 21:56:51]

can you just be more explicit and rename this scheme_is_https_ ?

[xingx1, 2014/07/30 03:44:19]

Done.

+
+  // Carrier ID.
+  const unsigned carrier_id_;
+
+  // Values of the Chrome-Proxy header, with fingerprint of the Chrome-Proxy
+  // header value removed. Save it as a temporary result to avoid parsing the

[bengr, 2014/07/28 21:56:49]

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

The memoized Chrome-Proxy header with the fingerprint of the Chrome-Proxy header
removed. 

[xingx1, 2014/07/30 03:44:19]

Exactly！ Done.

+  // Chrome-Proxy header again.
+  std::vector<std::string>* clean_chrome_proxy_header_values_;
+
+  // Maps a fingerprint name (string) to a fingerprint code (enum).

[bengr, 2014/07/28 21:56:50]

Why? What uses this?

[xingx1, 2014/07/30 03:44:20]

Removed. Right now using three IF instead of SWITCH.

+  std::map<std::string, FingerprintCode> fingerprint_name_code_map_;
+};
+

[bengr, 2014/07/28 21:56:49]

DISALLOW_COPY_AND_ASSIGN

[xingx1, 2014/07/30 03:44:20]

Done.

+}  // namespace data_reduction_proxy
+#endif  // COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECTION_H_