From f80a7b767ef4bde6302ece020711f79c3af550f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 11 Aug 2023 11:52:10 +0000 Subject: [PATCH] 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. --- lib/fs.js | 22 ++++++++++++++++ lib/internal/fs/promises.js | 14 +++++++++++ .../test-permission-fs-symlink-relative.js | 25 +++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 test/parallel/test-permission-fs-symlink-relative.js diff --git a/lib/fs.js b/lib/fs.js index 34c58e22ad54a4..4f4246d19b56c7 100644 --- a/lib/fs.js +++ b/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 { } = require('internal/validators'); const { readFileSyncUtf8 } = require('internal/fs/read/utf8'); +const permission = require('internal/process/permission'); + let truncateWarn = true; let fs; @@ -1762,6 +1766,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 (!isAbsolute(toPathIfFileURL(target))) { + callback(new ERR_ACCESS_DENIED('relative symbolic link target')); + return; + } + } + target = getValidatedPath(target, 'target'); path = getValidatedPath(path); @@ -1818,6 +1831,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 (!isAbsolute(toPathIfFileURL(target))) { + throw new ERR_ACCESS_DENIED('relative symbolic link target'); + } + } + target = getValidatedPath(target, 'target'); path = getValidatedPath(path); const flags = stringToSymlinkType(type); diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 8f56baf77505f1..0f934ab24f198b 100644 --- a/lib/internal/fs/promises.js +++ b/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, @@ -84,6 +85,8 @@ const { validateString, } = require('internal/validators'); const pathModule = require('path'); +const { isAbsolute } = pathModule; +const { toPathIfFileURL } = require('internal/url'); const { kEmptyObject, lazyDOMException, @@ -96,6 +99,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'); @@ -877,6 +882,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 (!isAbsolute(toPathIfFileURL(target))) { + throw new ERR_ACCESS_DENIED('relative symbolic link target'); + } + } + target = getValidatedPath(target, 'target'); path = getValidatedPath(path); return binding.symlink(preprocessSymlinkDestination(target, type, path), diff --git a/test/parallel/test-permission-fs-symlink-relative.js b/test/parallel/test-permission-fs-symlink-relative.js new file mode 100644 index 00000000000000..2fef2898939803 --- /dev/null +++ b/test/parallel/test-permission-fs-symlink-relative.js @@ -0,0 +1,25 @@ +// 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 target of ['a', './b/c', '../d', 'e/../f', 'C:drive-relative', 'ntfs:alternate']) { + 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); + } +}