Reland "Adds mojo_base.mojom.ReadOnlyFile"

This is a reland of a358e673

Original change's description:
> Adds mojo_base.mojom.ReadOnlyFile
>
> Adds a mojo ReadOnlyFile that won't serialize if it can be written.
>
> The expectation is that this handle will then be sent to another process
> which lacks the rights to add any write permissions to the handle. This
> may not be true on all platforms but is useful where this holds.
>
> Code to check for readonly-ness is implemented within mojo rather than
> base::File as it is (a) difficult to say in general what it means to be
> readonly, and (b) not possible on every platform.
>
> Bug: 1130762
> Change-Id: I6f00934005da7e433af4fee94454dfcd28dfd3c5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417549
> Commit-Queue: Alex Gough <ajgo@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Ken Rockot <rockot@google.com>
> Reviewed-by: Matthew Denton <mpdenton@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#813987}

Bug: 1130762
Change-Id: I4ae14a37b57867e6b02ad2e3f69d248032e0c3c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2450893Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814537}

mojo/public/cpp/base/BUILD.gn

@@ -90,6 +90,8 @@ component("shared_typemap_traits") {
     "generic_pending_receiver_mojom_traits.h",
     "read_only_buffer_mojom_traits.cc",
     "read_only_buffer_mojom_traits.h",
+    "read_only_file_mojom_traits.cc",
+    "read_only_file_mojom_traits.h",
     "shared_memory_mojom_traits.cc",
     "shared_memory_mojom_traits.h",
     "time_mojom_traits.cc",

mojo/public/cpp/base/file_unittest.cc

@@ -3,9 +3,13 @@
 // found in the LICENSE file.
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/sync_socket.h"
+#include "build/build_config.h"
 #include "mojo/public/cpp/base/file_mojom_traits.h"
+#include "mojo/public/cpp/base/read_only_file_mojom_traits.h"
 #include "mojo/public/cpp/test_support/test_utils.h"
 #include "mojo/public/mojom/base/file.mojom.h"
+#include "mojo/public/mojom/base/read_only_file.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace mojo_base {
@@ -69,5 +73,95 @@ TEST(FileTest, InvalidFile) {
   EXPECT_FALSE(file_out.IsValid());
 }
 
+TEST(FileTest, ReadOnlyFile) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  base::File file(
+      temp_dir.GetPath().AppendASCII("test_file.txt"),
+      base::File::FLAG_CREATE | base::File::FLAG_WRITE | base::File::FLAG_READ);
+  const base::StringPiece test_content =
+      "A test string to be stored in a test file";
+  file.WriteAtCurrentPos(test_content.data(),
+                         base::checked_cast<int>(test_content.size()));
+  file.Close();
+
+  base::File readonly(temp_dir.GetPath().AppendASCII("test_file.txt"),
+                      base::File::FLAG_OPEN | base::File::FLAG_READ);
+
+  base::File file_out;
+  ASSERT_TRUE(mojo::test::SerializeAndDeserialize<mojom::ReadOnlyFile>(
+      &readonly, &file_out));
+  std::vector<char> content(test_content.size());
+  ASSERT_TRUE(file_out.IsValid());
+  ASSERT_FALSE(file_out.async());
+  ASSERT_EQ(static_cast<int>(test_content.size()),
+            file_out.Read(0, content.data(),
+                          base::checked_cast<int>(test_content.size())));
+  EXPECT_EQ(test_content,
+            base::StringPiece(content.data(), test_content.size()));
+}
+
+// This dies only if we can interrogate the underlying platform handle.
+#if defined(OS_WIN) || defined(OS_POSIX) || defined(OS_FUCHSIA)
+#if !defined(OS_NACL) && !defined(OS_AIX)
+TEST(FileTest, ReadOnlyFileDeath) {
+#if defined(OFFICIAL_BUILD)
+  const char kReadOnlyFileCheckFailedRegex[] = "";
+#else
+  const char kReadOnlyFileCheckFailedRegex[] = "Check failed: IsReadOnlyFile";
+#endif
+
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  base::File file(
+      temp_dir.GetPath().AppendASCII("test_file.txt"),
+      base::File::FLAG_CREATE | base::File::FLAG_WRITE | base::File::FLAG_READ);
+  const base::StringPiece test_content =
+      "A test string to be stored in a test file";
+  file.WriteAtCurrentPos(test_content.data(),
+                         base::checked_cast<int>(test_content.size()));
+  file.Close();
+
+  base::File writable(
+      temp_dir.GetPath().AppendASCII("test_file.txt"),
+      base::File::FLAG_OPEN | base::File::FLAG_READ | base::File::FLAG_WRITE);
+
+  base::File file_out;
+  EXPECT_DEATH_IF_SUPPORTED(
+      mojo::test::SerializeAndDeserialize<mojom::ReadOnlyFile>(&writable,
+                                                               &file_out),
+      kReadOnlyFileCheckFailedRegex);
+}
+#endif  // !defined(OS_NACL) && !defined(OS_AIX)
+#endif  // defined(OS_WIN) || defined(OS_POSIX) || defined(OS_FUCHSIA)
+
+// This should work on all platforms. This check might be relaxed in which case
+// this test can be removed.
+TEST(FileTest, NonPhysicalFileDeath) {
+#if defined(OFFICIAL_BUILD)
+  const char kPhysicalFileCheckFailedRegex[] = "";
+#else
+  const char kPhysicalFileCheckFailedRegex[] = "Check failed: IsPhysicalFile";
+#endif
+
+  base::SyncSocket sync_a;
+  base::SyncSocket sync_b;
+  ASSERT_TRUE(base::SyncSocket::CreatePair(&sync_a, &sync_b));
+  base::File file_pipe_a(sync_a.Take());
+  base::File file_pipe_b(sync_b.Take());
+
+  base::File file_out;
+  EXPECT_DEATH_IF_SUPPORTED(
+      mojo::test::SerializeAndDeserialize<mojom::ReadOnlyFile>(&file_pipe_a,
+                                                               &file_out),
+      kPhysicalFileCheckFailedRegex);
+  EXPECT_DEATH_IF_SUPPORTED(
+      mojo::test::SerializeAndDeserialize<mojom::ReadOnlyFile>(&file_pipe_b,
+                                                               &file_out),
+      kPhysicalFileCheckFailedRegex);
+}
+
 }  // namespace file_unittest
 }  // namespace mojo_base

