[Backport] CVE-2019-13755: Insufficient policy enforcement in extensions.

Manual backport of patch:
Content Verification: Make computed hashes aware of dot/space
suffix treatment.

On windows, filename with (.| )+ suffix is ignored, i.e. "foo.html."
and "foo.html" are treated the same.

VerifiedContents is already aware of this and it stores a
canonicalized version of filename for filenames containing (.| )+
suffix.
This CL makes ComputedHashes aware of the change too, so that searching
for hashes will consider canonicalized version of the filename as
candidate. This makes ComputedHashes::Reader and VerifiedContents treat
this suffix behavior consistently.

This CL also adds unittest and browsertest for the fix.

Bug: 696208
Test: See bug for test repro.
Change-Id: I98e3f851b2f4cf3cb9cdb4a49f5414c476e2d5bd
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

5 files changed, 93 insertions, 33 deletions

diff --git a/chromium/extensions/browser/BUILD.gn b/chromium/extensions/browser/BUILD.gn
index 6ec2a9925e0..a440fa6c741 100644
--- a/chromium/extensions/browser/BUILD.gn
+++ b/chromium/extensions/browser/BUILD.gn
@@ -86,6 +86,8 @@ jumbo_source_set("browser_sources") {
     "content_verifier/content_hash.cc",
     "content_verifier/content_hash.h",
     "content_verifier/content_verifier_key.h",
+    "content_verifier/content_verifier_utils.cc",
+    "content_verifier/content_verifier_utils.h",
     "content_verifier_delegate.h",
     "content_verifier_io_data.cc",
     "content_verifier_io_data.h",
diff --git a/chromium/extensions/browser/computed_hashes.cc b/chromium/extensions/browser/computed_hashes.cc
index c99e990f179..b159e1df796 100644
--- a/chromium/extensions/browser/computed_hashes.cc
+++ b/chromium/extensions/browser/computed_hashes.cc
@@ -16,8 +16,10 @@
 #include "base/stl_util.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
+#include "extensions/browser/content_verifier/content_verifier_utils.h"
 
 namespace extensions {
 
@@ -148,24 +150,43 @@ bool ComputedHashes::Reader::GetHashes(const base::FilePath& relative_path,
                                        int* block_size,
                                        std::vector<std::string>* hashes) const {
   base::FilePath path = relative_path.NormalizePathSeparatorsTo('/');
-  auto i = data_.find(path);
+  auto find_data = [&](const base::FilePath& normalized_path) {
+    auto i = data_.find(normalized_path);
+    if (i == data_.end()) {
+      // If we didn't find the entry using exact match, it's possible the
+      // developer is using a path with some letters in the incorrect case,
+      // which happens to work on windows/osx. So try doing a linear scan to
+      // look for a case-insensitive match. In practice most extensions don't
+      // have that big a list of files so the performance penalty is probably
+      // not too big here. Also for crbug.com/29941 we plan to start warning
+      // developers when they are making this mistake, since their extension
+      // will be broken on linux/chromeos.
+      for (i = data_.begin(); i != data_.end(); ++i) {
+        const base::FilePath& entry = i->first;
+        if (base::FilePath::CompareEqualIgnoreCase(entry.value(),
+                                                   normalized_path.value())) {
+          break;
+        }
+      }
+    }
+    return i;
+  };
+  auto i = find_data(path);
+#if defined(OS_WIN)
   if (i == data_.end()) {
-    // If we didn't find the entry using exact match, it's possible the
-    // developer is using a path with some letters in the incorrect case, which
-    // happens to work on windows/osx. So try doing a linear scan to look for a
-    // case-insensitive match. In practice most extensions don't have that big
-    // a list of files so the performance penalty is probably not too big
-    // here. Also for crbug.com/29941 we plan to start warning developers when
-    // they are making this mistake, since their extension will be broken on
-    // linux/chromeos.
-    for (i = data_.begin(); i != data_.end(); ++i) {
-      const base::FilePath& entry = i->first;
-      if (base::FilePath::CompareEqualIgnoreCase(entry.value(), path.value()))
-        break;
+    base::FilePath::StringType trimmed_path_value;
+    // Also search for path with (.| )+ suffix trimmed as they are ignored in
+    // windows. This matches the canonicalization behavior of
+    // VerifiedContents::Create.
+    if (content_verifier_utils::TrimDotSpaceSuffix(path.value(),
+                                                   &trimmed_path_value)) {
+      i = find_data(base::FilePath(trimmed_path_value));
     }
-    if (i == data_.end())
-      return false;
   }
+#endif  // defined(OS_WIN)
+  if (i == data_.end())
+    return false;
+
   const HashInfo& info = i->second;
   *block_size = info.first;
   *hashes = info.second;
diff --git a/chromium/extensions/browser/content_verifier/content_verifier_utils.cc b/chromium/extensions/browser/content_verifier/content_verifier_utils.cc
new file mode 100644
index 00000000000..a9c3797aa28
--- /dev/null
+++ b/chromium/extensions/browser/content_verifier/content_verifier_utils.cc
@@ -0,0 +1,24 @@
+// Copyright 2019 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 "extensions/browser/content_verifier/content_verifier_utils.h"
+
+namespace extensions {
+namespace content_verifier_utils {
+
+#if defined(OS_WIN)
+bool TrimDotSpaceSuffix(const base::FilePath::StringType& path,
+                        base::FilePath::StringType* out_path) {
+  base::FilePath::StringType::size_type trim_pos =
+      path.find_last_not_of(FILE_PATH_LITERAL(". "));
+  if (trim_pos == base::FilePath::StringType::npos)
+    return false;
+
+  *out_path = path.substr(0, trim_pos + 1);
+  return true;
+}
+#endif  // defined(OS_WIN)
+
+}  // namespace content_verifier_utils
+}  // namespace extensions
diff --git a/chromium/extensions/browser/content_verifier/content_verifier_utils.h b/chromium/extensions/browser/content_verifier/content_verifier_utils.h
new file mode 100644
index 00000000000..bcfaebf8e29
--- /dev/null
+++ b/chromium/extensions/browser/content_verifier/content_verifier_utils.h
@@ -0,0 +1,23 @@
+// Copyright 2019 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 EXTENSIONS_BROWSER_CONTENT_VERIFIER_CONTENT_VERIFIER_UTILS_H_
+#define EXTENSIONS_BROWSER_CONTENT_VERIFIER_CONTENT_VERIFIER_UTILS_H_
+
+#include "base/files/file_path.h"
+#include "build/build_config.h"
+
+namespace extensions {
+namespace content_verifier_utils {
+
+#if defined(OS_WIN)
+// Returns true if |path| ends with (.| )+.
+// |out_path| will contain "." and/or " " suffix removed from |path|.
+bool TrimDotSpaceSuffix(const base::FilePath::StringType& path,
+                        base::FilePath::StringType* out_path);
+#endif  // defined(OS_WIN)
+
+}  // namespace content_verifier_utils
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_CONTENT_VERIFIER_CONTENT_VERIFIER_UTILS_H_
diff --git a/chromium/extensions/browser/verified_contents.cc b/chromium/extensions/browser/verified_contents.cc
index 8beeb151be5..cef5277bb25 100644
--- a/chromium/extensions/browser/verified_contents.cc
+++ b/chromium/extensions/browser/verified_contents.cc
@@ -18,6 +18,7 @@
 #include "build/build_config.h"
 #include "components/crx_file/id_util.h"
 #include "crypto/signature_verifier.h"
+#include "extensions/browser/content_verifier/content_verifier_utils.h"
 #include "extensions/common/extension.h"
 
 namespace {
@@ -94,21 +95,6 @@ class ScopedUMARecorder {
   DISALLOW_COPY_AND_ASSIGN(ScopedUMARecorder);
 };
 
-#if defined(OS_WIN)
-// Returns true if |path| ends with (.| )+.
-// |out_path| will contain "." and/or " " suffix removed from |path|.
-bool TrimDotSpaceSuffix(const base::FilePath::StringType& path,
-                        base::FilePath::StringType* out_path) {
-  base::FilePath::StringType::size_type trim_pos =
-      path.find_last_not_of(FILE_PATH_LITERAL(". "));
-  if (trim_pos == base::FilePath::StringType::npos)
-    return false;
-
-  *out_path = path.substr(0, trim_pos + 1);
-  return true;
-}
-#endif  // defined(OS_WIN)
-
 }  // namespace
 
 namespace extensions {
@@ -225,9 +211,11 @@ std::unique_ptr<VerifiedContents> VerifiedContents::Create(
       // that any filename with (.| )+ suffix can be matched later, see
       // HasTreeHashRoot() and TreeHashRootEquals().
       base::FilePath::StringType trimmed_path;
-      if (TrimDotSpaceSuffix(lowercase_file_path, &trimmed_path))
+      if (content_verifier_utils::TrimDotSpaceSuffix(lowercase_file_path,
+                                                     &trimmed_path)) {
         verified_contents->root_hashes_.insert(
             std::make_pair(trimmed_path, i->second));
+     }
 #endif  // defined(OS_WIN)
     }
 
@@ -246,7 +234,7 @@ bool VerifiedContents::HasTreeHashRoot(
 
 #if defined(OS_WIN)
   base::FilePath::StringType trimmed_path;
-  if (TrimDotSpaceSuffix(path, &trimmed_path))
+  if (content_verifier_utils::TrimDotSpaceSuffix(path, &trimmed_path))
     return base::Contains(root_hashes_, trimmed_path);
 #endif  // defined(OS_WIN)
   return false;
@@ -261,8 +249,10 @@ bool VerifiedContents::TreeHashRootEquals(const base::FilePath& relative_path,
 
 #if defined(OS_WIN)
   base::FilePath::StringType trimmed_relative_path;
-  if (TrimDotSpaceSuffix(normalized_relative_path, &trimmed_relative_path))
+  if (content_verifier_utils::TrimDotSpaceSuffix(normalized_relative_path,
+                                                 &trimmed_relative_path)) {
     return TreeHashRootEqualsImpl(trimmed_relative_path, expected);
+  }
 #endif  // defined(OS_WIN)
   return false;
 }