Bug 1760340: Re-allow forward slashes in windows sandbox file system policy rules. r=handyman
authorBob Owen <bobowencode@gmail.com>
Wed, 27 Apr 2022 19:27:28 +0000
changeset 615521 3b57c0c8f9672253e7f440f05e89b54a2e7e114b
parent 615520 0d0db8c31d9f52069da17aed5d99cb9f120cbbf9
child 615522 edc4088288ca1d7a617ebfc189b52a61f763ca97
push id161821
push userbobowencode@gmail.com
push dateWed, 27 Apr 2022 19:29:59 +0000
treeherderautoland@3b57c0c8f967 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershandyman
bugs1760340
milestone101.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1760340: Re-allow forward slashes in windows sandbox file system policy rules. r=handyman Differential Revision: https://phabricator.services.mozilla.com/D144849
security/sandbox/chromium-shim/patches/with_update/allow_reparse_points.patch
security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
--- a/security/sandbox/chromium-shim/patches/with_update/allow_reparse_points.patch
+++ b/security/sandbox/chromium-shim/patches/with_update/allow_reparse_points.patch
@@ -99,17 +99,35 @@ diff --git a/security/sandbox/chromium/s
  
    uint32_t broker = BROKER_TRUE;
    const wchar_t* filename = name.c_str();
    CountedParameterSet<FileName> params;
    params[FileName::NAME] = ParamPickerMake(filename);
 diff --git a/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc b/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
 --- a/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
 +++ b/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
-@@ -39,22 +39,16 @@ NTSTATUS NtCreateFileInTarget(HANDLE* ta
+@@ -1,16 +1,17 @@
+ // Copyright (c) 2011 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 "sandbox/win/src/filesystem_policy.h"
+ 
+ #include <stdint.h>
+ 
++#include <algorithm>
+ #include <string>
+ 
+ #include "base/logging.h"
+ #include "base/stl_util.h"
+ #include "base/win/scoped_handle.h"
+ #include "base/win/windows_version.h"
+ #include "sandbox/win/src/ipc_tags.h"
+ #include "sandbox/win/src/policy_engine_opcodes.h"
+@@ -39,22 +40,16 @@ NTSTATUS NtCreateFileInTarget(HANDLE* ta
    NTSTATUS status =
        NtCreateFile(&local_handle, desired_access, obj_attributes,
                     io_status_block, nullptr, file_attributes, share_access,
                     create_disposition, create_options, ea_buffer, ea_length);
    if (!NT_SUCCESS(status)) {
      return status;
    }
  
@@ -122,45 +140,43 @@ diff --git a/security/sandbox/chromium/s
    if (!::DuplicateHandle(::GetCurrentProcess(), local_handle, target_process,
                           target_file_handle, 0, false,
                           DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
      return STATUS_ACCESS_DENIED;
    }
    return STATUS_SUCCESS;
  }
  
-@@ -400,23 +394,35 @@ bool FileSystemPolicy::SetInformationFil
+@@ -400,23 +395,32 @@ bool FileSystemPolicy::SetInformationFil
        static_cast<FILE_INFORMATION_CLASS>(info_class);
    *nt_status = NtSetInformationFile(local_handle, io_block, file_info, length,
                                      file_info_class);
  
    return true;
  }
  
  bool PreProcessName(std::wstring* path) {
 -  ConvertToLongPath(path);
 +  // We now allow symbolic links to be opened via the broker, so we can no
 +  // longer rely on the same object check where we checked the path of the
 +  // opened file against the original. We don't specify a root when creating
 +  // OBJECT_ATTRIBUTES from file names for brokering so they must be fully
 +  // qualified and we can just check for the parent directory double dot between
 +  // two backslashes. NtCreateFile doesn't seem to allow it anyway, but this is
-+  // just an extra precaution. It also doesn't seem to allow the forward slash
-+  // at least in fully qualified names so we rule out that as well, to simplify
-+  // the combinations we might have to check.
-+  if (path->find(L'/') != std::wstring::npos) {
++  // just an extra precaution. It also doesn't seem to allow the forward slash,
++  // but this is also used for checking policy rules, so we just replace forward
++  // slashes with backslashes.
++  std::replace(path->begin(), path->end(), L'/', L'\\');
++  if (path->find(L"\\..\\") != std::wstring::npos) {
 +    return false;
 +  }
  
 -  if (ERROR_NOT_A_REPARSE_POINT == IsReparsePoint(*path))
 -    return true;
-+  if (path->find(L"\\..\\") != std::wstring::npos) {
-+    return false;
-+  }
- 
+-
 -  // We can't process a reparsed file.
 -  return false;
 +  ConvertToLongPath(path);
 +  return true;
  }
  
  std::wstring FixNTPrefixForMatch(const std::wstring& name) {
    std::wstring mod_name = name;
--- a/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
+++ b/security/sandbox/chromium/sandbox/win/src/filesystem_policy.cc
@@ -1,16 +1,17 @@
 // Copyright (c) 2011 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 "sandbox/win/src/filesystem_policy.h"
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <string>
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_version.h"
 #include "sandbox/win/src/ipc_tags.h"
 #include "sandbox/win/src/policy_engine_opcodes.h"
@@ -400,23 +401,20 @@ bool FileSystemPolicy::SetInformationFil
 
 bool PreProcessName(std::wstring* path) {
   // We now allow symbolic links to be opened via the broker, so we can no
   // longer rely on the same object check where we checked the path of the
   // opened file against the original. We don't specify a root when creating
   // OBJECT_ATTRIBUTES from file names for brokering so they must be fully
   // qualified and we can just check for the parent directory double dot between
   // two backslashes. NtCreateFile doesn't seem to allow it anyway, but this is
-  // just an extra precaution. It also doesn't seem to allow the forward slash
-  // at least in fully qualified names so we rule out that as well, to simplify
-  // the combinations we might have to check.
-  if (path->find(L'/') != std::wstring::npos) {
-    return false;
-  }
-
+  // just an extra precaution. It also doesn't seem to allow the forward slash,
+  // but this is also used for checking policy rules, so we just replace forward
+  // slashes with backslashes.
+  std::replace(path->begin(), path->end(), L'/', L'\\');
   if (path->find(L"\\..\\") != std::wstring::npos) {
     return false;
   }
 
   ConvertToLongPath(path);
   return true;
 }