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

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Aug 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.)

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 13, 2023
@anonrig anonrig self-requested a review August 13, 2023 23:28
@RafaelGSS RafaelGSS self-requested a review August 14, 2023 00:35
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

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))) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tricky. I didn't find any elegant solution. It cannot be after getValidatedPath() because isAbsolute() will always be true after getValidatedPath() returns.

@tniessen tniessen marked this pull request as draft August 14, 2023 09:11
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@LiviaMedeiros
Copy link
Contributor

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.

It we add to symlink methods relative option that is set to false by default, would it be enough to assume that users who don't enable it want absolute symlink?

@tniessen
Copy link
Member Author

It we add to symlink methods relative option that is set to false by default, would it be enough to assume that users who don't enable it want absolute symlink?

That would be a significant breaking change. If a user really wants an absolute symlink, they can just pass in an absolute target path.

@RafaelGSS
Copy link
Member

@tniessen why this is a draft?

@tniessen
Copy link
Member Author

@RafaelGSS Because I haven't yet figured out how to deal with non-string paths.

@RafaelGSS RafaelGSS added the permission Issues and PRs related to the Permission Model label Sep 7, 2023
@tniessen tniessen force-pushed the permission-no-relative-symlink-creation branch from f80a7b7 to 3194e9d Compare October 10, 2023 10:04
@tniessen tniessen marked this pull request as ready for review October 10, 2023 10:09
@tniessen
Copy link
Member Author

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.

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.

@tniessen tniessen added the review wanted PRs that need reviews. label Oct 11, 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))) {
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.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

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 tniessen force-pushed the permission-no-relative-symlink-creation branch from 3194e9d to 13b72a7 Compare November 21, 2023 21:49
@tniessen
Copy link
Member Author

I had to rebase due to #50318. PTAL.

@RafaelGSS
Copy link
Member

@tniessen once you get a green CI feel free to ping me

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

CI is green @RafaelGSS. It would be great if you could re-approve and add the commit-queue label 🙂

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit 041d435 into nodejs:main Nov 22, 2023
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

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>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
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>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
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
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants