Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

permission: fix chmod,chown,link, and lutimes #47529

Merged
merged 1 commit into from Apr 13, 2023
Merged

permission: fix chmod,chown,link, and lutimes #47529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/node_file.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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',
});
}