permission: fix chmod,chown improve fs coverage

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: https://github.com/nodejs/node/pull/47529
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

5 files changed, 176 insertions, 16 deletions

diff --git a/src/node_file.cc b/src/node_file.cc
index 43f1922b96..3eaa46e9c2 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -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)
@@ -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();
@@ -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());
@@ -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());
@@ -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();
diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js
index e7551c8422..2c03232968 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 dfe1386ba4..2783e6f82f 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 b1c5121a4f..430b2f9b09 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 ecbd91999e..6164736d1a 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',
+  });
+}