Changes around FILE:/DIR: ccache checks#8344
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the ccache checking logic for FILE: and DIR: type ccaches. The changes are a good improvement, especially the move from using CAP_DAC_READ_SEARCH to switching to the user's UID/GID for permission checks, which enhances security by reducing the process's privileges. The logic is also updated to perform these checks regardless of the command type, which correctly handles more complex authentication flows. I've found one minor issue with an outdated debug message that should be corrected for consistency and to avoid confusion during future debugging.
ad160b2 to
7177266
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ccache handling logic in krb5_child. The changes are well-aligned with the goals stated in the description.
The removal of sss_krb5_precheck_ccache simplifies the code by delegating permission checks to the OS during ccache creation, which is a sound approach. This also removes the need for CAP_DAC_READ_SEARCH for this purpose.
The logic for reusing an old ccache is now consolidated into the new k5c_ccache_check function. The call to this function is now correctly conditioned on the presence of setid capabilities and is no longer skipped during pre-authentication, which is a good improvement.
Finally, replacing sss_set_cap_effective(..., false) with sss_drop_cap(...) is a good security hardening measure, as it drops capabilities from all sets, not just the effective set.
Overall, the changes are positive and improve the correctness and security of the code.
7177266 to
fbb3273
Compare
|
Note: Covscan is green. |
fbb3273 to
1a663ba
Compare
1a663ba to
d6524f3
Compare
src/providers/krb5/krb5_child.c
Outdated
| * this currently use 'CAP_DAC_READ_SEARCH'. | ||
| * Cache files are not needed during preauth. */ | ||
| if (kr->pd->cmd != SSS_PAM_PREAUTH) { | ||
| if (kr->krb5_child_has_setid_caps && (kr->pd->cmd != SSS_PAM_PREAUTH)) { |
There was a problem hiding this comment.
Hi,
do I understand correctly that if SSSD is "classically" build and configured to as root krb5_child will automatically have all needed capabilities so that kr->krb5_child_has_setid_caps is true?
bye,
Sumit
There was a problem hiding this comment.
If "classically" means --with-sssd-user=root then difference vs --with-sssd-user=sssd in current 'master' branch is negligible.
In all cases service configuration file has:
sssd/src/sysv/systemd/sssd.service.in
Line 28 in 08c2ccf
so no process will gain any capabilities "automatically" but will rely on "file capabilities" of corresponding binaries, limited by a
Line 115 in 08c2ccf
But I must admit we do not test building --with-sssd-user=root, PR CI only has limited testing coverage for "--with-sssd-user=sssd but run under root".
There was a problem hiding this comment.
But if anything is broken for this ./configure option then most probably it is broken for some time already, not in this PR.
Practically our spec-file example caters for --with-sssd-user=sssd case.
There was a problem hiding this comment.
Hi,
ok, thanks.
bye,
Sumit
There was a problem hiding this comment.
But I must admit we do not test building
--with-sssd-user=root
Well, looks like Debian (for some reason) is stick to --with-sssd-user=root:
https://salsa.debian.org/sssd-team/sssd/-/blob/master/debian/rules#L48
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ccache handling in krb5_child to improve security. The main change is the removal of sss_krb5_precheck_ccache, which eliminates the need for CAP_DAC_READ_SEARCH capability when checking ccache paths. Instead, the logic now relies on setting the real UID/GID of the process to the user's and then switching the effective UID to perform ccache operations with user privileges. This is a solid security improvement. The changes also include consolidating ccache check logic into a new k5c_ccache_check function and adjusting the call sites to perform these checks regardless of the command type, which covers more use cases. The use of sss_drop_cap instead of sss_set_cap_effective is another good security hardening measure. The code is cleaner and more secure. I have no issues to report.
ikerexxe
left a comment
There was a problem hiding this comment.
Just a minor comment inline and another one in a commit message. In commit KRB5_CHILD: allow k5c_ccache_check() during SSS_PAM_PREAUTH you wrote PREUATH instead of PREAUTH
d6524f3 to
0c0632a
Compare
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the patches, I have no further comments, ACK.
bye,
Sumit
ikerexxe
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the fix!
'krb5_child' doesn't precreate path to FILE:/DIR: ccache since 5e17bc2 Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
to check FILE:/DIR: ccache path. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
This should cover a case when a single execution of 'krb5_child' handles both PREAUTH and AUTH Resolves: SSSD#8331 Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Inaccessible FILE:/DIR: path is a system configuration error that needs to be fixed anyway. It doesn't make much difference to fail before or after credentials check in this case. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
handles both PREUATH and AUTH
Resolves: #8331