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

Non-root owned symlink causes install failure #3100

Closed
dmnks opened this issue May 15, 2024 · 8 comments
Closed

Non-root owned symlink causes install failure #3100

dmnks opened this issue May 15, 2024 · 8 comments
Assignees
Labels

Comments

@dmnks
Copy link
Contributor

dmnks commented May 15, 2024

Describe the bug
Installing a package that ships a file in a directory symlink that's owned by a non-root user results in a cpio unpacking error.

UPDATE:
Failing is expected here of course, the error message can be quite cryptic though.

To Reproduce
Steps to reproduce the behavior:

  1. useradd user
  2. chown -h user /lib
  3. dnf install -y kernel-core

Expected behavior
Better error message.

Output

error: unpacking of archive failed on file /lib/modules/6.8.9-300.fc40.x86_64/build;6644c9d4: cpio: open failed - Not a directory

Environment

  • OS / Distribution: Fedora 40
  • Version rpm-4.19.1.1

Additional context
Filed originally in RH Jira: https://issues.redhat.com/browse/RHEL-33393. The reporter has suggested the culprit in the second comment in that issue.

@ffesti
Copy link
Contributor

ffesti commented Jun 18, 2024

I guess this is very much intentional as the owner may send RPM to overwrite random files otherwise. This is the very case we did re-write the fsm for.

This still needs to give a sane error message, giving the user a fighting chance to find out what's the issue.

@ffesti ffesti self-assigned this Jun 18, 2024
@pmatilai
Copy link
Member

Yup, we can't follow non-root owned symlinks, the main issue is error message.

Ideally we'd toss the bad link with a warning and continue installation but whether that's possible is a whole other question.

@ffesti
Copy link
Contributor

ffesti commented Jun 19, 2024

OK, we actually set errno to ENOTDIR explicitly. Simplest "fix" would be to set it to EPERM for this case. This results in "Operation not permitted" instead of "Not a directory". This is obviously better and closer to the truth. But this still does not quite make it obvious what's the actual issue.

ffesti added a commit to ffesti/rpm that referenced this issue Jun 19, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with other
issues resulting in us setting errno to ENOTDIR. This led to confusing
as the symlink often indeed points at a directory. Using EPERM is still
not 100% right but points at least roughly into the right direction.

May be we should catch EPERM further up the call stack and use it to
give a more meaningfull error message.

Resolves: rpm-software-management#3100
@pmatilai
Copy link
Member

EPERM is just a different kind of misleading: I'm root, what do you mean "not permitted"?

@pmatilai
Copy link
Member

pmatilai commented Jun 20, 2024

So seeing this thing was like popping a cork in my head and then all the memories of the symlink CVE's bubbling up throughout yesterday evening to entertain me... Commenting here instead of the PR because there's more to this than what's in the PR now.

This error is not a system error so there's no errno that will adequately, much less satisfactorily match the situation. Been there, pored through it. It's an rpm specific error, and that you now added in the PR. I was about to say I don't remember why I didn't add that a specific error for this, but actually I did: 2668a2c. Only I forgot to actually use it 😆 . So, we don't need a new code for it.

Don't add any EPERM in there, ENOTDIR is technically the right thing for mimicing O_DIRECTORY behavior as it intends to. Instead, this invalid symlink thing can happen in any number of situations, not all of them related to directories. It needs to be caught inside the "follow links" condition and reported as the already existing RPMERR_INVALID_SYMLINK. And then make sure the O_DIRECTORY logic doesn't mess up the code. I think it doesn't but you'll want to double-check.

Finally, besides reporting the invalid symlink as such when encountered, the right thing to do and to actually resolve this ticket is to catch this in the pre-flight checks and prevent the transaction from even starting. It doesn't remove the need to handle it inside the fsm - the fsm needs to catch it real-time like it does now to prevent bad actors, but the pre-flight check should be there to catch admin mistakes such as in the original report. Failing an update mid-transaction can have pretty catastrophic consequences so we should catch what we can. I didn't chase that back then because fixing the CVE was the priority.

@pmatilai
Copy link
Member

