diff --git a/src/node_file.cc b/src/node_file.cc index de623b598db348..9e0083d10a2f87 100644 --- a/src/node_file.cc +++ b/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& args) { } } +static inline Maybe 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()); + } + if (rwflags != UV_FS_O_RDONLY || write_as_side_effect) { + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, + permission::PermissionScope::kFileSystemWrite, + pathView, + Nothing()); + } + return JustVoid(); +} + static void Open(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1964,22 +1998,7 @@ static void Open(const FunctionCallbackInfo& args) { CHECK(args[2]->IsInt32()); const int mode = args[2].As()->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,9 +2029,6 @@ static void OpenFileHandle(const FunctionCallbackInfo& 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()->Value(); @@ -2020,21 +2036,7 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { CHECK(args[2]->IsInt32()); const int mode = args[2].As()->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) diff --git a/test/parallel/test-permission-deny-fs-read.js b/test/parallel/test-permission-deny-fs-read.js index 0f918acffd771d..8a5caa5989b702 100644 --- a/test/parallel/test-permission-deny-fs-read.js +++ b/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 diff --git a/test/parallel/test-permission-deny-fs-write.js b/test/parallel/test-permission-deny-fs-write.js index 1f0d2997be6e31..a146ec38f8d15b 100644 --- a/test/parallel/test-permission-deny-fs-write.js +++ b/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' + }); + } +}