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(a)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