From 1dfdd6fab06316e0333e3dbaec68d2d1486364ca Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Thu, 13 Apr 2023 11:06:44 -0300 Subject: [PATCH] permission: fix chmod,chown improve fs coverage Signed-off-by: RafaelGSS PR-URL: https://github.com/nodejs/node/pull/47529 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Marco Ippolito --- src/node_file.cc | 18 +++ test/fixtures/permission/fs-read.js | 16 --- .../permission/fs-symlink-target-write.js | 33 ++++++ test/fixtures/permission/fs-symlink.js | 16 +++ test/fixtures/permission/fs-write.js | 109 ++++++++++++++++++ 5 files changed, 176 insertions(+), 16 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 43f1922b96755f..3eaa46e9c2b0d1 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1338,8 +1338,18 @@ static void Link(const FunctionCallbackInfo& args) { BufferValue src(isolate, args[0]); CHECK_NOT_NULL(*src); + const auto src_view = src.ToStringView(); + // To avoid bypass the link target should be allowed to read and write + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemRead, src_view); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, src_view); + BufferValue dest(isolate, args[1]); CHECK_NOT_NULL(*dest); + const auto dest_view = dest.ToStringView(); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, dest_view); FSReqBase* req_wrap_async = GetReqWrap(args, 2); if (req_wrap_async != nullptr) { // link(src, dest, req) @@ -2422,6 +2432,8 @@ static void Chmod(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); CHECK(args[1]->IsInt32()); int mode = args[1].As()->Value(); @@ -2485,6 +2497,8 @@ static void Chown(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); CHECK(IsSafeJsInt(args[1])); const uv_uid_t uid = static_cast(args[1].As()->Value()); @@ -2551,6 +2565,8 @@ static void LChown(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); CHECK(IsSafeJsInt(args[1])); const uv_uid_t uid = static_cast(args[1].As()->Value()); @@ -2646,6 +2662,8 @@ static void LUTimes(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); CHECK(args[1]->IsNumber()); const double atime = args[1].As()->Value(); diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js index e7551c84229008..2c032329686d4c 100644 --- a/test/fixtures/permission/fs-read.js +++ b/test/fixtures/permission/fs-read.js @@ -5,14 +5,11 @@ const common = require('../../common'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); -const os = require('os'); const blockedFile = process.env.BLOCKEDFILE; const blockedFolder = process.env.BLOCKEDFOLDER; const allowedFolder = process.env.ALLOWEDFOLDER; const regularFile = __filename; -const uid = os.userInfo().uid; -const gid = os.userInfo().gid; // fs.readFile { @@ -106,19 +103,6 @@ const gid = os.userInfo().gid; }); } -// fs.chownSync (should not bypass) -{ - assert.throws(() => { - // This operation will work fine - fs.chownSync(blockedFile, uid, gid); - fs.readFileSync(blockedFile); - }, common.expectsError({ - code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemRead', - resource: path.toNamespacedPath(blockedFile), - })); -} - // fs.copyFile { assert.throws(() => { diff --git a/test/fixtures/permission/fs-symlink-target-write.js b/test/fixtures/permission/fs-symlink-target-write.js index dfe1386ba49daf..2783e6f82f397c 100644 --- a/test/fixtures/permission/fs-symlink-target-write.js +++ b/test/fixtures/permission/fs-symlink-target-write.js @@ -31,6 +31,15 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; permission: 'FileSystemWrite', resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')), })); + assert.throws(() => { + fs.link(path.join(readOnlyFolder, 'file'), path.join(readWriteFolder, 'link-to-read-only'), (err) => { + assert.ifError(err); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')), + })); // App will be able to symlink to a writeOnlyFolder fs.symlink(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write'), 'file', (err) => { @@ -48,6 +57,21 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; // App will be able to write to the symlink fs.writeFile(path.join(writeOnlyFolder, 'link-to-read-write'), 'some content', common.mustSucceed()); }); + fs.link(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write2'), (err) => { + assert.ifError(err); + // App will won't be able to read the link + assert.throws(() => { + fs.readFile(path.join(writeOnlyFolder, 'link-to-read-write2'), (err) => { + assert.ifError(err); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemRead', + })); + + // App will be able to write to the link + fs.writeFile(path.join(writeOnlyFolder, 'link-to-read-write2'), 'some content', common.mustSucceed()); + }); // App won't be able to symlink to a readOnlyFolder assert.throws(() => { @@ -59,4 +83,13 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER; permission: 'FileSystemWrite', resource: path.toNamespacedPath(path.join(readOnlyFolder, 'link-to-read-only')), })); + assert.throws(() => { + fs.link(path.join(readWriteFolder, 'file'), path.join(readOnlyFolder, 'link-to-read-only'), (err) => { + assert.ifError(err); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + resource: path.toNamespacedPath(path.join(readOnlyFolder, 'link-to-read-only')), + })); } diff --git a/test/fixtures/permission/fs-symlink.js b/test/fixtures/permission/fs-symlink.js index b1c5121a4ff809..430b2f9b09574c 100644 --- a/test/fixtures/permission/fs-symlink.js +++ b/test/fixtures/permission/fs-symlink.js @@ -80,6 +80,14 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', })); + assert.throws(() => { + fs.link(regularFile, blockedFolder + '/asdf', (err) => { + assert.ifError(err); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + })); // App won't be able to symlink BLOCKEDFILE to REGULARDIR assert.throws(() => { @@ -90,4 +98,12 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK; code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', })); + assert.throws(() => { + fs.link(blockedFile, path.join(__dirname, '/asdf'), (err) => { + assert.ifError(err); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemRead', + })); } diff --git a/test/fixtures/permission/fs-write.js b/test/fixtures/permission/fs-write.js index ecbd91999ec6a9..6164736d1a06ab 100644 --- a/test/fixtures/permission/fs-write.js +++ b/test/fixtures/permission/fs-write.js @@ -108,6 +108,17 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); })); } +// fs.lutimes +{ + assert.throws(() => { + fs.lutimes(blockedFile, new Date(), new Date(), () => {}); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + resource: path.toNamespacedPath(blockedFile), + })); +} + // fs.mkdir { assert.throws(() => { @@ -270,3 +281,101 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); }); } } + +// fs.chmod +{ + assert.throws(() => { + fs.chmod(blockedFile, 0o755, common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.chmod(blockedFile, 0o755); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); +} + +// fs.lchmod +{ + if (common.isOSX) { + assert.throws(() => { + fs.lchmod(blockedFile, 0o755, common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.lchmod(blockedFile, 0o755); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + } +} + +// fs.appendFile +{ + assert.throws(() => { + fs.appendFile(blockedFile, 'new data', common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.appendFile(blockedFile, 'new data'); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); +} + +// fs.chown +{ + assert.throws(() => { + fs.chown(blockedFile, 1541, 999, common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.chown(blockedFile, 1541, 999); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); +} + +// fs.lchown +{ + assert.throws(() => { + fs.lchown(blockedFile, 1541, 999, common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.lchown(blockedFile, 1541, 999); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); +} + +// fs.link +{ + assert.throws(() => { + fs.link(blockedFile, path.join(blockedFolder, '/linked'), common.mustNotCall()); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); + assert.rejects(async () => { + await fs.promises.link(blockedFile, path.join(blockedFolder, '/linked')); + }, { + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + }); +}