-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we move this to C++? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
|
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()); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.