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.

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

permission: do not create symlinks if target is relative #49156

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const {
} = constants;

const pathModule = require('path');
const { isAbsolute } = pathModule;
const { isArrayBufferView } = require('internal/util/types');

const binding = internalBinding('fs');
Expand All @@ -68,6 +69,7 @@ const { Buffer } = require('buffer');
const {
aggregateTwoErrors,
codes: {
ERR_ACCESS_DENIED,
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
},
Expand Down Expand Up @@ -143,6 +145,8 @@ const {
kValidateObjectAllowNullable,
} = require('internal/validators');

const permission = require('internal/process/permission');

let truncateWarn = true;
let fs;

Expand Down Expand Up @@ -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))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could check if it's a Buffer and default it to utf8 to check if isAbsolute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On POSIX, that generally works because POSIX only checks the first byte (not character) of the path to see if it is absolute. On Windows, I am not so sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have played a bit on Windows today (I was fixing another issue) and I couldn't reproduce the case you've mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RafaelGSS Is this a blocking suggestion? I think it'd be appropriate to add support for Uint8Array paths in a follow-up PR if desired. I myself am not confident that I'd do so without introducing new issues on Windows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking suggestion. Once you merge it I can create an issue to work on that.

callback(new ERR_ACCESS_DENIED('relative symbolic link target'));
return;
}
}

target = getValidatedPath(target, 'target');
path = getValidatedPath(path);

Expand Down Expand Up @@ -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))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we move this to C++?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to do so as part of this bug fix? Is there a potential security issue here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am moving each of these functions to C++. Adding more JS, means we will move it to C++ in a different PR. It just bloats the git history.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something here. This bug fix has to be applied in either case. I don't think moving this to C++ would reduce the size of the git diff either. Bug fixes should be as small as possible so that they can be backported easily.

If you want to request changes on this PR and incorporate the fix in your own PR, that's fine by me, but moving this logic to C++ as part of a bug fix, absent of any known issues with the JavaScript implementation, is not something that I'll spend time on.

Otherwise, I'll merge this soon-ish because bug fixes still have priority over most other changes.

throw new ERR_ACCESS_DENIED('relative symbolic link target');
}
}

target = getValidatedPath(target, 'target');
path = getValidatedPath(path);

Expand Down
14 changes: 14 additions & 0 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -86,6 +87,8 @@ const {
kValidateObjectAllowNullable,
} = require('internal/validators');
const pathModule = require('path');
const { isAbsolute } = pathModule;
const { toPathIfFileURL } = require('internal/url');
const {
kEmptyObject,
lazyDOMException,
Expand All @@ -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');
Expand Down Expand Up @@ -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(
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-permission-fs-symlink-relative.js
Original file line number Diff line number Diff line change
@@ -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());
}
}
}