Skip to content

Commit 8bcf0a4

Browse files
RafaelGSSRafaelGSS
committed
permission: fix chmod,chown improve fs coverage
Signed-off-by: RafaelGSS <[email protected]> PR-URL: #47529 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
1 parent c02a7e7 commit 8bcf0a4

File tree

5 files changed

+176
-16
lines changed

5 files changed

+176
-16
lines changed

‎src/node_file.cc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,8 +1338,18 @@ static void Link(const FunctionCallbackInfo<Value>& args) {
13381338
BufferValue src(isolate, args[0]);
13391339
CHECK_NOT_NULL(*src);
13401340

1341+
const auto src_view = src.ToStringView();
1342+
// To avoid bypass the link target should be allowed to read and write
1343+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1344+
env, permission::PermissionScope::kFileSystemRead, src_view);
1345+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1346+
env, permission::PermissionScope::kFileSystemWrite, src_view);
1347+
13411348
BufferValue dest(isolate, args[1]);
13421349
CHECK_NOT_NULL(*dest);
1350+
const auto dest_view = dest.ToStringView();
1351+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1352+
env, permission::PermissionScope::kFileSystemWrite, dest_view);
13431353

13441354
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
13451355
if (req_wrap_async != nullptr) { // link(src, dest, req)
@@ -2422,6 +2432,8 @@ static void Chmod(const FunctionCallbackInfo<Value>& args) {
24222432

24232433
BufferValue path(env->isolate(), args[0]);
24242434
CHECK_NOT_NULL(*path);
2435+
THROW_IF_INSUFFICIENT_PERMISSIONS(
2436+
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
24252437

24262438
CHECK(args[1]->IsInt32());
24272439
int mode = args[1].As<Int32>()->Value();
@@ -2485,6 +2497,8 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
24852497

24862498
BufferValue path(env->isolate(), args[0]);
24872499
CHECK_NOT_NULL(*path);
2500+
THROW_IF_INSUFFICIENT_PERMISSIONS(
2501+
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
24882502

24892503
CHECK(IsSafeJsInt(args[1]));
24902504
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());
@@ -2551,6 +2565,8 @@ static void LChown(const FunctionCallbackInfo<Value>& args) {
25512565

25522566
BufferValue path(env->isolate(), args[0]);
25532567
CHECK_NOT_NULL(*path);
2568+
THROW_IF_INSUFFICIENT_PERMISSIONS(
2569+
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
25542570

25552571
CHECK(IsSafeJsInt(args[1]));
25562572
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());
@@ -2646,6 +2662,8 @@ static void LUTimes(const FunctionCallbackInfo<Value>& args) {
26462662

26472663
BufferValue path(env->isolate(), args[0]);
26482664
CHECK_NOT_NULL(*path);
2665+
THROW_IF_INSUFFICIENT_PERMISSIONS(
2666+
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
26492667

26502668
CHECK(args[1]->IsNumber());
26512669
const double atime = args[1].As<Number>()->Value();

‎test/fixtures/permission/fs-read.js‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,11 @@ const common = require('../../common');
55
const assert = require('assert');
66
const fs = require('fs');
77
const path = require('path');
8-
const os = require('os');
98

109
const blockedFile = process.env.BLOCKEDFILE;
1110
const blockedFolder = process.env.BLOCKEDFOLDER;
1211
const allowedFolder = process.env.ALLOWEDFOLDER;
1312
const regularFile = __filename;
14-
const uid = os.userInfo().uid;
15-
const gid = os.userInfo().gid;
1613

1714
// fs.readFile
1815
{
@@ -106,19 +103,6 @@ const gid = os.userInfo().gid;
106103
});
107104
}
108105

109-
// fs.chownSync (should not bypass)
110-
{
111-
assert.throws(() => {
112-
// This operation will work fine
113-
fs.chownSync(blockedFile, uid, gid);
114-
fs.readFileSync(blockedFile);
115-
}, common.expectsError({
116-
code: 'ERR_ACCESS_DENIED',
117-
permission: 'FileSystemRead',
118-
resource: path.toNamespacedPath(blockedFile),
119-
}));
120-
}
121-
122106
// fs.copyFile
123107
{
124108
assert.throws(() => {

‎test/fixtures/permission/fs-symlink-target-write.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER;
3131
permission: 'FileSystemWrite',
3232
resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')),
3333
}));
34+
assert.throws(() => {
35+
fs.link(path.join(readOnlyFolder, 'file'), path.join(readWriteFolder, 'link-to-read-only'), (err) => {
36+
assert.ifError(err);
37+
});
38+
}, common.expectsError({
39+
code: 'ERR_ACCESS_DENIED',
40+
permission: 'FileSystemWrite',
41+
resource: path.toNamespacedPath(path.join(readOnlyFolder, 'file')),
42+
}));
3443

3544
// App will be able to symlink to a writeOnlyFolder
3645
fs.symlink(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write'), 'file', (err) => {
@@ -48,6 +57,21 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER;
4857
// App will be able to write to the symlink
4958
fs.writeFile(path.join(writeOnlyFolder, 'link-to-read-write'), 'some content', common.mustSucceed());
5059
});
60+
fs.link(path.join(readWriteFolder, 'file'), path.join(writeOnlyFolder, 'link-to-read-write2'), (err) => {
61+
assert.ifError(err);
62+
// App will won't be able to read the link
63+
assert.throws(() => {
64+
fs.readFile(path.join(writeOnlyFolder, 'link-to-read-write2'), (err) => {
65+
assert.ifError(err);
66+
});
67+
}, common.expectsError({
68+
code: 'ERR_ACCESS_DENIED',
69+
permission: 'FileSystemRead',
70+
}));
71+
72+
// App will be able to write to the link
73+
fs.writeFile(path.join(writeOnlyFolder, 'link-to-read-write2'), 'some content', common.mustSucceed());
74+
});
5175

5276
// App won't be able to symlink to a readOnlyFolder
5377
assert.throws(() => {
@@ -59,4 +83,13 @@ const writeOnlyFolder = process.env.WRITEONLYFOLDER;
5983
permission: 'FileSystemWrite',
6084
resource: path.toNamespacedPath(path.join(readOnlyFolder, 'link-to-read-only')),
6185
}));
86+
assert.throws(() => {
87+
fs.link(path.join(readWriteFolder, 'file'), path.join(readOnlyFolder, 'link-to-read-only'), (err) => {
88+
assert.ifError(err);
89+
});
90+
}, common.expectsError({
91+
code: 'ERR_ACCESS_DENIED',
92+
permission: 'FileSystemWrite',
93+
resource: path.toNamespacedPath(path.join(readOnlyFolder, 'link-to-read-only')),
94+
}));
6295
}

‎test/fixtures/permission/fs-symlink.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,14 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK;
8080
code: 'ERR_ACCESS_DENIED',
8181
permission: 'FileSystemWrite',
8282
}));
83+
assert.throws(() => {
84+
fs.link(regularFile, blockedFolder + '/asdf', (err) => {
85+
assert.ifError(err);
86+
});
87+
}, common.expectsError({
88+
code: 'ERR_ACCESS_DENIED',
89+
permission: 'FileSystemWrite',
90+
}));
8391

8492
// App won't be able to symlink BLOCKEDFILE to REGULARDIR
8593
assert.throws(() => {
@@ -90,4 +98,12 @@ const symlinkFromBlockedFile = process.env.EXISTINGSYMLINK;
9098
code: 'ERR_ACCESS_DENIED',
9199
permission: 'FileSystemRead',
92100
}));
101+
assert.throws(() => {
102+
fs.link(blockedFile, path.join(__dirname, '/asdf'), (err) => {
103+
assert.ifError(err);
104+
});
105+
}, common.expectsError({
106+
code: 'ERR_ACCESS_DENIED',
107+
permission: 'FileSystemRead',
108+
}));
93109
}

‎test/fixtures/permission/fs-write.js‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,17 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder);
108108
}));
109109
}
110110

111+
// fs.lutimes
112+
{
113+
assert.throws(() => {
114+
fs.lutimes(blockedFile, new Date(), new Date(), () => {});
115+
}, common.expectsError({
116+
code: 'ERR_ACCESS_DENIED',
117+
permission: 'FileSystemWrite',
118+
resource: path.toNamespacedPath(blockedFile),
119+
}));
120+
}
121+
111122
// fs.mkdir
112123
{
113124
assert.throws(() => {
@@ -270,3 +281,101 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder);
270281
});
271282
}
272283
}
284+
285+
// fs.chmod
286+
{
287+
assert.throws(() => {
288+
fs.chmod(blockedFile, 0o755, common.mustNotCall());
289+
}, {
290+
code: 'ERR_ACCESS_DENIED',
291+
permission: 'FileSystemWrite',
292+
});
293+
assert.rejects(async () => {
294+
await fs.promises.chmod(blockedFile, 0o755);
295+
}, {
296+
code: 'ERR_ACCESS_DENIED',
297+
permission: 'FileSystemWrite',
298+
});
299+
}
300+
301+
// fs.lchmod
302+
{
303+
if (common.isOSX) {
304+
assert.throws(() => {
305+
fs.lchmod(blockedFile, 0o755, common.mustNotCall());
306+
}, {
307+
code: 'ERR_ACCESS_DENIED',
308+
permission: 'FileSystemWrite',
309+
});
310+
assert.rejects(async () => {
311+
await fs.promises.lchmod(blockedFile, 0o755);
312+
}, {
313+
code: 'ERR_ACCESS_DENIED',
314+
permission: 'FileSystemWrite',
315+
});
316+
}
317+
}
318+
319+
// fs.appendFile
320+
{
321+
assert.throws(() => {
322+
fs.appendFile(blockedFile, 'new data', common.mustNotCall());
323+
}, {
324+
code: 'ERR_ACCESS_DENIED',
325+
permission: 'FileSystemWrite',
326+
});
327+
assert.rejects(async () => {
328+
await fs.promises.appendFile(blockedFile, 'new data');
329+
}, {
330+
code: 'ERR_ACCESS_DENIED',
331+
permission: 'FileSystemWrite',
332+
});
333+
}
334+
335+
// fs.chown
336+
{
337+
assert.throws(() => {
338+
fs.chown(blockedFile, 1541, 999, common.mustNotCall());
339+
}, {
340+
code: 'ERR_ACCESS_DENIED',
341+
permission: 'FileSystemWrite',
342+
});
343+
assert.rejects(async () => {
344+
await fs.promises.chown(blockedFile, 1541, 999);
345+
}, {
346+
code: 'ERR_ACCESS_DENIED',
347+
permission: 'FileSystemWrite',
348+
});
349+
}
350+
351+
// fs.lchown
352+
{
353+
assert.throws(() => {
354+
fs.lchown(blockedFile, 1541, 999, common.mustNotCall());
355+
}, {
356+
code: 'ERR_ACCESS_DENIED',
357+
permission: 'FileSystemWrite',
358+
});
359+
assert.rejects(async () => {
360+
await fs.promises.lchown(blockedFile, 1541, 999);
361+
}, {
362+
code: 'ERR_ACCESS_DENIED',
363+
permission: 'FileSystemWrite',
364+
});
365+
}
366+
367+
// fs.link
368+
{
369+
assert.throws(() => {
370+
fs.link(blockedFile, path.join(blockedFolder, '/linked'), common.mustNotCall());
371+
}, {
372+
code: 'ERR_ACCESS_DENIED',
373+
permission: 'FileSystemWrite',
374+
});
375+
assert.rejects(async () => {
376+
await fs.promises.link(blockedFile, path.join(blockedFolder, '/linked'));
377+
}, {
378+
code: 'ERR_ACCESS_DENIED',
379+
permission: 'FileSystemWrite',
380+
});
381+
}

0 commit comments

Comments
 (0)