Skip to content

Commit

Permalink
permission: fix some vulnerabilities in fs
Browse files Browse the repository at this point in the history 
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: #47090
PR-URL: #47091
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
@tniessen
tniessen authored Mar 19, 2023
1 parent 3904ef8 commit aa30e16
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 34 deletions.
70 changes: 36 additions & 34 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-permission-deny-fs-read.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-permission-deny-fs-write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
});
}
}

0 comments on commit aa30e16

Please sign in to comment.