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

Fixes #5536 #7285

Closed
wants to merge 2 commits into from
Closed

Fixes #5536 #7285

wants to merge 2 commits into from

Conversation

alexey-tikhonov
Copy link
Member

@alexey-tikhonov alexey-tikhonov commented Apr 10, 2024 •
edited
Loading

See commits comments for details.

Note about keep 'set-user-ID' in k5c_become_user(): this change potentially creates a way to regain euid back from suid. If SSSD runs under 'sssd' user it's not an issue at all. If SSSD runs under 'root' then it's worse - man capabilities:

If the effective user ID is changed from nonzero to 0, then the permitted set is copied to the effective set.

but:

  • 'permitted' set should be clean at this point (after sss_drop_all_caps()), but I didn't test it yet
  • frankly, if 'krb5_child' is compromised there are plenty of other (more simple) ways to exploit it than to regain capabilities from bounding set (that are present at startup anyway)
  • users looking for security hardening should run SSSD under 'sssd' user

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Apr 10, 2024
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review April 11, 2024 07:38
justin-stephenson
justin-stephenson approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

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

Ack, thank you.

@alexey-tikhonov
LDAP_CHILD: replace become_user() with sss_drop_all_caps()
74be564 
Since e2c26e8 'ldap_child' always runs
under SSSD_USER and uses file capabilities instead. For this reason
it doesn't make sense to call `become_user()` - `sss_drop_all_caps()`
is enough.
@alexey-tikhonov
KRB5_CHILD: keep 'set-user-ID' in k5c_become_user()
7bfe4f5 
Keep saved set-user-ID in `k5c_become_user()` so that 'sssd_be'
running under SSSD_USER could signal it.

Resolves: SSSD#5536
@alexey-tikhonov
Copy link
Member Author

(just a rebase)

sumit-bose
sumit-bose approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the patch, it was working well in my tests, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7285

  • master
    • 2891e74 - KRB5_CHILD: keep 'set-user-ID' in k5c_become_user()
    • b3a487a - LDAP_CHILD: replace become_user() with sss_drop_all_caps()

@alexey-tikhonov alexey-tikhonov mentioned this pull request Apr 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbrezina pbrezina pbrezina approved these changes

@sumit-bose sumit-bose sumit-bose approved these changes

@justin-stephenson justin-stephenson justin-stephenson approved these changes

Assignees

@justin-stephenson justin-stephenson

@sumit-bose sumit-bose

@pbrezina pbrezina

Labels
no-backport This should go to target branch only. non-privileged Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexey-tikhonov @justin-stephenson @sumit-bose @pbrezina