sssd nss: issues with applications not using endpwent()

[Heiko Schlittermann]

Hello,

I'm using Dovecot with its "passwd" userdb, which effectivly uses NSS.
NSS services are provided by the files and by the sss "plugins".

The `doveadm user *` command enumerates the list of users. Repeating the
command doesn't enumerate the users provided by sssd again.

Analyzing this issue reveals:

Dovecot uses a long living process talking to NSS. For user
enumeration it uses

setpwent()
        while (…) { getpwent(); }

and then misses the call to endpwent(). This bug is already confirmed by
the Dovecot developers.

I'm not sure about the semantics of setpwent()/endpwend(), especially
about calling sequences like

setpwent()
        while (…) { getpwent(); }

setpwent()
        while (…) { getpwent(); }

According to setpwent(3) it should rewind to the beginning. Calling
endpwent() seems to be for curtesy only (to have resources freed)

I suggest calling a preventive endpwent() before using setpwent() again
in nss_cmd.c.

Attached you'll find my patch. I'd be happy about integration into
upstream.

Best regards from Dresden/Germany
    Viele Grüße aus Dresden
    Heiko Schlittermann
--
 SCHLITTERMANN.de ---------------------------- internet & unix support -
 Heiko Schlittermann, Dipl.-Ing. (TU) - {fon,fax}: +49.351.802998{1,3} -
 gnupg encrypted messages are welcome --------------- key ID: F69376CE -

[Sumit Bose]

On Thu, Feb 25, 2021 at 05:17:22PM +0100, Heiko Schlittermann wrote:
> Hello,
>
> I'm using Dovecot with its "passwd" userdb, which effectivly uses NSS.
> NSS services are provided by the files and by the sss "plugins".
>
> The `doveadm user *` command enumerates the list of users. Repeating the
> command doesn't enumerate the users provided by sssd again.
>
> Analyzing this issue reveals:
>
> Dovecot uses a long living process talking to NSS. For user
> enumeration it uses
>     setpwent()
>     while (…) { getpwent(); }
>
> and then misses the call to endpwent(). This bug is already confirmed by
> the Dovecot developers.
>
> I'm not sure about the semantics of setpwent()/endpwend(), especially
> about calling sequences like
>     setpwent()
>     while (…) { getpwent(); }
>
>     setpwent()
>     while (…) { getpwent(); }
>
> According to setpwent(3) it should rewind to the beginning. Calling
> endpwent() seems to be for curtesy only (to have resources freed)
>
> I suggest calling a preventive endpwent() before using setpwent() again
> in nss_cmd.c.
>
> Attached you'll find my patch. I'd be happy about integration into
> upstream.

Hi,

I guess  you are right that setpwent() should make sure to rewind to the
beginning.

Would you mind to open a ticket at
https://github.com/SSSD/sssd/issues/new about this?

About your patch, I think it would be better to fix this in the NSS
responder of SSSD like e.g. (not tested)

diff --git a/src/responder/nss/nss_cmd.c b/src/responder/nss/nss_cmd.c
index 844776c5f..5ad192031 100644
--- a/src/responder/nss/nss_cmd.c
+++ b/src/responder/nss/nss_cmd.c
@@ -999,6 +999,11 @@ static errno_t nss_cmd_getpwuid_ex(struct cli_ctx *cli_ctx)
 static errno_t nss_cmd_setpwent(struct cli_ctx *cli_ctx)
 {
     struct nss_ctx *nss_ctx;
+    struct nss_state_ctx *state_ctx;
+
+    state_ctx = talloc_get_type(cli_ctx->state_ctx, struct nss_state_ctx);
+    state_ctx->pwent->domain = 0;
+    state_ctx->pwent->result = 0;

nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx);

Can you check if this works as well? Feel free to use this for creating a pull
request at https://github.com/SSSD/sssd/pulls. I wonder if you can add a simlar
logic to the other setXYent() calls as well?