mojo/public/cpp/base/read_only_file_mojom_traits.cc

+// Copyright 2020 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 "mojo/public/cpp/base/read_only_file_mojom_traits.h"
+
+#include "base/files/file.h"
+#include "build/build_config.h"
+
+#if defined(OS_POSIX) || defined(OS_FUCHSIA)
+#include <fcntl.h>
+#endif
+#if defined(OS_WIN)
+#include <windows.h>
+#include <winternl.h>
+#endif
+
+namespace mojo {
+namespace {
+#if defined(OS_WIN)
+bool GetGrantedAccess(HANDLE handle, DWORD* flags) {
+  static const auto nt_query_object =
+      reinterpret_cast<decltype(&NtQueryObject)>(
+          GetProcAddress(GetModuleHandle(L"ntdll.dll"), "NtQueryObject"));
+  PUBLIC_OBJECT_BASIC_INFORMATION info;
+  ULONG len = sizeof(info);
+  ULONG consumed = 0;
+  auto ret =
+      nt_query_object(handle, ObjectBasicInformation, &info, len, &consumed);
+  if (ret)
+    return false;
+  *flags = info.GrantedAccess;
+  return true;
+}
+#endif  // defined(OS_WIN)
+
+// True if the underlying handle is only readable. Where possible this excludes
+// deletion, writing, truncation, append and other operations that might modify
+// the underlying file. False if we can tell that the file could be modified.
+// On platforms where we cannot test the handle, always returns true.
+bool IsReadOnlyFile(base::File& file) {
+  bool is_readonly = true;
+#if defined(OS_WIN)
+  DWORD flags = 0;
+  if (!GetGrantedAccess(file.GetPlatformFile(), &flags))
+    return false;
+
+  // Cannot use GENERIC_WRITE as that includes SYNCHRONIZE.
+  // This is ~(all the writable permissions).
+  is_readonly =
+      !(flags & (FILE_APPEND_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_DATA |
+                 FILE_WRITE_EA | WRITE_DAC | WRITE_OWNER | DELETE));
+#elif defined(OS_FUCHSIA) || \
+    (defined(OS_POSIX) && !defined(OS_NACL) && !defined(OS_AIX))
+  is_readonly =
+      (fcntl(file.GetPlatformFile(), F_GETFL) & O_ACCMODE) == O_RDONLY;
+#endif
+  return is_readonly;
+}
+
+bool IsPhysicalFile(base::File& file) {
+#if defined(OS_WIN)
+  // Verify if this is a real file (not a socket/pipe etc.).
+  DWORD type = GetFileType(file.GetPlatformFile());
+  return type == FILE_TYPE_DISK;
+#else
+  base::stat_wrapper_t stat;
+  if (base::File::Fstat(file.GetPlatformFile(), &stat) != 0)
+    return false;
+  return S_ISREG(stat.st_mode);
+#endif
+}
+
+}  // namespace
+
+mojo::PlatformHandle StructTraits<mojo_base::mojom::ReadOnlyFileDataView,
+                                  base::File>::fd(base::File& file) {
+  DCHECK(file.IsValid());
+  // For now we require real files as on some platforms it is too difficult to
+  // be sure that more general handles cannot be written or made writable. This
+  // could be relaxed if an interface needs readonly pipes.
+  CHECK(IsPhysicalFile(file));
+  CHECK(IsReadOnlyFile(file));
+
+  return mojo::PlatformHandle(
+      base::ScopedPlatformFile(file.TakePlatformFile()));
+}
+
+bool StructTraits<mojo_base::mojom::ReadOnlyFileDataView, base::File>::Read(
+    mojo_base::mojom::ReadOnlyFileDataView data,
+    base::File* file) {
+  *file = base::File(data.TakeFd().TakePlatformFile(), data.async());
+  return true;
+}
+
+}  // namespace mojo