Heh, I'd already forgotten this one: 89ce4e7 - but truly, been there, didn't help 😄

@dmnks
Copy link
Contributor Author

dmnks commented Aug 1, 2024

I guess this is very much intentional as the owner may send RPM to overwrite random files otherwise. This is the very case we did re-write the fsm for.

This still needs to give a sane error message, giving the user a fighting chance to find out what's the issue.

Oh, indeed. Actually that's also what the original RHEL ticket is about (i.e. to improve the error reporting here), I just filed this GH ticket in a rush and apparently didn't quite read through the RHEL ticket properly. I'll fix up the "Expected results" here.

That's just the "MVP" here, a better (proper) fix would be the one @pmatilai suggested above, of course.

@pmatilai
Copy link
Member

pmatilai commented Aug 7, 2024

The MVP to improve user understanding would be using the specific error code we have for this.
That would be something we could sneak into 4.20 at this point, but detecting this before transaction is way out of scope there, pushing this back to todo.

ffesti added a commit to ffesti/rpm that referenced this issue Aug 13, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with other
issues resulting in us setting errno to ENOTDIR. This led to confusing
as the symlink often indeed points at a directory.

Change fsmOpenat to report back an error code and use rpmfilesErrorCodes
from rpmarchive.h - which are already used in the surrounding code.

Give meaningful error message when encountering unsave symlinks.

Retire the abuse of errno for our own reporting.

Resolves: rpm-software-management#3100
ffesti added a commit to ffesti/rpm that referenced this issue Aug 14, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with other
issues resulting in us setting errno to ENOTDIR. This led to confusing
as the symlink often indeed points at a directory.

Change fsmOpenat to report back an error code and use rpmfilesErrorCodes
from rpmarchive.h - which are already used in the surrounding code.

Give meaningful error message when encountering unsave symlinks.

Retire the abuse of errno for our own reporting.

Resolves: rpm-software-management#3100
ffesti added a commit to ffesti/rpm that referenced this issue Aug 14, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with other
issues resulting in us setting errno to ENOTDIR. This led to confusing
as the symlink often indeed points at a directory.

Change fsmOpenat to report back an error code and use rpmfilesErrorCodes
from rpmarchive.h - which are already used in the surrounding code.

Give meaningful error message when encountering unsave symlinks.

Retire the abuse of errno for our own reporting.

Resolves: rpm-software-management#3100
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 20, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with
O_DIRECTORY behavior, leading to confusing error message as the symlink
often indeed points at a directory.

Give a more meaningful error message when encountering unsafe symlinks.

Co-authored-by: Florian Festi <[email protected]>

Resolves: rpm-software-management#3100
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 20, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with
O_DIRECTORY behavior, leading to confusing error message as the symlink
often indeed points at a directory. Emit a more meaningful error message
when encountering unsafe symlinks.

We already detect the error condition in the main if block here, might
as well set the error code right there and then so we don't need to
redetect later. We previously only tested for the unsafe link condition
when our O_DIRECTORY equivalent was set, but that seems wrong. Probably
doesn't matter with the existing callers, but we really must not
follow those unsafe symlinks no matter what.

Co-authored-by: Florian Festi <[email protected]>

Resolves: rpm-software-management#3100
pmatilai added a commit to pmatilai/rpm that referenced this issue Aug 26, 2024
RPM refuses to follow non root owned symlinks pointing to files owned by
another user for security reasons. This case was lumped in with
O_DIRECTORY behavior, leading to confusing error message as the symlink
often indeed points at a directory. Emit a more meaningful error message
when encountering unsafe symlinks.

We already detect the error condition in the main if block here, might
as well set the error code right there and then so we don't need to
redetect later. We previously only tested for the unsafe link condition
when our O_DIRECTORY equivalent was set, but that seems wrong. Probably
doesn't matter with the existing callers, but we really must not
follow those unsafe symlinks no matter what.

Co-authored-by: Florian Festi <[email protected]>

Resolves: rpm-software-management#3100
@ffesti ffesti closed this as completed in 535eacc Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants