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.
+
+#ifndef COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_
+#define COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_
+
+#include <map>
+
+#include "net/http/http_response_headers.h"

[bengr, 2014/07/02 17:31:01]

add:
namespace net {
class HttpResponseHeaders;
}

[xingx, 2014/07/06 03:18:19]

Done.

+
+namespace data_reduction_proxy {
+
+// There are two fingerprints will be added to Chrome-Proxy header.

[bengr, 2014/07/02 17:31:00]

Remove "There are"

[xingx, 2014/07/06 03:18:19]

Done.

+// One starts with |kTamperDetectFingerprintChromeProxy|, which is the
+// fingerprint for Chrome-Proxy header.
+// The other one starts with |kTamperDetectFingerprint|, which includes
+// all other fingerprints.
+const char kTamperDetectFingerprint[] = "fp=";
+const char kTamperDetectFingerprintChromeProxy[] = "cp=";
+
+// In fingerprint starts with |kTamperDetectFingerprint|, it contains multiple

[bengr, 2014/07/02 17:31:00]

"In" --> "If |kTamperDetectFingerprint| contains

[xingx, 2014/07/06 03:18:19]

Done.

+// fingerprints, each starts with a tag followed by "=" and its fingerprint
+// value. Currently we have 3 of fingerprints and thus 3 tags, defined below.

[bengr, 2014/07/02 17:30:59]

Currently --> Three fingerprints and their respective tags are defined below.

[xingx, 2014/07/06 03:18:19]

Done.

+const char kTamperDetectFingerprintVia[] = "via";

[bengr, 2014/07/02 17:31:00]

Are these needed outside the class? If not, defined them in an anonymous
namespace in the .cc

[xingx, 2014/07/06 03:18:19]

Done.

+const char kTamperDetectFingerprintOther[] = "oh";
+const char kTamperDetectFingerprintContengLength[] = "cl";
+
+// Macro for UMA report.

[bengr, 2014/07/02 17:30:59]

Why is this in the .h?

[xingx, 2014/07/06 03:18:19]

Done.

+// If |is_secure_scheme| is true, report to |HTTPS_histogram|,
+// otherwise report to |HTTP_histogram|.
+// Both's bucket are Carrier IDs |mcc_mnc|.
+// The other histogram counts the total number, |HTTP(S)_histogram| "_Total".
+// which only has one bucket, 0.
+#define UMA_REPORT(is_secure_scheme, HTTP_histogram, HTTPS_histogram, mcc_mnc) \

[bengr, 2014/07/02 17:31:00]

Unless you've seen this pattern in other UMA reporting, I'd name these
http_histogram, and https_histogram.

[xingx, 2014/07/06 03:18:20]

Done.

+  do { \
+    if (is_secure_scheme) { \

[bengr, 2014/07/02 17:31:00]

Is the scheme always https if |is_secure_scheme| is true? If so, rename this
|scheme_is_https|. Otherwise, add a comment that explains the exceptions.

[xingx, 2014/07/06 03:18:19]

Done.

+      UMA_HISTOGRAM_SPARSE_SLOWLY(HTTPS_histogram, mcc_mnc); \
+      UMA_HISTOGRAM_SPARSE_SLOWLY(HTTPS_histogram "_Total", 0); \

[bolian, 2014/07/02 23:47:37]

Should use UMA_HISTOGRAM_COUNTS here.

[xingx, 2014/07/06 03:18:19]

Done.

+    } else { \
+      UMA_HISTOGRAM_SPARSE_SLOWLY(HTTP_histogram, mcc_mnc); \
+      UMA_HISTOGRAM_SPARSE_SLOWLY(HTTP_histogram "_Total", 0); \

[bolian, 2014/07/02 23:47:37]

same here, UMA_HISTOGRAM_COUNTS

[xingx, 2014/07/06 03:18:19]

Done.

+    }\
+  } while (0)
+
+// Utility function, exposed for unittest.
+// Return MD5 value for a given string |input|.
+std::string GetMD5(const std::string& input);

[bengr, 2014/07/02 17:31:00]

I don't think you should expose such a function to the namespace. Can this be a
virtual protected method of the class?

[xingx, 2014/07/06 03:18:19]

Done.

+
+// Utility function, exposed for unittest.
+// Return all the values of a header field |header_name| of the
+// response header |headers|, as a vector.
+std::vector<std::string> GetHeaderValues(

[bengr, 2014/07/02 17:31:00]

Do you really need this? HttpResponseHeaders parses the headers into basically
the same thing.

[xingx, 2014/07/06 03:18:19]

Discussed with you, reason is I need to sort the values.

+    const net::HttpResponseHeaders* headers, const std::string& header_name);
+
+// Utility function, exposed for unittest.
+// Check whether values of a header field |values| contains the Chrome-Proxy
+// header's fingerprint (starts with |kTamperDetectFingerprintChromeProxy|).
+// If there is, return true, and save Chrome-Proxy header's fingerprint to
+// |chrome_proxy_fingerprint|;
+// and save other fingerprints (starts with |kTamperDetectFingerprintOther|)
+// to |other_fingerprints|.
+// Return false if there is no Chrome-Proxy header's fingerprint found.
+bool ContainsTamperDetectFingerprints(std::vector<std::string>& values,

[bengr, 2014/07/02 17:31:00]

Don't use non-const references.

[xingx, 2014/07/06 03:18:19]

The function checks whether there is chrome-proxy fingerprint field, if there
is, remove it and save other values for future usage to avoid parse the header
values twice. Let me know if such design is OK, if not, I'll figure out another
way.

+                                      std::string& chrome_proxy_fingerprint,
+                                      std::string& other_fingerprints);
+
+// The main function for detecting tamper.

[bengr, 2014/07/02 17:31:00]

Fill out comments to the 80-char limit.

[xingx, 2014/07/06 03:18:19]

Done.

+// It takes two parameters as input,
+//   1. a pointer to HttpResponseHeaders,
+//   2. a boolean variable indicates whether the connection
+//      between Chrome and data reduction proxy is on HTTPS or not.
+// For such response, the function checks whether there is a tamper detect
+// request (contains fingerprints) from data reduction proxy, if so, it checks
+// whether there are tampers and report the results to UMA.
+void CheckResponseFingerprint(const net::HttpResponseHeaders*, const bool);
+
+
+
+// The class for detecting tamper.
+// It wraps up the functionalities for tamper detection.
+// For each fingerprint, we need to implement two functions:
+//    * checking function:  returns tamper or not for such fingerprint;
+//                          (function name starts with Check...)
+//    * reporting function: reporting results to corresponding UMA
+//                          when there are tampers detected.
+//                          (function name starts with Report...)
+class DataReductionProxyTamperDetect {
+ public:
+  DataReductionProxyTamperDetect(const net::HttpResponseHeaders*, const bool,
+                                 const unsigned, std::vector<std::string>*);
+  virtual ~DataReductionProxyTamperDetect();
+
+  // For Chrome-Proxy header tamper detection...

[bolian, 2014/07/02 23:47:37]

Let's simplify and reformat the doc of this func and you can apply it to other
similar func doc.
How about 

// Returns true if Chrome-Proxy has been tampered.
bool CheckHeaderChromeProxy(const std::string& received_fingerprint) const;


I think this covers everything you want to say. The named parameter should be
clear what the it is.

[xingx, 2014/07/06 03:18:20]

Done.

+  // Check whether values of data reduction proxy's header Chrome-Proxy
+  // have been tampered or not.
+  // It takes one parameters as input,
+  //   1. fingerprint received from data reduction proxy
+  // Returns true if it has been tampered.
+  bool CheckHeaderChromeProxy(const std::string&);

[bengr, 2014/07/02 17:31:00]

Can this function be const? What about others below?

[xingx, 2014/07/06 03:18:19]

Done.

+
+  // For Via header tamper detection...

[bengr, 2014/07/02 17:31:00]

Use complete sentences in comments.

[xingx, 2014/07/06 03:18:19]

Done.

+  // Check whether there are proxies/middleboxes between Chrome
+  // It takes one parameters as input,
+  //   1. fingerprint received from data reduction proxy
+  // Returns true if there are.
+  bool CheckHeaderVia(const std::string&);

[bengr, 2014/07/02 17:30:59]

What does this function do?

[xingx, 2014/07/06 03:18:19]

Done.

+  // Reporting function for Via header tampering.
+  void ReportHeaderVia();
+
+  // For other headers tamper detection...
+  // Check whether values of a predefined list of headers have been tampered.
+  // It takes one parameters as input,
+  //   1. fingerprint received from data reduction proxy
+  // Returns true if tamper detected for these headers.
+  bool CheckHeaderOtherHeaders(const std::string&);
+  // Reporting function for tampering of values of the list of headers.
+  void ReportHeaderOtherHeaders();
+
+  // For Content-Length tamper detection...
+  // Check whether the Content-Length value is different from what
+  // data reduction proxy sees. This is an indicator that the response body
+  // have been modified.
+  // It takes one parameters as input,
+  //   1. fingerprint received from data reduction proxy
+  // Returns true if different Content-Length value is observed.
+  bool CheckHeaderContentLength(const std::string&);
+  // Reporting function for Content-Length tamper detected.
+  void ReportHeaderContentLength();
+
+
+  // Function calls checking and reporting function for tamper detect.
+  // (i.e., above defined function pairs)
+  // Fingerprint type is specified by fingerprint name |key|
+  // (e.g., |kTamperDetectFingerprintVia|), and fingerprint from
+  // data reduction proxy is |fingerprint|.
+  // call it's corresponding check function as well as report function,
+  void CheckReportFingerprint(const std::string& key,
+                              const std::string& fingerprint);
+
+
+  // Function pointer to checking function.
+  typedef bool (DataReductionProxyTamperDetect::*CheckTamper)(

[bengr, 2014/07/02 17:31:00]

Why do you need function pointers?

[xingx, 2014/07/06 03:18:19]

removed.

+                    const std::string&);
+
+  // Function pointer to reporting function.
+  typedef void (DataReductionProxyTamperDetect::*ReportTamper)();
+
+  // Struct contains a pair of function pointers for one fingerprint:
+  // checking function pointer and one reporting function pointer.
+  struct CheckReportFuncs {
+    CheckTamper check_tamper_func;

[bengr, 2014/07/02 17:31:00]

variable names should not be abbreviated. E.g., this should be
check_tamper_function.

[xingx, 2014/07/06 03:18:19]

Done.

+    ReportTamper report_tamper_func;
+  };
+
+ private:
+  // Response header.
+  const net::HttpResponseHeaders* response_headers;

[bengr, 2014/07/02 17:31:00]

Add a blank line after each variable.

[xingx, 2014/07/06 03:18:19]

Done.

+  // HTTPS or HTTP.
+  const bool is_secure_scheme;
+  // Carrier ID.
+  const unsigned mcc_mnc;
+  // Values for Chrome-Proxy header, with |kTamperDetectFingerprintChromeProxy|
+  // removed. Save it as temporary result so we don't need to parse
+  // Chrome-Proxy header twice.
+  std::vector<std::string>* clean_chrome_proxy_header_values;
+  // The checking function and reporting function pointers map, which maps
+  // a fingerprint name to |CheckReportFuncs| which contains pointers to
+  // corresponding checking function and reporting function.
+  std::map<std::string, CheckReportFuncs> check_report_func_map;
+};
+
+}  // namespace data_reduction_proxy
+#endif  // COMPONENTS_DATA_REDUCTION_PROXY_BROWSER_DATA_REDUCTION_PROXY_TAMPER_DETECT_H_