mojo/public/cpp/base/read_only_file_mojom_traits.h

+// Copyright 2020 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 MOJO_PUBLIC_CPP_BASE_READ_ONLY_FILE_MOJOM_TRAITS_H_
+#define MOJO_PUBLIC_CPP_BASE_READ_ONLY_FILE_MOJOM_TRAITS_H_
+
+#include "base/component_export.h"
+#include "base/files/file.h"
+#include "mojo/public/mojom/base/read_only_file.mojom-shared.h"
+
+namespace mojo {
+
+template <>
+struct COMPONENT_EXPORT(MOJO_BASE_SHARED_TRAITS)
+    StructTraits<mojo_base::mojom::ReadOnlyFileDataView, base::File> {
+  static bool IsNull(const base::File& file) { return !file.IsValid(); }
+
+  static void SetToNull(base::File* file) { *file = base::File(); }
+
+  static mojo::PlatformHandle fd(base::File& file);
+  static bool async(base::File& file) { return file.async(); }
+  static bool Read(mojo_base::mojom::ReadOnlyFileDataView data,
+                   base::File* file);
+};
+
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BASE_READ_ONLY_FILE_MOJOM_TRAITS_H_

mojo/public/mojom/base/BUILD.gn

@@ -21,6 +21,7 @@ mojom_component("base") {
     "message_pump_type.mojom",
     "process_id.mojom",
     "read_only_buffer.mojom",
+    "read_only_file.mojom",
     "ref_counted_memory.mojom",
     "shared_memory.mojom",
     "string16.mojom",
@@ -118,6 +119,24 @@ mojom_component("base") {
         "//mojo/public/cpp/base:shared_typemap_traits",
       ]
     },
+    {
+      types = [
+        {
+          mojom = "mojo_base.mojom.ReadOnlyFile"
+          cpp = "::base::File"
+          move_only = true
+          nullable_is_same_type = true
+          force_serialize = true
+        },
+      ]
+      traits_headers =
+          [ "//mojo/public/cpp/base/read_only_file_mojom_traits.h" ]
+      traits_public_deps = [
+        "//base",
+        "//mojo/public/cpp/base",
+        "//mojo/public/cpp/base:shared_typemap_traits",
+      ]
+    },
     {
       types = [
         {

mojo/public/mojom/base/read_only_file.mojom

+// Copyright 2020 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.
+
+module mojo_base.mojom;
+
+// Corresponds to |base::File| in base/files/file.h but, on most
+// platforms, will not serialise handles which are writable. At
+// present this only supports physically backed files, but this may be
+// relaxed in future.
+//
+// SECURITY_NOTE: This type is an indication that a readonly handle can
+// be provided. A sandboxed process should ensure that the handle cannot
+// be made writable. This may not be possible on all platforms.
+//
+// See |file_mojom_traits.cc| for details.
+struct ReadOnlyFile {
+  handle<platform> fd;
+  bool async;
+};