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.

Sign up for GitHub

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

Jump to bottom

Revert "Lookup xauth in PATH." #6129

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Revert "Lookup xauth in PATH." #6129

merged 1 commit into from
Dec 21, 2023

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Dec 14, 2023 •
edited
Loading

This reverts commit 407c05e.

If --private-lib is used (and firejail is configured with
--enable-private-lib), the following error occurs:

$ firejail --quiet --noprofile --private-lib true
firejail: fs_lib.c:56: find_in_path: Assertion `geteuid() != 0' failed.
Error: proc 10000 cannot sync with peer: unexpected EOF
Peer 10001 unexpectedly killed (Segmentation fault)

Given that it causes an uid assertion failure, the logic appears to not
be correct and the current behavior may be unsafe, so for now revert
that commit until the issue is properly addressed.

Relates to #6006 #6087.

Fixes #6113.

Cc: @chestnykh @hashelq

@kmk3
Revert "Lookup xauth in PATH."
8f33e72 
This reverts commit 407c05e.

If --private-lib is used (and firejail is configured with
--enable-private-lib), the following error occurs:

    $ firejail --quiet --noprofile --private-lib true
    firejail: fs_lib.c:56: find_in_path: Assertion `geteuid() != 0' failed.
    Error: proc 10000 cannot sync with peer: unexpected EOF
    Peer 10001 unexpectedly killed (Segmentation fault)

Given that it causes an uid assertion failure, the logic appears to not
be correct and the current behavior may be unsafe, so for now revert
that commit until the issue is properly addressed.

Relates to netblue30#6006 netblue30#6087.

Fixes netblue30#6113.
@kmk3 kmk3 added this to In progress in Release 0.9.74 via automation Dec 14, 2023
@glitsj16
Copy link
Collaborator

@kmk3 Thanks for reverting this. I assume my (unreviewed) patch mentioned in #6113 (comment) flew under the radar. Not a problem, yet I would be interested to hear opinions on it for learning purposes.

@kmk3
Copy link
Collaborator Author

kmk3 commented Dec 14, 2023

@kmk3 Thanks for reverting this. I assume my (unreviewed) patch mentioned in
#6113
(comment)
flew under the radar. Not a problem, yet I would be interested to hear
opinions on it for learning purposes.

The idea to use EUID_USER seems good (especially considering that $PATH may
be set by the user), though I think it would make more sense to always use it
(along with if (called_as_root) etc) instead of special-casing private-lib.

Also, this function is used by other parts of the code (which I'm not 100% sure
would work correctly with EUID_USER) and the commit breaks existing
functionality, so I'd rather just revert it for now and check it later.

@glitsj16
Copy link
Collaborator

The idea to use EUID_USER seems good (especially considering that $PATH may
be set by the user), though I think it would make more sense to always use it
(along with if (called_as_root) etc) instead of special-casing private-lib.

That makes sense. Avoiding special-casing any option would be the more secure route to deal with this. Thanks for taking the time to elaborate!

@netblue30 netblue30 merged commit f0be1a4 into netblue30:master Dec 21, 2023
13 checks passed
@netblue30
Copy link
Owner

Merged!

@kmk3 kmk3 moved this from In progress to Done (RELNOTES N/A) in Release 0.9.74 Jan 3, 2024
kmk3 added a commit that referenced this pull request Jan 3, 2024
@kmk3
RELNOTES: remove xauth modif item
07084fa 
Reverted by commit 8f33e72 ("Revert "Lookup xauth in PATH."",
2023-12-13) / PR #6129.

Relates to #6006 #6087.
@kmk3 kmk3 deleted the revert-lookup-xauth branch January 4, 2024 00:32
@kmk3 kmk3 moved this from Done (RELNOTES N/A) to Reverted in Release 0.9.74 Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glitsj16 glitsj16 glitsj16 approved these changes

@rusty-snake rusty-snake Awaiting requested review from rusty-snake

Assignees
No one assigned
Labels
None yet
Projects
Release 0.9.74
  
Reverted (RELNOTES N/A)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

firejail: fs_lib.c:56: find_in_path: Assertion \`geteuid() != 0' failed
3 participants
@kmk3 @glitsj16 @netblue30