From 233eb626927c57e3615680e08a484ee7269ef749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 14 Mar 2023 17:38:18 +0000 Subject: [PATCH] permission: fix some vulnerabilities in fs Without this patch, any restrictions imposed by the permission model can be easily bypassed, granting full read and write access to any file. On Windows, this could even be used to delete files that are supposed to be write-protected. Fixes: https://github.com/nodejs/node/issues/47090 --- src/node_file.cc | 70 ++++++++++--------- test/parallel/test-permission-deny-fs-read.js | 15 ++++ .../parallel/test-permission-deny-fs-write.js | 29 ++++++++ 3 files changed, 80 insertions(+), 34 deletions(-) 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' + }); + } +}