[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