Bug 1402403

I'm moving it to SSSD for investigation.

The choice of nsswitch path to a proper backend should be irrelevant for PAM operations as home directory creation happens after a user has been looked up already and the directory is not available.

Hi,

I can reproduce this on RHEL7 (not on RHEL8 or recent Fedora). The reason is that pam_oddjob_mkhomedir call getpwnam_r() starting with a very small buffer, 4 bytes. Here it is expected that getpwnam_r() returns ERANGE(34) and the pam_oddjob_mkhomedir will increase the buffer.

On RHEL7 in compat mode 0 is returned with empty passwd struct. A short reproducer is:

"""
#include <stdio.h>
#include <sys/types.h>
#include <pwd.h>

int main(int argc, char *argv[])
{
        char buf[4];
        int ret;
        struct passwd pwd;
        struct passwd *pw;

        if (argc != 2) {
                return -1;
        }

        ret = getpwnam_r(argv[1], &pwd, buf, sizeof(buf), &pw);

        printf("ret: %d\n", ret);

        return 0;
}
"""

You can call it with any input an the expected output is:

$ /tmp/small dfw
ret: 34

On RHEL7 in compat mode with sss it is 

$ /tmp/small dfw
ret: 0


It might be similar to https://pagure.io/SSSD/sssd/pull-request/3589. But since this is already included in RHEL7 build we might have missed an place where we return a wrong error code in compat mode.

Hi,

I took a closer look and wonder if a fix in the SSSD side would really help.

I can see the following:

 - __getpwnam_r() calls _nss_compat_getpwnam_r() which call internal_getpwnam_r() as expected
 - internal_getpwnam_r() fails with result==NSS_STATUS_TRYAGAIN and errno==ERANGE as expected
 - internal_endpwent() is called unconditionally which seem to call the related endpwent function of the nss module given by passwd_compat
 - in the case of libnss_sss _nss_sss_endpwent() is called which will set errno to 0 at some point
 - after internal_endpwent() returns errno==0
 - since __getpwnam_r() calls _nss_compat_getpwnam_r() with &errno as fifth argument and just checks the global errno the expected errno==ERANGE cannot be detected anymore since errno was set to 0 during endpwent processing

Since this may happen with any other nss module used with password_compat I wonder if it is expected (documented?) that nss modules have to make sure that errno is not change during endpwent (and maybe other calls) or if this should be better fixed in the glibc side?

Florian, I hope you are the right person to ask, if not please let me know who would be a better target.

Thanks.

bye,
Sumit

(In reply to Sumit Bose from comment #5)
> Hi,
> 
> I took a closer look and wonder if a fix in the SSSD side would really help.
> 
> I can see the following:
> 
>  - __getpwnam_r() calls _nss_compat_getpwnam_r() which call
> internal_getpwnam_r() as expected
>  - internal_getpwnam_r() fails with result==NSS_STATUS_TRYAGAIN and
> errno==ERANGE as expected
>  - internal_endpwent() is called unconditionally which seem to call the
> related endpwent function of the nss module given by passwd_compat
>  - in the case of libnss_sss _nss_sss_endpwent() is called which will set
> errno to 0 at some point
>  - after internal_endpwent() returns errno==0
>  - since __getpwnam_r() calls _nss_compat_getpwnam_r() with &errno as fifth
> argument and just checks the global errno the expected errno==ERANGE cannot
> be detected anymore since errno was set to 0 during endpwent processing
> 
> Since this may happen with any other nss module used with password_compat I
> wonder if it is expected (documented?) that nss modules have to make sure
> that errno is not change during endpwent (and maybe other calls) or if this
> should be better fixed in the glibc side?

_nss_sss_endpwent setting errno to 0 seems rather fishy. As a general rule, library functions should not do this. If they need to set errno to zero to detect errors in the obscure cases that need that, they are expected to save and restore errno around that, so that setting errno to 0 is not visible to the callers.

I think as a belt-and-suspends fix in glibc, we could preserve errno during in internal_endpwent in nss_compat. I can post an upstream fix for that and see if we can get it accepted. We can decide on potential backports after that.

Hi Florian,

thanks for the comment. Since this ticket was already somewhat planned for RHEL-7.9 I can add a fix in the SSSD side at least for _nss_sss_endpwent() to get around this issue.

Nevertheless I think some protection on the glibc side might be useful as well since I think there are not only obscure cases where errno might be set. Even if not set explicitly to 0 it might be set automatically by an error e.g. when calling open() or close().

bye,
Sumit

(In reply to Sumit Bose from comment #7)
> thanks for the comment. Since this ticket was already somewhat planned for
> RHEL-7.9 I can add a fix in the SSSD side at least for _nss_sss_endpwent()
> to get around this issue.
> 
> Nevertheless I think some protection on the glibc side might be useful as
> well since I think there are not only obscure cases where errno might be
> set. Even if not set explicitly to 0 it might be set automatically by an
> error e.g. when calling open() or close().

I tend to agree, so I posted a patch: https://sourceware.org/pipermail/libc-alpha/2020-May/113864.html

Upstream ticket:
https://github.com/SSSD/sssd/issues/5153

Hi,

a review comment by Alexey made we wonder why we set errno in the first place and I came across https://pubs.opengroup.org/onlinepubs/9699919799/functions/endpwent.html which says:

"""
The setpwent() and endpwent() functions shall not change the setting of errno if successful.

On error, the setpwent() and endpwent() functions shall set errno to indicate the error.

Since no value is returned by the setpwent() and endpwent() functions, an application wishing to check for error situations should set errno to 0, then call the function, then check errno.
"""

I would conclude that this should be true for nss module functions like e.g. _nss_sss_endpwent() because who else can set errno in the case of an error. I will change the SSSD functions accordingly. But this also means that the patch for glibc is really required for the compat case.

Florian, will you patch be automatically added to RHEL-8 as well or shall I open a RHEL-8 ticket for glibc?

bye,
Sumit