permission: do not create symlinks if target is relative #49156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
fs
anonrig
reviewed
Aug 14, 2023
lib/fs.js
Outdated
| 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))) { |
There was a problem hiding this comment.
This applies to all: If you move this line after getValidatedPath, you'd reduce the number of C++ calls, since getValidatedPath also calls toPathIfFileUrl function.
There was a problem hiding this comment.
This is tricky. I didn't find any elegant solution. It cannot be after getValidatedPath() because isAbsolute() will always be true after getValidatedPath() returns.
It we add to symlink methods |
That would be a significant breaking change. If a user really wants an absolute symlink, they can just pass in an absolute target path. |
|
@tniessen why this is a draft? |
|
@RafaelGSS Because I haven't yet figured out how to deal with non-string paths. |
tniessen
force-pushed
the
permission-no-relative-symlink-creation
branch
from
f80a7b7 to
3194e9d
Compare
|
I am giving up on non-string inputs for now. Blindly coercing to strings as in 1f64147 seems like a mistake so me, so for now, the implementation always throws on non-string inputs. I do not like contributing to how the permission model restricts legitimate applications in such ways, but I do not have energy or time to find a better solution and none has been suggested here either. |
RafaelGSS
reviewed
Oct 10, 2023
| 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.
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.
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.
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.
@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.
No blocking suggestion. Once you merge it I can create an issue to work on that.
jasnell
approved these changes
Oct 28, 2023
anonrig
reviewed
Nov 19, 2023
| 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.
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.
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.
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.
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.)
tniessen
force-pushed
the
permission-no-relative-symlink-creation
branch
from
3194e9d to
13b72a7
Compare
|
I had to rebase due to #50318. PTAL. |
|
@tniessen once you get a green CI feel free to ping me |
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
|
CI is green @RafaelGSS. It would be great if you could re-approve and add the |
RafaelGSS
approved these changes
Nov 22, 2023
RafaelGSS
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 041d435 |
targos
pushed a commit
that referenced
this pull request
Nov 23, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
martenrichter
pushed a commit
to martenrichter/node
that referenced
this pull request
Nov 26, 2023
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>
lucshi
pushed a commit
to lucshi/node
that referenced
this pull request
Nov 27, 2023
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>
Merged
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 29, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 30, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon
pushed a commit
that referenced
this pull request
Dec 11, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
UlisesGascon
pushed a commit
that referenced
this pull request
Dec 13, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon
pushed a commit
that referenced
this pull request
Dec 15, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon
pushed a commit
that referenced
this pull request
Dec 19, 2023
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: #49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
tniessen
added a commit
to tniessen/node
that referenced
this pull request
Apr 3, 2024
Commit 2000c26 added explicit handling of Buffers to fs.symlink, but not to fs.symlinkSync or fs.promises.symlink. This change adapts the latter two functions to behave like fs.symlink. Refs: nodejs#49156 Refs: nodejs#51212
nodejs-github-bot
pushed a commit
that referenced
this pull request
Apr 6, 2024
Commit 2000c26 added explicit handling of Buffers to fs.symlink, but not to fs.symlinkSync or fs.promises.symlink. This change adapts the latter two functions to behave like fs.symlink. Refs: #49156 Refs: #51212 PR-URL: #52348 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito
pushed a commit
that referenced
this pull request
May 2, 2024
Commit 2000c26 added explicit handling of Buffers to fs.symlink, but not to fs.symlinkSync or fs.promises.symlink. This change adapts the latter two functions to behave like fs.symlink. Refs: #49156 Refs: #51212 PR-URL: #52348 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito
pushed a commit
that referenced
this pull request
May 3, 2024
Commit 2000c26 added explicit handling of Buffers to fs.symlink, but not to fs.symlinkSync or fs.promises.symlink. This change adapts the latter two functions to behave like fs.symlink. Refs: #49156 Refs: #51212 PR-URL: #52348 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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
Uint8Arrayobjects 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 anUint8Array(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.)