bye,
Sumit
> Best regards from Dresden/Germany
> Viele Grüße aus Dresden
> Heiko Schlittermann
>
> --
>  SCHLITTERMANN.de ---------------------------- internet & unix support -
>  Heiko Schlittermann, Dipl.-Ing. (TU) - {fon,fax}: +49.351.802998{1,3} -
>  gnupg encrypted messages are welcome --------------- key ID: F69376CE -
> From b798e72e5edda97ebb31e60634c38c7e9db70692 Mon Sep 17 00:00:00 2001
> From: "Heiko Schlittermann (HS12-RIPE)" 
> Date: Thu, 25 Feb 2021 17:15:11 +0100
> Subject: [PATCH] Add preventive endpwent() prior calling setpwent()
>
> src/responder/nss/nss_cmd.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/src/responder/nss/nss_cmd.c b/src/responder/nss/nss_cmd.c
> index 844776c5f..27307a66c 100644
> --- a/src/responder/nss/nss_cmd.c
> +++ b/src/responder/nss/nss_cmd.c
> @@ -996,10 +996,19 @@ static errno_t nss_cmd_getpwuid_ex(struct cli_ctx *cli_ctx)
>                          SSS_MC_PASSWD, nss_protocol_fill_pwent);
>  }
>
> +static errno_t nss_cmd_endpwent(struct cli_ctx *cli_ctx)
> +{
> struct nss_state_ctx *state_ctx;
> state_ctx = talloc_get_type(cli_ctx->state_ctx, struct nss_state_ctx);
> return nss_endent(cli_ctx, &state_ctx->pwent);
>
> +}
>
> static errno_t nss_cmd_setpwent(struct cli_ctx *cli_ctx)
>  {
>      struct nss_ctx *nss_ctx;
>
> nss_cmd_endpwent(cli_ctx);
> nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx);
>
> return nss_setent(cli_ctx, CACHE_REQ_ENUM_USERS, nss_ctx->pwent);
>
> @@ -1018,15 +1027,6 @@ static errno_t nss_cmd_getpwent(struct cli_ctx *cli_ctx)
>                        nss_ctx->pwent);
>  }
>
> -static errno_t nss_cmd_endpwent(struct cli_ctx *cli_ctx)
> -{
> struct nss_state_ctx *state_ctx;
> state_ctx = talloc_get_type(cli_ctx->state_ctx, struct nss_state_ctx);
> return nss_endent(cli_ctx, &state_ctx->pwent);
>
> -}
>
> static errno_t nss_cmd_getgrnam(struct cli_ctx *cli_ctx)
>  {
>      return nss_getby_name(cli_ctx, false, CACHE_REQ_GROUP_BY_NAME, NULL,
> -- 
> 2.20.1
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedorahosted.org/archives/list/[e-mail].o...
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Heiko Schlittermann]

Hi Sumit,

thank you for responding.

Sumit Bose  (Mo 01 Mär 2021 11:33:51 CET):
> I guess  you are right that setpwent() should make sure to rewind to the
> beginning.
>
> Would you mind to open a ticket at
> https://github.com/SSSD/sssd/issues/new about this?

Sure.
I'm about to perform some more tests, and then I'll open an issue there.
> About your patch, I think it would be better to fix this in the NSS
> responder of SSSD like e.g. (not tested)
> diff --git a/src/responder/nss/nss_cmd.c b/src/responder/nss/nss_cmd.c

…
>  struct nss_ctx *nss_ctx;
>
> struct nss_state_ctx *state_ctx;
>
> state_ctx = talloc_get_type(cli_ctx->state_ctx, struct nss_state_ctx);
>
> state_ctx->pwent->domain = 0;
>
> state_ctx->pwent->result = 0;
>
> nss_ctx = talloc_get_type(cli_ctx->rctx->pvt_ctx, struct nss_ctx);

Yes, after digging a bit more over the weekend, I had the same idea,
I'm testing it right now. Same for the other setXYent() calls, that came
into my mind as well.

Best regards from Dresden/Germany
    Viele Grüße aus Dresden
    Heiko Schlittermann
--
 SCHLITTERMANN.de ---------------------------- internet & unix support -
 Heiko Schlittermann, Dipl.-Ing. (TU) - {fon,fax}: +49.351.802998{1,3} -
 gnupg encrypted messages are welcome --------------- key ID: F69376CE -

[Heiko Schlittermann]

Heiko Schlittermann  (Mo 01 Mär 2021 12:41:58 CET):
> Hi Sumit,
>
> thank you for responding.
>
> Sumit Bose  (Mo 01 Mär 2021 11:33:51 CET):
> > I guess  you are right that setpwent() should make sure to rewind to the
> > beginning.
> >
> > Would you mind to open a ticket at
> > https://github.com/SSSD/sssd/issues/new about this?
>
> Sure.

https://github.com/SSSD/sssd/issues/5523
https://github.com/SSSD/sssd/pull/5524
> > state_ctx->pwent->domain = 0;
> > state_ctx->pwent->result = 0;

state_ctx->pwent.domain = 0;
         state_ctx->pwent.result = 0;
> Yes, after digging a bit more over the weekend, I had the same idea,
> I'm testing it right now. Same for the other setXYent() calls, that came
> into my mind as well.

Fixed that for pwent, grent, servent (svcent).