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

usertools: force local user for sssd process user #5867

Closed
wants to merge 2 commits into from

Conversation

ikerexxe
Copy link
Contributor

System hardening by forcing the sssd user to be loaded from a local database (/etc/passwd) instead of using any remote user. This could happen in very special conditions and might change the owner of the sssd databases and generate a denial of service.

Moreover, clarify user option in sssd.conf, as it accepts both the user name and the id as input. The only constraint is that the user should be present in the local database (/etc/passwd).

src/util/usertools_extra.c Outdated Show resolved Hide resolved
src/util/usertools_extra.c Outdated Show resolved Hide resolved
src/tests/cwrap/common_mock_nss_dl_load.c Outdated Show resolved Hide resolved
src/tests/cwrap/common_mock_nss_dl_load.c Outdated Show resolved Hide resolved
src/tests/cwrap/common_mock_nss_dl_load.c Outdated Show resolved Hide resolved
src/tests/cwrap/test_responder_common.c Show resolved Hide resolved
src/tests/cwrap/common_mock_nss_dl_load.c Outdated Show resolved Hide resolved
@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 10, 2021

There are memory leaks:
https://s3.eu-central-1.amazonaws.com/sssd-ci/PR-5867/3/debian10/ci-make-check-valgrind.log

FAIL: usertools-tests
FAIL: responder_common-tests

Did you try to run make check-valgrind-memcheck locally?

@ikerexxe
Copy link
Contributor Author

Failure in debian is not related with my changes. From logs:

The following additional packages will be installed:
  fuse
The following NEW packages will be installed:
  fuse sshfs
0 upgraded, 2 newly installed, 0 to remove and 14 not upgraded.
Need to get 118 kB of archives.
After this operation, 268 kB of additional disk space will be used.
Err:1 http://deb.debian.org/debian buster/main amd64 fuse amd64 2.9.9-1+deb10u1
  Temporary failure resolving 'debian.map.fastlydns.net' Temporary failure resolving 'deb.debian.org'
Err:2 http://deb.debian.org/debian buster/main amd64 sshfs amd64 2.10+repack-2
  Temporary failure resolving 'debian.map.fastlydns.net' Temporary failure resolving 'deb.debian.org'


Stderr from the command:

E: Failed to fetch http://deb.debian.org/debian/pool/main/f/fuse/fuse_2.9.9-1+deb10u1_amd64.deb  Temporary failure resolving 'debian.map.fastlydns.net' Temporary failure resolving 'deb.debian.org'
E: Failed to fetch http://deb.debian.org/debian/pool/main/s/sshfs-fuse/sshfs_2.10+repack-2_amd64.deb  Temporary failure resolving 'debian.map.fastlydns.net' Temporary failure resolving 'deb.debian.org'
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

Moreover, I executed the CI on demand for debian and it works. So, @alexey-tikhonov I think you can do another round of reviews.

@alexey-tikhonov
Copy link
Member

Hi @ikerexxe,

Probably, #5867 (comment) should be answered first.

Imo, humber of changes can be reduced significantly (and thus some other comments will not require addressing).

System hardening by forcing the sssd user to be loaded from a local
database (/etc/passwd) instead of using any remote user. This could
happen in very special conditions and might change the owner of the sssd
databases and generate a denial of service.

Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
user and allowed_uids options should be accessible via the files service
of nsswitch.conf.

Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
@alexey-tikhonov
Copy link
Member

Thanks for your patience, ACK.

@ikerexxe
Copy link
Contributor Author

Thanks for investing so much time reviewing it and giving feedback.

@pbrezina pbrezina added the Ready to push Ready to push label Dec 13, 2021
@pbrezina
Copy link
Member

Pushed PR: #5867

  • master
    • 3d25724 - man: sssd.conf and sssd-ifp clarify user option
    • 9c447dc - usertools: force local user for sssd process user

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Dec 13, 2021
@pbrezina pbrezina closed this Dec 13, 2021
@ikerexxe ikerexxe deleted the sssd_hardening branch December 14, 2021 08:19
alexey-tikhonov pushed a commit that referenced this pull request Jan 6, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see #5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
pbrezina pushed a commit that referenced this pull request Jan 17, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see #5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
jakub-vavra-cz pushed a commit to jakub-vavra-cz/sssd that referenced this pull request Jan 25, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see SSSD#5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
jakub-vavra-cz pushed a commit to jakub-vavra-cz/sssd that referenced this pull request Jan 25, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see SSSD#5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
shridhargadekar pushed a commit to shridhargadekar/sssd that referenced this pull request Apr 1, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see SSSD#5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
shridhargadekar pushed a commit to shridhargadekar/sssd that referenced this pull request Apr 1, 2022
only local users can be configured in `allowed_uids`
This check is now enforced - see SSSD#5867

Reviewed-by: Steeve Goveas <sgoveas@redhat.com>
@alexey-tikhonov
Copy link
Member

Hi @ikerexxe,

This could happen in very special conditions

what was that special conditions?

If we are talking about 'sssd user', wouldn't _SSS_LOOPS actually protect from using external user resolvable by SSSD?
Or did you mean some "3rd party" NSS modules? And what actually the issue in this case?

@sumit-bose
Copy link
Contributor

Hi,

yes, it is about 3rd party modules as well. The issue was that with some special configuration it was possible that SSSD was not able to access its own cache files anymore, but I can't remember the details, maybe Iker still has the description of the special setup. There might be other solutions to fix it.

But I think Jonathan's suggestion from the bugzilla ticket to use libnss_altfiles as a fallback is worth to consider as well.

bye,
Sumit

@ikerexxe
Copy link
Contributor Author

ikerexxe commented Apr 6, 2022

But I think Jonathan's suggestion from the bugzilla ticket to use libnss_altfiles as a fallback is worth to consider as well.

I also agree. If SSSD is unable to find the user in the "standard" database it should fallback to the ones provided by libnss_altfiles.

@alexey-tikhonov
Copy link
Member

But I think Jonathan's suggestion from the bugzilla ticket to use libnss_altfiles as a fallback is worth to consider as well.

Sumit, what do you mean "to use libnss_altfiles as a fallback"?

I read his comment as "let glibc do it's job, just refrains from resolving this user in SSSD" (and this is exactly what _SSS_LOOPS was already doing).

I also agree. If SSSD is unable to find the user in the "standard" database it should fallback to the ones provided by libnss_altfiles.

And when RHCOS switches to systemd-sysusers?
This feels like reimplementing glibc NSS subsystem within SSSD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants