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

Add target group as first gid when specified #936

Closed

Conversation

bjorn3
Copy link
Collaborator

@bjorn3 bjorn3 commented Dec 16, 2024

On FreeBSD the first gid in the setgroups list is the effective gid. And the groups list should contain this gid a second time to avoid losing it when running a setgid program according to the man page of setgroups.

This somehow causes the group in when_specified_more_than_once_all_groups_are_added_to_group_list to be duplicated. I can't check correctness of this duplication against original su as on FreeBSD su doesn't provide an option to specify groups.

This fixes the following test failures:

---- su::flag_group::when_specified_more_than_once_all_groups_are_added_to_group_list stdout ----
thread 'su::flag_group::when_specified_more_than_once_all_groups_are_added_to_group_list' panicked at sudo-compliance-tests/src/su/flag_group.rs:84:5:
assertion `left == right` failed
  left: "1001 1000"
 right: "1001"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- sudo::flag_group::adds_group_to_groups_output stdout ----
thread 'sudo::flag_group::adds_group_to_groups_output' panicked at sudo-compliance-tests/src/sudo/flag_group.rs:74:9:
assertion failed: `(left == right)`

Diff < left / right > :
 {
<    "users",
     "secondary-group",
>    "ferris",
>    "users",
 }

Part of #869

@squell
Copy link
Member

squell commented Dec 19, 2024

I'm fearful of the added complexity of making the request group into an Option, especially as this impacts the permission checker (by the time it runs the requested group has to be completely figured out). And it also doesn't solve the issue when someone is running -u user -g user (we're still non-compliant with these changes).

But I think the .insert instead of .push is onto something.

Copy link
Member

@squell squell left a comment

Choose a reason for hiding this comment

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

Option<Group> should not really be in the context object

@squell
Copy link
Member

squell commented Dec 19, 2024

So on BSD, setgroups expect the first argument to be the effective GID, which also means a subsequent call to setgid will overwrite it; that causes the tricky behaviour here.

On FreeBSD the first gid in the setgroups list is the effective gid.
And the groups list should contain this gid a second time to avoid
losing it when running a setgid program according to the man page of
setgroups.
@bjorn3 bjorn3 force-pushed the freebsd_group_change branch from ccd59b5 to db910f1 Compare December 19, 2024 14:42
@bjorn3
Copy link
Collaborator Author

bjorn3 commented Dec 19, 2024

Closed in favor of #937

@bjorn3 bjorn3 closed this Dec 19, 2024
@bjorn3 bjorn3 deleted the freebsd_group_change branch December 19, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants