permission: do not create symlinks if target is relative

The permission model's security guarantees fall apart in the presence of
relative symbolic links. When an application attempts to create a
relative symlink, the permission model currently resolves the relative
path into an absolute path based on the process's current working
directory, checks whether the process has the relevant permissions, and
then creates the symlink using the absolute target path. This behavior
is plainly incorrect for two reasons:

1. The target path should never be resolved relative to the current
   working directory. If anything, it should be resolved relative to the
   symlink's location. (Of course, there is one insane exception to this
   rule: on Windows, each process has a current working directory per
   drive, and symlinks can be created with a target path relative to the
   current working directory of a specific drive. In that case, the
   relative path will be resolved relative to the current working
   directory for the respective drive, and the symlink will be created
   on disk with the resulting absolute path. Other relative symlinks
   will be stored as-is.)
2. Silently creating an absolute symlink when the user requested a
   relative symlink is wrong. The user may (or may not) rely on the
   symlink being relative. For example, npm heavily relies on relative
   symbolic links such that node_modules directories can be moved around
   without breaking.

Because we don't know the user's intentions, we don't know if creating
an absolute symlink instead of a relative symlink is acceptable. This
patch prevents the faulty behavior by not (incorrectly) resolving
relative symlink targets when the permission model is enabled, and by
instead simply refusing the create any relative symlinks.

The fs APIs accept Uint8Array objects for paths to be able to handle
arbitrary file name charsets, however, checking whether such an object
represents a relative part in a reliable and portable manner is tricky.
Other parts of the permission model incorrectly convert such objects to
strings and then back to an Uint8Array (see 1f64147),
however, for now, this bug fix will simply throw on non-string symlink
targets when the permission model is enabled. (The permission model
already breaks existing applications in various ways, so this shouldn't
be too dramatic.)

PR-URL: nodejs#49156
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: tniessen

lib/fs.js

@@ -58,6 +58,7 @@ const {
 } = constants;
 
 const pathModule = require('path');
+const { isAbsolute } = pathModule;
 const { isArrayBufferView } = require('internal/util/types');
 
 const binding = internalBinding('fs');
@@ -68,6 +69,7 @@ const { Buffer } = require('buffer');
 const {
   aggregateTwoErrors,
   codes: {
+    ERR_ACCESS_DENIED,
     ERR_FS_FILE_TOO_LARGE,
     ERR_INVALID_ARG_VALUE,
   },
@@ -143,6 +145,8 @@ const {
   kValidateObjectAllowNullable,
 } = require('internal/validators');
 
+const permission = require('internal/process/permission');
+
 let truncateWarn = true;
 let fs;
 
@@ -1742,6 +1746,15 @@ function symlink(target, path, type_, callback_) {
   const type = (typeof type_ === 'string' ? type_ : null);
   const callback = makeCallback(arguments[arguments.length - 1]);
 
+  if (permission.isEnabled()) {
+    // The permission model's security guarantees fall apart in the presence of
+    // relative symbolic links. Thus, we have to prevent their creation.
+    if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) {
+      callback(new ERR_ACCESS_DENIED('relative symbolic link target'));
+      return;
+    }
+  }
+
   target = getValidatedPath(target, 'target');
   path = getValidatedPath(path);
 
@@ -1798,6 +1811,15 @@ function symlinkSync(target, path, type) {
       type = 'dir';
     }
   }
+
+  if (permission.isEnabled()) {
+    // The permission model's security guarantees fall apart in the presence of
+    // relative symbolic links. Thus, we have to prevent their creation.
+    if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) {
+      throw new ERR_ACCESS_DENIED('relative symbolic link target');
+    }
+  }
+
   target = getValidatedPath(target, 'target');
   path = getValidatedPath(path);
 

lib/internal/fs/promises.js

@@ -33,6 +33,7 @@ const { Buffer } = require('buffer');
 
 const {
   codes: {
+    ERR_ACCESS_DENIED,
     ERR_FS_FILE_TOO_LARGE,
     ERR_INVALID_ARG_VALUE,
     ERR_INVALID_STATE,
@@ -86,6 +87,8 @@ const {
   kValidateObjectAllowNullable,
 } = require('internal/validators');
 const pathModule = require('path');
+const { isAbsolute } = pathModule;
+const { toPathIfFileURL } = require('internal/url');
 const {
   kEmptyObject,
   lazyDOMException,
@@ -98,6 +101,8 @@ const nonNativeWatcher = require('internal/fs/recursive_watch');
 const { isIterable } = require('internal/streams/utils');
 const assert = require('internal/assert');
 
+const permission = require('internal/process/permission');
+
 const kHandle = Symbol('kHandle');
 const kFd = Symbol('kFd');
 const kRefs = Symbol('kRefs');
@@ -974,6 +979,15 @@ async function symlink(target, path, type_) {
       type = 'file';
     }
   }
+
+  if (permission.isEnabled()) {
+    // The permission model's security guarantees fall apart in the presence of
+    // relative symbolic links. Thus, we have to prevent their creation.
+    if (typeof target !== 'string' || !isAbsolute(toPathIfFileURL(target))) {
+      throw new ERR_ACCESS_DENIED('relative symbolic link target');
+    }
+  }
+
   target = getValidatedPath(target, 'target');
   path = getValidatedPath(path);
   return await PromisePrototypeThen(

test/parallel/test-permission-fs-symlink-relative.js

@@ -0,0 +1,27 @@
+// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=*
+'use strict';
+
+const common = require('../common');
+common.skipIfWorker();
+
+const assert = require('assert');
+const { symlinkSync, symlink, promises: { symlink: symlinkAsync } } = require('fs');
+
+const error = {
+  code: 'ERR_ACCESS_DENIED',
+  message: /relative symbolic link target/,
+};
+
+for (const targetString of ['a', './b/c', '../d', 'e/../f', 'C:drive-relative', 'ntfs:alternate']) {
+  for (const target of [targetString, Buffer.from(targetString)]) {
+    for (const path of [__filename, __dirname, process.execPath]) {
+      assert.throws(() => symlinkSync(target, path), error);
+      symlink(target, path, common.mustCall((err) => {
+        assert(err);
+        assert.strictEqual(err.code, error.code);
+        assert.match(err.message, error.message);
+      }));
+      assert.rejects(() => symlinkAsync(target, path), error).then(common.mustCall());
+    }
+  }
+}