FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

permission: fix some vulnerabilities in fs · nodejs/node@aa30e16 · GitHub

/ node Public

Commit aa30e16

Browse files
tniessen
authored
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: #47090 PR-URL: #47091 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 3904ef8 commit aa30e16

3 files changed

Lines changed: 80 additions & 34 deletions

File tree

‎src/node_file.cc‎

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,11 @@ using v8::HandleScope;
6464
using v8::Int32;
6565
using v8::Integer;
6666
using v8::Isolate;
67+
using v8::JustVoid;
6768
using v8::Local;
69+
using v8::Maybe;
6870
using v8::MaybeLocal;
71+
using v8::Nothing;
6972
using v8::Number;
7073
using v8::Object;
7174
using v8::ObjectTemplate;
@@ -1949,6 +1952,37 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
19491952
}
19501953
}
19511954

1955+
static inline Maybe<void> CheckOpenPermissions(Environment* env,
1956+
const BufferValue& path,
1957+
int flags) {
1958+
// These flags capture the intention of the open() call.
1959+
const int rwflags = flags & (UV_FS_O_RDONLY | UV_FS_O_WRONLY | UV_FS_O_RDWR);
1960+
1961+
// These flags have write-like side effects even with O_RDONLY, at least on
1962+
// some operating systems. On Windows, for example, O_RDONLY | O_TEMPORARY
1963+
// can be used to delete a file. Bizarre.
1964+
const int write_as_side_effect = flags & (UV_FS_O_APPEND | UV_FS_O_CREAT |
1965+
UV_FS_O_TRUNC | UV_FS_O_TEMPORARY);
1966+
1967+
// TODO(rafaelgss): it can be optimized to avoid two permission checks
1968+
auto pathView = path.ToStringView();
1969+
if (rwflags != UV_FS_O_WRONLY) {
1970+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1971+
env,
1972+
permission::PermissionScope::kFileSystemRead,
1973+
pathView,
1974+
Nothing<void>());
1975+
}
1976+
if (rwflags != UV_FS_O_RDONLY || write_as_side_effect) {
1977+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1978+
env,
1979+
permission::PermissionScope::kFileSystemWrite,
1980+
pathView,
1981+
Nothing<void>());
1982+
}
1983+
return JustVoid();
1984+
}
1985+
19521986
static void Open(const FunctionCallbackInfo<Value>& args) {
19531987
Environment* env = Environment::GetCurrent(args);
19541988

@@ -1964,22 +1998,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
19641998
CHECK(args[2]->IsInt32());
19651999
const int mode = args[2].As<Int32>()->Value();
19662000

1967-
auto pathView = path.ToStringView();
1968-
// Open can be called either in write or read
1969-
if (flags == O_RDWR) {
1970-
// TODO(rafaelgss): it can be optimized to avoid O(2*n)
1971-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1972-
env, permission::PermissionScope::kFileSystemRead, pathView);
1973-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1974-
env, permission::PermissionScope::kFileSystemWrite, pathView);
1975-
} else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
1976-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1977-
env, permission::PermissionScope::kFileSystemRead, pathView);
1978-
} else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
1979-
UV_FS_O_WRONLY)) != 0) {
1980-
THROW_IF_INSUFFICIENT_PERMISSIONS(
1981-
env, permission::PermissionScope::kFileSystemWrite, pathView);
1982-
}
2001+
if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
19832002

19842003
FSReqBase* req_wrap_async = GetReqWrap(args, 3);
19852004
if (req_wrap_async != nullptr) { // open(path, flags, mode, req)
@@ -2010,31 +2029,14 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
20102029

20112030
BufferValue path(isolate, args[0]);
20122031
CHECK_NOT_NULL(*path);
2013-
auto pathView = path.ToStringView();
2014-
THROW_IF_INSUFFICIENT_PERMISSIONS(
2015-
env, permission::PermissionScope::kFileSystemRead, pathView);
20162032

20172033
CHECK(args[1]->IsInt32());
20182034
const int flags = args[1].As<Int32>()->Value();
20192035

20202036
CHECK(args[2]->IsInt32());
20212037
const int mode = args[2].As<Int32>()->Value();
20222038

2023-
// Open can be called either in write or read
2024-
if (flags == O_RDWR) {
2025-
// TODO(rafaelgss): it can be optimized to avoid O(2*n)
2026-
THROW_IF_INSUFFICIENT_PERMISSIONS(
2027-
env, permission::PermissionScope::kFileSystemRead, pathView);
2028-
THROW_IF_INSUFFICIENT_PERMISSIONS(
2029-
env, permission::PermissionScope::kFileSystemWrite, pathView);
2030-
} else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
2031-
THROW_IF_INSUFFICIENT_PERMISSIONS(
2032-
env, permission::PermissionScope::kFileSystemRead, pathView);
2033-
} else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
2034-
UV_FS_O_WRONLY)) != 0) {
2035-
THROW_IF_INSUFFICIENT_PERMISSIONS(
2036-
env, permission::PermissionScope::kFileSystemWrite, pathView);
2037-
}
2039+
if (CheckOpenPermissions(env, path, flags).IsNothing()) return;
20382040

20392041
FSReqBase* req_wrap_async = GetReqWrap(args, 3);
20402042
if (req_wrap_async != nullptr) { // openFileHandle(path, flags, mode, req)

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,21 @@ const gid = os.userInfo().gid;
249249
fs.open(regularFile, 'r', (err) => {
250250
assert.ifError(err);
251251
});
252+
253+
// Extra flags should not enable trivially bypassing all restrictions.
254+
// See https://github.com/nodejs/node/issues/47090.
255+
assert.throws(() => {
256+
fs.openSync(blockedFile, fs.constants.O_RDONLY | fs.constants.O_NOCTTY);
257+
}, {
258+
code: 'ERR_ACCESS_DENIED',
259+
permission: 'FileSystemRead',
260+
});
261+
assert.throws(() => {
262+
fs.open(blockedFile, fs.constants.O_RDWR | 0x10000000, common.mustNotCall());
263+
}, {
264+
code: 'ERR_ACCESS_DENIED',
265+
permission: 'FileSystemRead',
266+
});
252267
}
253268

254269
// fs.opendir

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,3 +238,32 @@ const regularFile = fixtures.path('permission', 'deny', 'regular-file.md');
238238
resource: path.toNamespacedPath(blockedFile),
239239
}));
240240
}
241+
242+
// fs.open
243+
{
244+
// Extra flags should not enable trivially bypassing all restrictions.
245+
// See https://github.com/nodejs/node/issues/47090.
246+
assert.throws(() => {
247+
fs.open(blockedFile, fs.constants.O_RDWR | 0x10000000, common.mustNotCall());
248+
}, {
249+
code: 'ERR_ACCESS_DENIED',
250+
permission: 'FileSystemWrite',
251+
});
252+
assert.rejects(async () => {
253+
await fs.promises.open(blockedFile, fs.constants.O_RDWR | fs.constants.O_NOFOLLOW);
254+
}, {
255+
code: 'ERR_ACCESS_DENIED',
256+
permission: 'FileSystemWrite',
257+
});
258+
if (common.isWindows) {
259+
// In particular, on Windows, the permission system should not blindly let
260+
// code delete write-protected files.
261+
const O_TEMPORARY = 0x40;
262+
assert.throws(() => {
263+
fs.openSync(blockedFile, fs.constants.O_RDONLY | O_TEMPORARY);
264+
}, {
265+
code: 'ERR_ACCESS_DENIED',
266+
permission: 'FileSystemWrite'
267+
});
268+
}
269+
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL