permission: fix some vulnerabilities in fs

src/node_file.cc

@@ -64,8 +64,11 @@ using v8::HandleScope;
 using v8::Int32;
 using v8::Integer;
 using v8::Isolate;
+using v8::JustVoid;
 using v8::Local;
+using v8::Maybe;
 using v8::MaybeLocal;
+using v8::Nothing;
 using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -1949,6 +1952,37 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+static inline Maybe<void> CheckOpenPermissions(Environment* env,
+                                               const BufferValue& path,
+                                               int flags) {
+  // These flags capture the intention of the open() call.
+  const int rwflags = flags & (UV_FS_O_RDONLY | UV_FS_O_WRONLY | UV_FS_O_RDWR);
+
+  // These flags have write-like side effects even with O_RDONLY, at least on
+  // some operating systems. On Windows, for example, O_RDONLY | O_TEMPORARY
+  // can be used to delete a file. Bizarre.
+  const int write_as_side_effect = flags & (UV_FS_O_APPEND | UV_FS_O_CREAT |
+                                            UV_FS_O_TRUNC | UV_FS_O_TEMPORARY);
+
+  // TODO(rafaelgss): it can be optimized to avoid two permission checks
+  auto pathView = path.ToStringView();
+  if (rwflags != UV_FS_O_WRONLY) {
+    THROW_IF_INSUFFICIENT_PERMISSIONS(
+        env,
+        permission::PermissionScope::kFileSystemRead,
+        pathView,
+        Nothing<void>());
+  }
+  if (rwflags != UV_FS_O_RDONLY || write_as_side_effect) {
+    THROW_IF_INSUFFICIENT_PERMISSIONS(
+        env,
+        permission::PermissionScope::kFileSystemWrite,
+        pathView,
+        Nothing<void>());
+  }
+  return JustVoid();
+}
+
 static void Open(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -1964,22 +1998,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[2]->IsInt32());
   const int mode = args[2].As<Int32>()->Value();
 
-  auto pathView = path.ToStringView();
-  // Open can be called either in write or read
-  if (flags == O_RDWR) {
-    // TODO(rafaelgss): it can be optimized to avoid O(2*n)
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemRead, pathView);
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemWrite, pathView);
-  } else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemRead, pathView);
-  } else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
-                       UV_FS_O_WRONLY)) != 0) {
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemWrite, pathView);
-  }
+  if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
 
   FSReqBase* req_wrap_async = GetReqWrap(args, 3);
   if (req_wrap_async != nullptr) {  // open(path, flags, mode, req)
@@ -2010,31 +2029,14 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
-  auto pathView = path.ToStringView();
-  THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kFileSystemRead, pathView);
 
   CHECK(args[1]->IsInt32());
   const int flags = args[1].As<Int32>()->Value();
 
   CHECK(args[2]->IsInt32());
   const int mode = args[2].As<Int32>()->Value();
 
-  // Open can be called either in write or read
-  if (flags == O_RDWR) {
-    // TODO(rafaelgss): it can be optimized to avoid O(2*n)
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemRead, pathView);
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemWrite, pathView);
-  } else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemRead, pathView);
-  } else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
-                       UV_FS_O_WRONLY)) != 0) {
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kFileSystemWrite, pathView);
-  }
+  if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
 
   FSReqBase* req_wrap_async = GetReqWrap(args, 3);
   if (req_wrap_async != nullptr) {  // openFileHandle(path, flags, mode, req)

test/parallel/test-permission-deny-fs-read.js

@@ -249,6 +249,21 @@ const gid = os.userInfo().gid;
   fs.open(regularFile, 'r', (err) => {
     assert.ifError(err);
   });
+
+  // Extra flags should not enable trivially bypassing all restrictions.
+  // See https://github.com/nodejs/node/issues/47090.
+  assert.throws(() => {
+    fs.openSync(blockedFile, fs.constants.O_RDONLY | fs.constants.O_NOCTTY);
+  }, {
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemRead',
+  });
+  assert.throws(() => {
+    fs.open(blockedFile, fs.constants.O_RDWR | 0x10000000, common.mustNotCall());
+  }, {
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemRead',
+  });
 }
 
 // fs.opendir

test/parallel/test-permission-deny-fs-write.js

@@ -238,3 +238,32 @@ const regularFile = fixtures.path('permission', 'deny', 'regular-file.md');
     resource: path.toNamespacedPath(blockedFile),
   }));
 }
+
+// fs.open
+{
+  // Extra flags should not enable trivially bypassing all restrictions.
+  // See https://github.com/nodejs/node/issues/47090.
+  assert.throws(() => {
+    fs.open(blockedFile, fs.constants.O_RDWR | 0x10000000, common.mustNotCall());
+  }, {
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemWrite',
+  });
+  assert.rejects(async () => {
+    await fs.promises.open(blockedFile, fs.constants.O_RDWR | fs.constants.O_NOFOLLOW);
+  }, {
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemWrite',
+  });
+  if (common.isWindows) {
+    // In particular, on Windows, the permission system should not blindly let
+    // code delete write-protected files.
+    const O_TEMPORARY = 0x40;
+    assert.throws(() => {
+      fs.openSync(blockedFile, fs.constants.O_RDONLY | O_TEMPORARY);
+    }, {
+      code: 'ERR_ACCESS_DENIED',
+      permission: 'FileSystemWrite'
+    });
+  }
+}