Skip to content

Commit

Permalink
permission: fix chmod,chown improve fs coverage
Browse files Browse the repository at this point in the history 
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #47529
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
  • Loading branch information
@RafaelGSS
RafaelGSS authored Apr 13, 2023
1 parent 6fb74c7 commit 1323992
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 16 deletions.
18 changes: 18 additions & 0 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1338,8 +1338,18 @@ static void Link(const FunctionCallbackInfo<Value>& 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)
Expand Down Expand Up @@ -2422,6 +2432,8 @@ static void Chmod(const FunctionCallbackInfo<Value>& 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<Int32>()->Value();
Expand Down Expand Up @@ -2485,6 +2497,8 @@ static void Chown(const FunctionCallbackInfo<Value>& 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<uv_uid_t>(args[1].As<Integer>()->Value());
Expand Down Expand Up @@ -2551,6 +2565,8 @@ static void LChown(const FunctionCallbackInfo<Value>& 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<uv_uid_t>(args[1].As<Integer>()->Value());
Expand Down Expand Up @@ -2646,6 +2662,8 @@ static void LUTimes(const FunctionCallbackInfo<Value>& 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<Number>()->Value();
Expand Down
16 changes: 0 additions & 16 deletions test/fixtures/permission/fs-read.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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(() => {
Expand Down
33 changes: 33 additions & 0 deletions test/fixtures/permission/fs-symlink-target-write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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(() => {
Expand All @@ -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')),
}));
}
16 changes: 16 additions & 0 deletions test/fixtures/permission/fs-symlink.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -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',
}));
}
109 changes: 109 additions & 0 deletions test/fixtures/permission/fs-write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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',
});
}

0 comments on commit 1323992

Please sign in to comment.