[PATCH] PAM: only allow missing user name for certificate

[Sumit Bose]

Hi,

this patch aims to fix https://fedorahosted.org/sssd/ticket/2811. It
occurs if the client of sssd_pam does not provide a user name during
pre-auth and certificate/Smartcard authentication is not enabled.

I relaxed the user name check when adding the Smartcard support and
added tests as well but unfortunately only for the case when Smartcard
authentication is enabled. This patch adds a check for the other case as
well.

Besides the check itself I tried to send a more suitable error code then
PAM_SYSTEM_ERR. For this I had to make a small change in pam_forwarder()
to not unconditionally overwrite the return code with EINVAL. I think
the change is safe because pam_forwarder_parse_data() does not return
ENOENT which would be the only case when a misleading error would be
send back to the caller.

bye,
Sumit

[Lukas Slebodnik]

On (01/10/15 10:57), Sumit Bose wrote:
> Hi,
>
> this patch aims to fix https://fedorahosted.org/sssd/ticket/2811. It
> occurs if the client of sssd_pam does not provide a user name during
> pre-auth and certificate/Smartcard authentication is not enabled.
>
> I relaxed the user name check when adding the Smartcard support and
> added tests as well but unfortunately only for the case when Smartcard
> authentication is enabled. This patch adds a check for the other case as
> well.
>
> Besides the check itself I tried to send a more suitable error code then
> PAM_SYSTEM_ERR. For this I had to make a small change in pam_forwarder()
> to not unconditionally overwrite the return code with EINVAL. I think
> the change is safe because pam_forwarder_parse_data() does not return
> ENOENT which would be the only case when a misleading error would be
> send back to the caller.
>
> bye,
> Sumit
> From 170c8c970c9ee2bfd10a6f3d988d7cd5fd33b322 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 1 Oct 2015 10:10:22 +0200
> Subject: [PATCH] PAM: only allow missing user name for certificate
> authentication
> Resolves https://fedorahosted.org/sssd/ticket/2811
>
> src/responder/pam/pamsrv_cmd.c  | 12 +++++++++---
> src/tests/cmocka/test_pam_srv.c | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 3 deletions(-)

CI passed http://sssd-ci.duckdns.org/logs/job/28/26/summary.html
and crash is fixed
> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> index 27dddcf43c1ff6eb465e1cb58d6dddf21413dcc4..978c637e22b03d3be1e07e8dc713aa01c4bb22e5 100644
> --- a/src/responder/pam/pamsrv_cmd.c
> +++ b/src/responder/pam/pamsrv_cmd.c
> @@ -957,11 +957,13 @@ static errno_t pam_forwarder_parse_data(struct cli_ctx *cctx, struct pam_data *p
>     } else {
>         /* Only SSS_PAM_PREAUTH request may have a missing name, e.g. if the
>          * name is determined with the help of a certificate */
>    if (pd->cmd == SSS_PAM_PREAUTH) {
>    if (pd->cmd == SSS_PAM_PREAUTH
>            && may_do_cert_auth(talloc_get_type(cctx->rctx->pvt_ctx,
>                                                struct pam_ctx),pd)) {
>       ret = EOK;
>   } else {
>       DEBUG(SSSDBG_CRIT_FAILURE, "Missing logon name in PAM request.\n");
>        ret = EINVAL;
>        ret = ENODATA;

^^^^^^^
It's a little bit non standard errno. (it's not in BSD)
https://www.freebsd.org/cgi/man.cgi?query=errno&sektion=2
It might be better to either use different errno or
to use our custom error code.
>         goto done;
>     }
> }
>
> @@ -1104,7 +1106,6 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
>         }
>         goto done;
>     } else if (ret != EOK) {
>    ret = EINVAL;
>   goto done;
> }
>
> @@ -1610,6 +1611,11 @@ static int pam_check_user_done(struct pam_auth_req *preq, int ret)
>         pam_reply(preq);
>         break;
> case ENODATA:
>    preq->pd->pam_status = PAM_CRED_INSUFFICIENT;
>    pam_reply(preq);
>    break;
> default:
>       preq->pd->pam_status = PAM_SYSTEM_ERR;
>       pam_reply(preq);
>
> diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
> index ab33433fdce8d6030331a57e2b7cbd97ce5637df..dd20a19be5ca7fd57376b3d639a36b1b4f5158f4 100644
> --- a/src/tests/cmocka/test_pam_srv.c
> +++ b/src/tests/cmocka/test_pam_srv.c
> @@ -623,6 +623,23 @@ static int test_pam_wrong_pw_offline_auth_check(uint32_t status,
>     return test_pam_simple_check(status, body, blen);
> }
>
> +static int test_pam_creds_insufficient_check(uint32_t status,
>                                            uint8_t *body, size_t blen)

^^^
                                            few extra spaces

Thank you very much for unit test.

LS

[Pavel Reichl]

> > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> > index 27dddcf43c1ff6eb465e1cb58d6dddf21413dcc4..978c637e22b03d3be1e07e8dc713aa01c4bb22e5 100644
> > --- a/src/responder/pam/pamsrv_cmd.c
> > +++ b/src/responder/pam/pamsrv_cmd.c
> > @@ -957,11 +957,13 @@ static errno_t pam_forwarder_parse_data(struct cli_ctx *cctx, struct pam_data *p
> >      } else {
> >          /* Only SSS_PAM_PREAUTH request may have a missing name, e.g. if the
> >           * name is determined with the help of a certificate */
> >    if (pd->cmd == SSS_PAM_PREAUTH) {
> >    if (pd->cmd == SSS_PAM_PREAUTH
> >            && may_do_cert_auth(talloc_get_type(cctx->rctx->pvt_ctx,
> >                                                struct pam_ctx),pd)) {

Since you might be touching the code again could you please fix this super minor nitpick (missing space after comma)? Thanks!

[Sumit Bose]

On Thu, Oct 01, 2015 at 03:12:06PM +0200, Pavel Reichl wrote:
> > > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> > > index 27dddcf43c1ff6eb465e1cb58d6dddf21413dcc4..978c637e22b03d3be1e07e8dc713aa01c4bb22e5 100644
> > > --- a/src/responder/pam/pamsrv_cmd.c
> > > +++ b/src/responder/pam/pamsrv_cmd.c
> > > @@ -957,11 +957,13 @@ static errno_t pam_forwarder_parse_data(struct cli_ctx *cctx, struct pam_data *p
> > >     } else {
> > >         /* Only SSS_PAM_PREAUTH request may have a missing name, e.g. if the
> > >          * name is determined with the help of a certificate */
> > >    if (pd->cmd == SSS_PAM_PREAUTH) {
> > >    if (pd->cmd == SSS_PAM_PREAUTH
> > >            && may_do_cert_auth(talloc_get_type(cctx->rctx->pvt_ctx,
> > >                                                struct pam_ctx),pd)) {
>
> Since you might be touching the code again could you please fix this super minor nitpick (missing space after comma)? Thanks!

Lukas, Pavel, thank you for the comments, I included all of them, new
version attached.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (02/10/15 10:21), Sumit Bose wrote:
> On Thu, Oct 01, 2015 at 03:12:06PM +0200, Pavel Reichl wrote:
> > > > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> > > > index 27dddcf43c1ff6eb465e1cb58d6dddf21413dcc4..978c637e22b03d3be1e07e8dc713aa01c4bb22e5 100644
> > > > --- a/src/responder/pam/pamsrv_cmd.c
> > > > +++ b/src/responder/pam/pamsrv_cmd.c
> > > > @@ -957,11 +957,13 @@ static errno_t pam_forwarder_parse_data(struct cli_ctx *cctx, struct pam_data *p
> > > >     } else {
> > > >         /* Only SSS_PAM_PREAUTH request may have a missing name, e.g. if the
> > > >          * name is determined with the help of a certificate */
> > > >    if (pd->cmd == SSS_PAM_PREAUTH) {
> > > >    if (pd->cmd == SSS_PAM_PREAUTH
> > > >            && may_do_cert_auth(talloc_get_type(cctx->rctx->pvt_ctx,
> > > >                                                struct pam_ctx),pd)) {
> >
> > Since you might be touching the code again could you please fix this super minor nitpick (missing space after comma)? Thanks!
>
> Lukas, Pavel, thank you for the comments, I included all of them, new
> version attached.
>
> bye,
> Sumit
> From 4ac4a4d504575491c05390c37be2bf78c437e185 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 1 Oct 2015 10:10:22 +0200
> Subject: [PATCH] PAM: only allow missing user name for certificate
> authentication
> Resolves https://fedorahosted.org/sssd/ticket/2811
>
> src/responder/pam/pamsrv_cmd.c  | 12 +++++++++---
> src/tests/cmocka/test_pam_srv.c | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+), 3 deletions(-)

ACK

LS

[Lukas Slebodnik]

On (02/10/15 12:11), Lukas Slebodnik wrote:
> On (02/10/15 10:21), Sumit Bose wrote:
> > On Thu, Oct 01, 2015 at 03:12:06PM +0200, Pavel Reichl wrote:
> > > > > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> > > > > index 27dddcf43c1ff6eb465e1cb58d6dddf21413dcc4..978c637e22b03d3be1e07e8dc713aa01c4bb22e5 100644
> > > > > --- a/src/responder/pam/pamsrv_cmd.c
> > > > > +++ b/src/responder/pam/pamsrv_cmd.c
> > > > > @@ -957,11 +957,13 @@ static errno_t pam_forwarder_parse_data(struct cli_ctx *cctx, struct pam_data *p
> > > > >     } else {
> > > > >         /* Only SSS_PAM_PREAUTH request may have a missing name, e.g. if the
> > > > >          * name is determined with the help of a certificate */
> > > > >    if (pd->cmd == SSS_PAM_PREAUTH) {
> > > > >    if (pd->cmd == SSS_PAM_PREAUTH
> > > > >            && may_do_cert_auth(talloc_get_type(cctx->rctx->pvt_ctx,
> > > > >                                                struct pam_ctx),pd)) {
> > >
> > > Since you might be touching the code again could you please fix this super minor nitpick (missing space after comma)? Thanks!
> >
> > Lukas, Pavel, thank you for the comments, I included all of them, new
> > version attached.
> >
> > bye,
> > Sumit
> > From 4ac4a4d504575491c05390c37be2bf78c437e185 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Thu, 1 Oct 2015 10:10:22 +0200
> > Subject: [PATCH] PAM: only allow missing user name for certificate
> > authentication
> > Resolves https://fedorahosted.org/sssd/ticket/2811
> >
> > src/responder/pam/pamsrv_cmd.c  | 12 +++++++++---
> > src/tests/cmocka/test_pam_srv.c | 38 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 47 insertions(+), 3 deletions(-)
>
> ACK
>
> LS

master:
* 2e76b32e74abedb23665808bacc73cafd1097c37

sssd-1-13:
* ba9d5c0456a2fbb9adf9b4b4dffbfb190628a273

LS
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel