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

Lukas Slebodnik lslebodn at redhat.com
Thu Oct 1 12:58:33 UTC 2015


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 <sbose at redhat.com>
>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


More information about the sssd-devel mailing list