CV: Fix verification for incorrect case filename in case-sensitive OS.

Content verification currently always performs case-insensitive
searches (by using lower case keys in VerifiedContents::root_hashes_,
or by searching using CompareEqualsIgnoreCase in ComputedHashes) to
support case-insensitive systems (win/mac). However, currently this
applies to case-sensitive systems (linux/cros) as well. The
effect is that a request to manifesT.json for example would return the
hash of manifest.json but file read of manifesT.json would not be found.
Therefore, the hashes would mismatch, incorrectly disabling the
extension due to content-verification failure.

This CL changes case-insensitive logic to only apply for
case-insensitive systems (win/mac) and not case-sensitive ones
(linux/cros).

This CL also moves case-sensitivity and dot-space logic to
central location under content_verifier_utils.

This CL also updates test expectations for existing tests and
adds a regression test for the fix.

Bug: 1033294
Change-Id: Ied57039e316ce7c5e5a57fcec96f28ddeac305dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1993940
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731874}

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -452,6 +452,36 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierTest,
   EXPECT_EQ(disable_reason::DISABLE_NONE, reasons);
 }
 
+// Tests that navigating to an extension resource with incorrect case does not
+// disable the extension, both in case-sensitive and case-insensitive systems.
+//
+// Regression test for https://crbug.com/1033294.
+IN_PROC_BROWSER_TEST_F(ContentVerifierTest,
+                       RemainsEnabledOnNavigateToPathWithIncorrectCase) {
+  const Extension* extension = InstallExtensionFromWebstore(
+      test_data_dir_.AppendASCII("content_verifier/dot_slash_paths.crx"), 1);
+  ASSERT_TRUE(extension);
+  const ExtensionId extension_id = extension->id();
+
+  // Note: the resource in |extension| is "page.html".
+  constexpr char kIncorrectCasePath[] = "PAGE.html";
+
+  TestContentVerifySingleJobObserver job_observer(
+      extension_id, base::FilePath().AppendASCII(kIncorrectCasePath));
+
+  GURL page_url = extension->GetResourceURL(kIncorrectCasePath);
+  ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete(
+      browser(), page_url, 1, WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  // Ensure that ContentVerifyJob has finished checking the resource.
+  EXPECT_EQ(ContentVerifyJob::NONE, job_observer.WaitForJobFinished());
+
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  int reasons = prefs->GetDisableReasons(extension_id);
+  EXPECT_EQ(disable_reason::DISABLE_NONE, reasons);
+}
+
 class ContentVerifierPolicyTest : public ContentVerifierTest {
  public:
   // We need to do this work here because the force-install policy values are

extensions/browser/computed_hashes.cc

@@ -170,9 +170,13 @@ bool ComputedHashes::GetHashes(const base::FilePath& relative_path,
                                int* block_size,
                                std::vector<std::string>* hashes) const {
   base::FilePath path = relative_path.NormalizePathSeparatorsTo('/');
+  // TODO(lazyboy): Align treatment of |data_| with that of
+  // VerifiedContents::root_hashes_, so that we don't have to perform the linear
+  // lookup below.
   auto find_data = [&](const base::FilePath& normalized_path) {
     auto i = data_.find(normalized_path);
-    if (i == data_.end()) {
+    if (i == data_.end() &&
+        !content_verifier_utils::IsFileAccessCaseSensitive()) {
       // 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
@@ -192,8 +196,8 @@ bool ComputedHashes::GetHashes(const base::FilePath& relative_path,
     return i;
   };
   auto i = find_data(path);
-#if defined(OS_WIN)
-  if (i == data_.end()) {
+  if (i == data_.end() &&
+      content_verifier_utils::IsDotSpaceFilenameSuffixIgnored()) {
     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
@@ -203,7 +207,6 @@ bool ComputedHashes::GetHashes(const base::FilePath& relative_path,
       i = find_data(base::FilePath(trimmed_path_value));
     }
   }
-#endif  // defined(OS_WIN)
   if (i == data_.end())
     return false;
 

extensions/browser/computed_hashes_unittest.cc

@@ -9,19 +9,16 @@
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "crypto/sha2.h"
+#include "extensions/browser/content_verifier/content_verifier_utils.h"
 #include "extensions/common/constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
-// Whether or not dot and space suffixes of filename are ignored in the
-// current OS.
-const bool kDotSpaceSuffixIgnored =
-#if defined(OS_WIN)
-    true;
-#else
-    false;
-#endif  // defined(OS_WIN)
+constexpr bool kIsDotSpaceSuffixIgnored =
+    extensions::content_verifier_utils::IsDotSpaceFilenameSuffixIgnored();
+constexpr bool kIsFileAccessCaseInsensitive =
+    !extensions::content_verifier_utils::IsFileAccessCaseSensitive();
 
 // Helper to return base64 encode result by value.
 std::string Base64Encode(const std::string& data) {
@@ -104,10 +101,13 @@ TEST(ComputedHashesTest, ComputedHashes) {
   // Make sure we can lookup hashes for a file using incorrect case
   base::FilePath path1_badcase(FILE_PATH_LITERAL("FoO.txt"));
   std::vector<std::string> read_hashes1_badcase;
-  EXPECT_TRUE(computed_hashes.GetHashes(path1_badcase, &block_size,
-                                        &read_hashes1_badcase));
-  EXPECT_EQ(block_size, 4096);
-  EXPECT_EQ(hashes1, read_hashes1_badcase);
+  EXPECT_EQ(kIsFileAccessCaseInsensitive,
+            computed_hashes.GetHashes(path1_badcase, &block_size,
+                                      &read_hashes1_badcase));
+  if (kIsFileAccessCaseInsensitive) {
+    EXPECT_EQ(4096, block_size);
+    EXPECT_EQ(hashes1, read_hashes1_badcase);
+  }
 
   // Finally make sure that we can retrieve the hashes for the subdir
   // path even when that path contains forward slashes (on windows).
@@ -193,17 +193,17 @@ TEST(ComputedHashesTest, DotSpaceSuffix) {
       // Sanity check: non existent file.
       {"notfound.html", false},
       // Path with "." suffix, along with incorrect case for the same.
-      {"foo.html.", kDotSpaceSuffixIgnored},
-      {"fOo.html.", kDotSpaceSuffixIgnored},
+      {"foo.html.", kIsDotSpaceSuffixIgnored},
+      {"fOo.html.", kIsDotSpaceSuffixIgnored},
       // Path with " " suffix, along with incorrect case for the same.
-      {"foo.html ", kDotSpaceSuffixIgnored},
-      {"fOo.html ", kDotSpaceSuffixIgnored},
+      {"foo.html ", kIsDotSpaceSuffixIgnored},
+      {"fOo.html ", kIsDotSpaceSuffixIgnored},
       // Path with ". " suffix, along with incorrect case for the same.
-      {"foo.html. ", kDotSpaceSuffixIgnored},
-      {"fOo.html. ", kDotSpaceSuffixIgnored},
+      {"foo.html. ", kIsDotSpaceSuffixIgnored},
+      {"fOo.html. ", kIsDotSpaceSuffixIgnored},
       // Path with " ." suffix, along with incorrect case for the same.
-      {"foo.html .", kDotSpaceSuffixIgnored},
-      {"fOo.html .", kDotSpaceSuffixIgnored},
+      {"foo.html .", kIsDotSpaceSuffixIgnored},
+      {"fOo.html .", kIsDotSpaceSuffixIgnored},
   };
 
   for (const auto& test_case : test_cases) {

extensions/browser/content_verifier/content_hash_unittest.cc

@@ -148,7 +148,7 @@ class TestExtensionBuilder {
     ListBuilder files;
     for (const auto& resource : extension_resources_) {
       base::FilePath::StringType path =
-          VerifiedContents::NormalizeResourcePath(resource.relative_path);
+          base::FilePath(resource.relative_path).value();
       std::string tree_hash =
           ContentHash::ComputeTreeHashForContent(resource.contents, block_size);
 

extensions/browser/content_verifier/content_verifier_utils.cc

@@ -4,12 +4,15 @@
 
 #include "extensions/browser/content_verifier/content_verifier_utils.h"
 
+#include "base/strings/string_util.h"
+
 namespace extensions {
 namespace content_verifier_utils {
 
-#if defined(OS_WIN)
 bool TrimDotSpaceSuffix(const base::FilePath::StringType& path,
                         base::FilePath::StringType* out_path) {
+  DCHECK(IsDotSpaceFilenameSuffixIgnored())
+      << "dot-space suffix shouldn't be trimmed in current system";
   base::FilePath::StringType::size_type trim_pos =
       path.find_last_not_of(FILE_PATH_LITERAL(". "));
   if (trim_pos == base::FilePath::StringType::npos)
@@ -18,7 +21,16 @@ bool TrimDotSpaceSuffix(const base::FilePath::StringType& path,
   *out_path = path.substr(0, trim_pos + 1);
   return true;
 }
-#endif  // defined(OS_WIN)
+
+base::FilePath::StringType CanonicalizeFilePath(const base::FilePath& path) {
+  base::FilePath::StringType canonicalized_path =
+      path.NormalizePathSeparatorsTo('/').value();
+  if (!IsFileAccessCaseSensitive())
+    canonicalized_path = base::ToLowerASCII(canonicalized_path);
+  if (IsDotSpaceFilenameSuffixIgnored())
+    TrimDotSpaceSuffix(canonicalized_path, &canonicalized_path);
+  return canonicalized_path;
+}
 
 }  // namespace content_verifier_utils
 }  // namespace extensions

extensions/browser/content_verifier/content_verifier_utils.h

@@ -10,12 +10,36 @@
 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)
+
+// Returns true if this system/OS's file access is case sensitive.
+constexpr bool IsFileAccessCaseSensitive() {
+#if defined(OS_WIN) || defined(OS_MACOSX)
+  return false;
+#else
+  return true;
+#endif
+}
+
+// Returns true if this system/OS ignores (.| )+ suffix in a filepath while
+// accessing the file.
+constexpr bool IsDotSpaceFilenameSuffixIgnored() {
+#if defined(OS_WIN)
+  static_assert(!IsFileAccessCaseSensitive(),
+                "DotSpace suffix should only be ignored in case-insensitive"
+                "systems");
+  return true;
+#else
+  return false;
+#endif
+}
+
+// Returns platform specific canonicalized version of |path| for content
+// verification system.
+base::FilePath::StringType CanonicalizeFilePath(const base::FilePath& path);
 
 }  // namespace content_verifier_utils
 }  // namespace extensions

extensions/browser/verified_contents.cc

@@ -170,25 +170,13 @@ std::unique_ptr<VerifiedContents> VerifiedContents::Create(
                                  &root_hash)) {
         return nullptr;
       }
-      base::FilePath file_path =
-          base::FilePath::FromUTF8Unsafe(*file_path_string);
-      base::FilePath::StringType lowercase_file_path =
-          base::ToLowerASCII(file_path.value());
+
+      base::FilePath::StringType canonicalized_path =
+          content_verifier_utils::CanonicalizeFilePath(
+              base::FilePath::FromUTF8Unsafe(*file_path_string));
       auto i = verified_contents->root_hashes_.insert(
-          std::make_pair(lowercase_file_path, std::string()));
+          std::make_pair(canonicalized_path, std::string()));
       i->second.swap(root_hash);
-
-#if defined(OS_WIN)
-      // Additionally store a canonicalized filename without (.| )+ suffix, so
-      // that any filename with (.| )+ suffix can be matched later, see
-      // HasTreeHashRoot() and TreeHashRootEquals().
-      base::FilePath::StringType 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)
     }
 
     break;
@@ -199,40 +187,15 @@ std::unique_ptr<VerifiedContents> VerifiedContents::Create(
 
 bool VerifiedContents::HasTreeHashRoot(
     const base::FilePath& relative_path) const {
-  base::FilePath::StringType path = NormalizeResourcePath(relative_path);
-  if (base::Contains(root_hashes_, path))
-    return true;
-
-#if defined(OS_WIN)
-  base::FilePath::StringType trimmed_path;
-  if (content_verifier_utils::TrimDotSpaceSuffix(path, &trimmed_path))
-    return base::Contains(root_hashes_, trimmed_path);
-#endif  // defined(OS_WIN)
-  return false;
+  return base::Contains(
+      root_hashes_,
+      content_verifier_utils::CanonicalizeFilePath(relative_path));
 }
 
 bool VerifiedContents::TreeHashRootEquals(const base::FilePath& relative_path,
                                           const std::string& expected) const {
-  base::FilePath::StringType normalized_relative_path =
-      NormalizeResourcePath(relative_path);
-  if (TreeHashRootEqualsImpl(normalized_relative_path, expected))
-    return true;
-
-#if defined(OS_WIN)
-  base::FilePath::StringType 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;
-}
-
-// static
-base::FilePath::StringType VerifiedContents::NormalizeResourcePath(
-    const base::FilePath& relative_path) {
-  return base::ToLowerASCII(
-      relative_path.NormalizePathSeparatorsTo('/').value());
+  return TreeHashRootEqualsImpl(
+      content_verifier_utils::CanonicalizeFilePath(relative_path), expected);
 }
 
 // We're loosely following the "JSON Web Signature" draft spec for signing

extensions/browser/verified_contents.h

@@ -49,9 +49,6 @@ class VerifiedContents {
   // signature" mode, this can return false.
   bool valid_signature() { return valid_signature_; }
 
-  static base::FilePath::StringType NormalizeResourcePath(
-      const base::FilePath& relative_path);
-
  private:
   // Note: the public_key must remain valid for the lifetime of this object.
   explicit VerifiedContents(base::span<const uint8_t> public_key);
@@ -85,15 +82,19 @@ class VerifiedContents {
   std::string extension_id_;
   base::Version version_;
 
-  // The expected treehash root hashes for each file, lower cased so we can do
+  // The expected treehash root hashes for each file.
+  // For case-sensitive systems (linux/chromeos) the key is exact cased, but for
+  // case-insensitive systems (win/macos) the key is lower cased to support
   // case-insensitive lookups.
   //
   // We use a multi-map here so that we can do fast lookups of paths from
   // requests on case-insensitive systems (windows, mac) where the request path
-  // might not have the exact right capitalization, but not break
-  // case-sensitive systems (linux, chromeos). TODO(asargent) - we should give
-  // developers client-side warnings in each of those cases, and have the
-  // webstore reject the cases they can statically detect. See crbug.com/29941
+  // might not have the exact right capitalization. Note that this doesn't
+  // affect case-sensitive systems (linux, chromeos) as we use the exact cased
+  // keys.
+  // TODO(crbug.com/29941) - we should give developers client-side warnings in
+  // each of those cases, and have the webstore reject the cases they can
+  // statically detect.
   typedef std::multimap<base::FilePath::StringType, std::string> RootHashes;
   RootHashes root_hashes_;