On (22/04/15 10:24), Sumit Bose wrote:
On Mon, Apr 06, 2015 at 08:58:16PM +0200, Sumit Bose wrote:
> Hi,
>
> this series of patches tries to add special prompting in the PAM dialog
> for 2-Factor-Authentication (2FA) and use it where suitable. As
> discussed on
>
https://fedorahosted.org/sssd/wiki/DesignDocs/PAMConversationForOTP if
> long-term password and one-time component are entered in a single prompt
> SSSD is in general not able to split them and use the long-term password
> of offline authentication or some user keyrings. With this patch-set an
> IPA user with configured OTP token will be prompted to enter the two
> factors separately (if the login application supports it) and the first
> factor (long-term password) will be save into the cache for offline
> authentication if it is long enough.
>
> Two new internals are needed for this. A special authentication token
> type for 2FA and a PAM pre-auth request to check if the given user user
> a standard password or 2FA/OTP.
>
> The first two patches enhance the authtok module to handle 2FA.
> Patches 4-6 add the pre-auth request to PAM client, responder and IPA
> backend. Patches 7-12 add the prompting, the minimal length check and
> the caching of the first factor. Finally tests for the PAM responder are
> added which should test existing and new behaviour.
>
Please find attached a new version of the patch set which contains a few
fixes which should silence a couple of compiler warnings.
bye,
Sumit
There are some coding style issues and clang warning
It shoudl be fixed with attached patches.
I wanted to simplyfy your job :-)
0002:
Applying: utils: add sss_authtok_[gs]et_2fa
/dev/shm/sssd/.git/rebase-apply/patch:660: trailing whitespace.
*/
/dev/shm/sssd/.git/rebase-apply/patch:684: trailing whitespace.
*/
/dev/shm/sssd/.git/rebase-apply/patch:335: new blank line at EOF.
+
warning: 3 lines add whitespace errors.
../sssd/src/sss_client/pam_sss.c:1228:49: error: cast from 'struct pam_conv **' to
'const void **' muve all intermediate pointers const qualified to be safe
[-Werror,-Wcast-qual]
ret = pam_get_item(pamh, 5, (const void **) &conv);
^
1 error generated.
From df63d2b3012242a847254081a232c61dbd7e2b8f Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Thu, 12 Feb 2015 21:53:15 +0100
Subject: [PATCH 01/14] Add leak check and command line option to test_authtok
---
Makefile.am | 3 ++
src/tests/cmocka/test_authtok.c | 67 +++++++++++++++++++++++++++++++++++------
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 52fbd510d67489dfd65b003c99673dc0869cfdc0..233d171b89ee2003881c58fb74fad808702746ae
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1858,11 +1858,14 @@ test_authtok_SOURCES = \
test_authtok_CFLAGS = \
$(AM_CFLAGS) \
$(TALLOC_CFLAGS) \
+ $(POPT_CFLAGS) \
$(DHASH_CFLAGS)
test_authtok_LDADD = \
$(TALLOC_LIBS) \
$(CMOCKA_LIBS) \
$(DHASH_LIBS) \
+ $(POPT_LIBS) \
+ libsss_test_common.la \
libsss_debug.la
sss_nss_idmap_tests_SOURCES = \
diff --git a/src/tests/cmocka/test_authtok.c b/src/tests/cmocka/test_authtok.c
index e37e92f68373d564f53b1267f078ea89c31ae051..0c7b7197fb2c03d69dc4df2310229ea100ad28d4
100644
--- a/src/tests/cmocka/test_authtok.c
+++ b/src/tests/cmocka/test_authtok.c
@@ -52,7 +54,12 @@ static int setup(void **state)
static int teardown(void **state)
{
struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
+
+ assert_non_null(ts);
+
+ assert_true(check_leaks_pop(ts) == true);
_check_leaks_pop
already return bool
talloc_free(ts);
+ assert_true(leak_check_teardown());
return 0;
}
From 3fe2e9fe20e41e4c8ed31ab94655d68debeb3268 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Wed, 7 Jan 2015 18:11:16 +0100
Subject: [PATCH 02/14] utils: add sss_authtok_[gs]et_2fa
---
Makefile.am | 5 ++
src/sss_client/pam_sss.c | 1 +
src/sss_client/sss_cli.h | 3 +
src/tests/cmocka/test_authtok.c | 154 +++++++++++++++++++++++++++++++++-
src/util/authtok-utils.c | 74 ++++++++++++++++
src/util/authtok-utils.h | 55 ++++++++++++
src/util/authtok.c | 182 ++++++++++++++++++++++++++++++++++++++++
src/util/authtok.h | 44 ++++++++++
8 files changed, 517 insertions(+), 1 deletion(-)
create mode 100644 src/util/authtok-utils.c
create mode 100644 src/util/authtok-utils.h
diff --git a/src/tests/cmocka/test_authtok.c b/src/tests/cmocka/test_authtok.c
index 0c7b7197fb2c03d69dc4df2310229ea100ad28d4..65828b82d5c8aa389df7db80df3deefe79f14a8b
100644
--- a/src/tests/cmocka/test_authtok.c
+++ b/src/tests/cmocka/test_authtok.c
@@ -311,6 +311,152 @@ static void test_sss_authtok_copy(void **state)
talloc_free(data);
}
+void test_sss_authtok_2fa(void **state)
+{
+ int ret;
+ const char *fa1;
+ size_t fa1_size;
+ const char *fa2;
+ size_t fa2_size;
+ struct test_state *ts;
+
+ ts = talloc_get_type_abort(*state, struct test_state);
+
+ ret = sss_authtok_set_2fa(NULL, "a", 0, "b", 0);
+ assert_int_equal(ret, EINVAL);
+
+ /* Test missing first factor */
+ ret = sss_authtok_set_2fa(ts->authtoken, NULL, 1, "b", 1);
+ assert_int_equal(ret, EINVAL);
+ /* Test missing second factor */
+ ret = sss_authtok_set_2fa(ts->authtoken, "a", 1, NULL, 1);
+ assert_int_equal(ret, EINVAL);
+ /* Test wrong first factor length */
+ ret = sss_authtok_set_2fa(ts->authtoken, "ab", 1, "b", 1);
+ assert_int_equal(ret, EINVAL);
+ /* Test wrong second factor length */
+ ret = sss_authtok_set_2fa(ts->authtoken, "a", 1, "bc", 1);
+ assert_int_equal(ret, EINVAL);
+
+ ret = sss_authtok_set_2fa(ts->authtoken, "a", 1, "bc", 2);
+ assert_int_equal(ret, EOK);
+ assert_int_equal(sss_authtok_get_size(ts->authtoken),
+ 2 * sizeof(uint32_t) + 5);
+ assert_int_equal(sss_authtok_get_type(ts->authtoken), SSS_AUTHTOK_TYPE_2FA);
+ assert_memory_equal(sss_authtok_get_data(ts->authtoken),
+ "\2\0\0\0\3\0\0\0a\0bc\0",
+ 2 * sizeof(uint32_t) + 5);
I
didn't tested it byt it might fail in big endian.
So it should be enough to use following tests with getters
This also applies to other places in this test.
+
+ ret = sss_authtok_get_2fa(ts->authtoken, &fa1, &fa1_size, &fa2,
&fa2_size);
+ assert_int_equal(ret, EOK);
+ assert_int_equal(fa1_size, 1);
+ assert_string_equal(fa1, "a");
+ assert_int_equal(fa2_size, 2);
+ assert_string_equal(fa2, "bc");
+
+ sss_authtok_set_empty(ts->authtoken);
+
+ /* check return code of empty token */
+ ret = sss_authtok_get_2fa(ts->authtoken, &fa1, &fa1_size, &fa2,
&fa2_size);
+ assert_int_equal(ret, ENOENT);
+
+ /* check return code for other token type */
+ ret = sss_authtok_set_password(ts->authtoken, "abc", 0);
here is a missing missing assert for ret.
+ ret = sss_authtok_get_2fa(ts->authtoken, &fa1,
&fa1_size, &fa2, &fa2_size);
+ assert_int_equal(ret, EACCES);
+
+ sss_authtok_set_empty(ts->authtoken);
+
+ /* check return code for garbage */
+ ret = sss_authtok_set(ts->authtoken, SSS_AUTHTOK_TYPE_2FA,
+ (const uint8_t *) "1111222233334444", 16);
+ assert_int_equal(ret, EINVAL);
+
+ sss_authtok_set_empty(ts->authtoken);
+}
/* Set debug level to invalid value so we can deside if -d 0 was used. */
diff --git a/src/util/authtok-utils.c b/src/util/authtok-utils.c
new file mode 100644
index 0000000000000000000000000000000000000000..3955e6667f0957fa456d8a33d0325299b56f8246
--- /dev/null
+++ b/src/util/authtok-utils.c
@@ -0,0 +1,74 @@
+/*
+ SSSD - auth utils helpers
+
+ Copyright (C) Sumit Bose <sbose(a)redhat.com> 2015
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
+*/
+
+/* This file is use by SSSD clients and the main daemons. Please do not add
+ * code which is specific to only one of them. */
+
+#include <errno.h>
+
+#include "sss_client/sss_cli.h"
+
+errno_t sss_auth_pack_2fa_blob(const char *fa1, size_t fa1_len,
+ const char *fa2, size_t fa2_len,
+ uint8_t *buf, size_t buf_len,
+ size_t *_2fa_blob_len)
+{
+ size_t c;
+ uint32_t tmp_uint32_t;
+
+ if (fa1 == NULL || fa1_len > UINT32_MAX || fa2 == NULL
+ || fa2_len > UINT32_MAX || (buf == NULL && buf_len != 0)) {
+ return EINVAL;
+ }
+
+ if (fa1_len == 0) {
+ fa1_len = strlen(fa1);
+ } else {
+ if (fa1[fa1_len] != '\0') {
+ return EINVAL;
+ }
+ }
+
+ if (fa2_len == 0) {
+ fa2_len = strlen(fa2);
+ } else {
+ if (fa2[fa2_len] != '\0') {
+ return EINVAL;
+ }
+ }
Do we want to allow strings fa1 and fa1 to have zero length ""?
Should we test it?
+
+ *_2fa_blob_len = fa1_len + fa2_len + 2 + 2 * sizeof(uint32_t);
+ if (buf_len < *_2fa_blob_len) {
+ return EAGAIN;
+ }
+
+ c = 0;
+ tmp_uint32_t = (uint32_t) fa1_len + 1;
+ SAFEALIGN_COPY_UINT32(buf, &tmp_uint32_t, &c);
+ tmp_uint32_t = (uint32_t) fa2_len + 1;
+ SAFEALIGN_COPY_UINT32(buf + c, &tmp_uint32_t, &c);
+
+ memcpy(buf + c, fa1, fa1_len + 1);
+ c += fa1_len + 1;
+
+ memcpy(buf + c, fa2, fa2_len + 1);
+
+ return 0;
+}
+
diff --git a/src/util/authtok-utils.h b/src/util/authtok-utils.h
new file mode 100644
index 0000000000000000000000000000000000000000..6ce294891a74fd66940e6f4eee5056beba697ee6
--- /dev/null
+++ b/src/util/authtok-utils.h
@@ -0,0 +1,55 @@
+/*
+ SSSD - auth utils helpers
+
+ Copyright (C) Sumit Bose <simo(a)redhat.com> 2015
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
+*/
+
+#ifndef __AUTHTOK_UTILS_H__
+#define __AUTHTOK_UTILS_H__
+
+#include <talloc.h>
+
+#include "sss_client/sss_cli.h"
+
+/**
+ * @brief Fill memory buffer with 2FA blob
+ *
+ * @param[in] fa1 First authentication factor, null terminated
+ * @param[in] fa1_len Length of the first authentication factor, if 0
+ * strlen() will be called internally
+ * @param[in] fa2 Second authentication factor, null terminated
+ * @param[in] fa2_len Length of the second authentication factor, if 0
+ * strlen() will be called internally
+ * @param[in] buf memory buffer of size buf_len
+ * @param[in] buf_len size of memory buffer buf
+ *
+ * @param[out] _2fa_blob_len size of the 2FA blob
+ *
+ * @return EOK on success
+ * EINVAL if input data is not consistent
+ * EAGAIN if provided buffer is too small, _2fa_blob_len
+ * contains the size needed to store the 2FA blob
+ */
+errno_t sss_auth_pack_2fa_blob(const char *fa1, size_t fa1_len,
+ const char *fa2, size_t fa2_len,
+ uint8_t *buf, size_t buf_len,
+ size_t *_2fa_blob_len);
+
+errno_t sss_auth_unpack_2fa_blob(TALLOC_CTX *mem_ctx,
+ const uint8_t *blob, size_t blob_len,
+ char **fa1, size_t *_fa1_len,
+ char **fa2, size_t *_fa2_len);
missing doc string
for 2nd function :-)
+errno_t sss_authtok_get_2fa(struct sss_auth_token *tok,
+ const char **fa1, size_t *fa1_len,
+ const char **fa2, size_t *fa2_len)
+{
+ size_t c;
+ uint32_t tmp_uint32_t;
+
+ if (tok->type != SSS_AUTHTOK_TYPE_2FA) {
+ return (tok->type == SSS_AUTHTOK_TYPE_EMPTY) ? ENOENT : EACCES;
+ }
+
+ if (tok->length < 2 * sizeof(uint32_t)) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Blob too small.\n");
+ return EINVAL;
+ }
+
+ c = 0;
+ SAFEALIGN_COPY_UINT32(&tmp_uint32_t, tok->data, &c);
+ *fa1_len = tmp_uint32_t - 1;
+ SAFEALIGN_COPY_UINT32(&tmp_uint32_t, tok->data + c, &c);
+ *fa2_len = tmp_uint32_t - 1;
+
+ if (*fa1_len == 0 || fa2_len == 0
^^^^^^^^^^^^
Do you want to test pointer here or value of "*fa2_len"?
+ || tok->length != 2 * sizeof(uint32_t) + *fa1_len +
*fa2_len + 2) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Blob size mismatch.\n");
+ return EINVAL;
+ }
+
+ if (tok->data[c + *fa1_len] != '\0'
+ || tok->data[c + *fa1_len + 1 + *fa2_len] != '\0') {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Missing terminating null character.\n");
+ return EINVAL;
+ }
+
+ *fa1 = (const char *) tok->data + c;
+ *fa2 = (const char *) tok->data + c + *fa1_len + 1;
We should document that
this function returns internal data
and should not be modified.
It does not worth to call strdup here; documentation should be sufficient.
+
+ return EOK;
+}
From 6fbeeba4eb1543620a8b3dc3bee7894253ccb924 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 24 Mar 2015 17:26:53 +0100
Subject: [PATCH 05/14] krb5-child: add preauth and split 2fa token support
---
src/providers/krb5/krb5_auth.c | 3 +-
src/providers/krb5/krb5_child.c | 289 ++++++++++++++++++++++++++++----
src/providers/krb5/krb5_child_handler.c | 4 +
src/sss_client/sss_cli.h | 6 +
4 files changed, 268 insertions(+), 34 deletions(-)
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -268,7 +271,88 @@ static int token_pin_destructor(char *mem)
return 0;
}
-static krb5_error_code tokeninfo_matches(TALLOC_CTX *mem_ctx,
+static krb5_error_code tokeninfo_matches_2fa(TALLOC_CTX *mem_ctx,
+ const krb5_responder_otp_tokeninfo *ti,
+ const char *fa1, size_t fa1_len,
+ const char *fa2, size_t fa2_len,
+ char **out_token, char **out_pin)
+{
+ char *token = NULL, *pin = NULL;
+ checker check = NULL;
+ int i;
+
+
+ if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_NEXTOTP) {
+ return ENOTSUP;
+ }
+
+ if (ti->challenge != NULL) {
+ return ENOTSUP;
+ }
+
+ /* This is a non-sensical value. */
+ if (ti->length == 0) {
+ return EPROTO;
+ }
+
+ if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_COLLECT_TOKEN) {
+ if (ti->length > 0 && ti->length != fa2_len) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Expected [%d] and given [%zu] token size " \
+ "do not match.\n", ti->length, fa2_len);
+ return EMSGSIZE;
+ }
+
+ if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_COLLECT_PIN) {
+ if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_SEPARATE_PIN) {
+
+ pin = talloc_strndup(mem_ctx, fa1, fa1_len);
+ if (pin == NULL) {
+ talloc_free(token);
+ return ENOMEM;
+ }
+ talloc_set_destructor(pin, token_pin_destructor);
+
+ token = talloc_strndup(mem_ctx, fa2, fa2_len);
+ if (token == NULL) {
in this case
"pin" would be still allocated on
mem_ctx. but in cese of we would not be able
to do anything. I checked and it is allocated on krb5_req
which isn't long term. So it shoudl be fine.
+ return ENOMEM;
+ }
+ talloc_set_destructor(token, token_pin_destructor);
+
+ check = pick_checker(ti->format);
+ }
+ } else {
+ token = talloc_asprintf(mem_ctx, "%s%s", fa1, fa2);
+ if (token == NULL) {
+ return ENOMEM;
+ }
+ talloc_set_destructor(token, token_pin_destructor);
+
+ check = pick_checker(ti->format);
+ }
+ } else {
+ /* Assuming PIN only required */
+ pin = talloc_strndup(mem_ctx, fa1, fa1_len);
+ if (pin == NULL) {
+ return ENOMEM;
+ }
+ talloc_set_destructor(pin, token_pin_destructor);
+ }
+
+ /* If check is set, we need to verify the contents of the token. */
+ for (i = 0; check != NULL && token[i] != '\0'; i++) {
+ if (!check(token[i])) {
+ talloc_free(token);
+ talloc_free(pin);
+ return EBADMSG;
+ }
+ }
+
+ *out_token = token;
+ *out_pin = pin;
+ return 0;
+}
+static krb5_error_code tokeninfo_matches_pwd(TALLOC_CTX *mem_ctx,
const krb5_responder_otp_tokeninfo *ti,
const char *pwd, size_t len,
char **out_token, char **out_pin)
@@ -364,15 +448,52 @@ static krb5_error_code tokeninfo_matches(TALLOC_CTX *mem_ctx,
return 0;
}
static krb5_error_code answer_otp(krb5_context ctx,
struct krb5_req *kr,
krb5_responder_context rctx)
{
krb5_responder_otp_challenge *chl;
char *token = NULL, *pin = NULL;
- const char *pwd = NULL;
krb5_error_code ret;
- size_t i, len;
+ size_t i;
ret = krb5_responder_otp_get_challenge(ctx, rctx, &chl);
if (ret != EOK || chl == NULL) {
@@ -388,14 +509,37 @@ static krb5_error_code answer_otp(krb5_context ctx,
kr->otp = true;
- /* Validate our assumptions about the contents of authtok. */
- ret = sss_authtok_get_password(kr->pd->authtok, &pwd, &len);
- if (ret != EOK)
- goto done;
+ if (kr->pd->cmd == SSS_PAM_PREAUTH) {
+ for (i = 0; chl->tokeninfo[i] != NULL; i++) {
+ DEBUG(SSSDBG_TRACE_ALL, "[%zu] Vendor [%s].\n",
+ i, chl->tokeninfo[i]->vendor);
+ DEBUG(SSSDBG_TRACE_ALL, "[%zu] Token-ID [%s].\n",
+ i, chl->tokeninfo[i]->token_id);
+ DEBUG(SSSDBG_TRACE_ALL, "[%zu] Challenge [%s].\n",
+ i, chl->tokeninfo[i]->challenge);
+ DEBUG(SSSDBG_TRACE_ALL, "[%zu] Flags [%d].\n",
+ i, chl->tokeninfo[i]->flags);
+ }
You iterate over array chl->tokeninfo.
After for loof chl->tokeninfo[i] will be NULL.
+
+ if (chl->tokeninfo[0]->vendor != NULL) {
+ kr->otp_vendor = talloc_strdup(kr, chl->tokeninfo[i]->vendor);
^^^^^^^^^^^^^^^^^
dereference of NULL pointer?
Do I read code correctly?
The same applies to next two if statements.
+ if (chl->tokeninfo[0]->token_id != NULL) {
+ kr->otp_token_id = talloc_strdup(kr, chl->tokeninfo[i]->token_id);
+ }
+ if (chl->tokeninfo[0]->challenge != NULL) {
+ kr->otp_challenge = talloc_strdup(kr,
chl->tokeninfo[i]->challenge);
+ }
+ /* Allocation errors are ignored on purpose */
+
+ DEBUG(SSSDBG_TRACE_INTERNAL, "Exit answer_otp during pre-auth.\n");
+ return EAGAIN;
+ }
+static errno_t k5c_attach_otp_info_msg(struct krb5_req *kr)
+{
+ uint8_t *msg = NULL;
+ size_t msg_len;
+ int ret;
+ size_t vendor_len = 0;
+ size_t token_id_len = 0;
+ size_t challenge_len = 0;
+
+ msg_len = 3;
+ if (kr->otp_vendor != NULL) {
+ vendor_len = strlen(kr->otp_vendor);
+ msg_len += vendor_len;
+ }
+
+ if (kr->otp_token_id != NULL) {
+ token_id_len = strlen(kr->otp_token_id);
+ msg_len += token_id_len;
+ }
+
+ if (kr->otp_challenge != NULL) {
+ challenge_len = strlen(kr->otp_challenge);
+ msg_len += challenge_len;
+ }
+
+ msg = talloc_zero_size(kr, msg_len);
+ if (msg == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_size failed.\n");
+ return ENOMEM;
+ }
+
+ if (kr->otp_vendor != NULL) {
+ memcpy(msg, kr->otp_vendor, vendor_len);
+ }
+
+ if (kr->otp_token_id != NULL) {
+ memcpy(msg + vendor_len + 1, kr->otp_token_id, token_id_len);
+ }
+
+ if (kr->otp_challenge != NULL) {
+ memcpy(msg + vendor_len + token_id_len + 2 , kr->otp_challenge,
+ challenge_len);
+ }
I would prefer to use local "index "variable with length or
increment
temporary pointer instead of "msg + vendor_len + token_id_len + 2"
+ ret = pam_add_response(kr->pd, SSS_PAM_OTP_INFO, msg_len,
msg);
+ talloc_zfree(msg);
+
+ return ret;
+}
+
From c410b69381540c3db3c2f5f0396b6b8162bca764 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 24 Mar 2015 11:19:46 +0100
Subject: [PATCH 06/14] IPA: create preauth indicator file at startup
---
src/providers/ipa/ipa_init.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
src/sss_client/sss_cli.h | 2 ++
2 files changed, 68 insertions(+)
diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c
index 4b26e8baad4d0592729aec9a0b188ae89973fa98..1560da1e8a89c67453e79243acc378abdd30d565
100644
--- a/src/providers/ipa/ipa_init.c
+++ b/src/providers/ipa/ipa_init.c
@@ -371,6 +371,62 @@ done:
return ret;
}
+void cleanup_ipa_preauth_indicator(void)
+{
+ int ret;
+
+ ret = unlink(PAM_PREAUTH_INDICATOR);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Failed to remove preauth indicator file [%s].\n",
+ PAM_PREAUTH_INDICATOR);
+ }
+}
+
+static errno_t create_ipa_preauth_indicator(void)
+{
+ int ret;
+ TALLOC_CTX *tmp_ctx = NULL;
+ int fd;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
+ return ENOMEM;
+ }
+
+ fd = open(PAM_PREAUTH_INDICATOR, O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW,
+ 0644);
Can we use syscall "creat" or do we need
special flags?
+ if (fd < 0) {
+ if (errno != EEXIST) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Failed to create preauth indicator file [%s].\n",
+ PAM_PREAUTH_INDICATOR);
+ ret = EOK;
+ goto done;
+ }
+
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Preauth indicator file [%s] already exists. " \
+ "Maybe it is left after an unplanned exit. Continuing.\n",
+ PAM_PREAUTH_INDICATOR);
+ } else {
+ close(fd);
+ }
+
+ ret = atexit(cleanup_ipa_preauth_indicator);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "atexit failed. Continuing.\n");
+ }
+
+ ret = EOK;
+
+done:
+ talloc_free(tmp_ctx);
+
+ return ret;
+}
+
From c8e85195377cb4878a1c879d28ec2b217cb01a38 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Thu, 12 Feb 2015 23:08:12 +0100
Subject: [PATCH 07/14] pam_sss: add pre-auth and 2fa support
---
Makefile.am | 1 +
src/sss_client/pam_sss.c | 226 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 225 insertions(+), 2 deletions(-)
There are some resource leaks caused by this patch
Error: RESOURCE_LEAK (CWE-772): [#def6]
sssd-1.12.90/src/sss_client/pam_sss.c:929: alloc_fn: Storage is returned from allocation
function "strdup".
sssd-1.12.90/src/sss_client/pam_sss.c:929: var_assign: Assigning:
"pi->otp_vendor" = storage returned from "strdup((char *)(buf +
p))".
sssd-1.12.90/src/sss_client/pam_sss.c:935: noescape: Resource
"pi->otp_vendor" is not freed or pointed-to in "strlen".
sssd-1.12.90/src/sss_client/pam_sss.c:929: overwrite_var: Overwriting
"pi->otp_vendor" in "pi->otp_vendor = strdup((char *)(buf + p))"
leaks the storage that "pi->otp_vendor" points to.
# 927| }
# 928|
# 929|-> pi->otp_vendor = strdup((char *) &buf[p]);
# 930| if (pi->otp_vendor == NULL) {
# 931| D(("strdup failed"));
Error: RESOURCE_LEAK (CWE-772): [#def7]
sssd-1.12.90/src/sss_client/pam_sss.c:940: alloc_fn: Storage is returned from allocation
function "strdup".
sssd-1.12.90/src/sss_client/pam_sss.c:940: var_assign: Assigning:
"pi->otp_token_id" = storage returned from "strdup((char *)(buf + (p +
offset)))".
sssd-1.12.90/src/sss_client/pam_sss.c:946: noescape: Resource
"pi->otp_token_id" is not freed or pointed-to in "strlen".
sssd-1.12.90/src/sss_client/pam_sss.c:940: overwrite_var: Overwriting
"pi->otp_token_id" in "pi->otp_token_id = strdup((char *)(buf + (p +
offset)))" leaks the storage that "pi->otp_token_id" points to.
# 938| break;
# 939| }
# 940|-> pi->otp_token_id = strdup((char *) &buf[p + offset]);
# 941| if (pi->otp_token_id == NULL) {
# 942| D(("strdup failed"));
Error: RESOURCE_LEAK (CWE-772): [#def8]
sssd-1.12.90/src/sss_client/pam_sss.c:951: alloc_fn: Storage is returned from allocation
function "strdup".
sssd-1.12.90/src/sss_client/pam_sss.c:951: var_assign: Assigning:
"pi->otp_challenge" = storage returned from "strdup((char *)(buf + (p +
offset)))".
sssd-1.12.90/src/sss_client/pam_sss.c:951: overwrite_var: Overwriting
"pi->otp_challenge" in "pi->otp_challenge = strdup((char *)(buf + (p
+ offset)))" leaks the storage that "pi->otp_challenge" points to.
# 949| break;
# 950| }
# 951|-> pi->otp_challenge = strdup((char *) &buf[p +
offset]);
# 952| if (pi->otp_challenge == NULL) {
# 953| D(("strdup failed"));
Error: RESOURCE_LEAK (CWE-772): [#def9]
sssd-1.12.90/src/sss_client/pam_sss.c:1634: alloc_arg:
"get_authtok_for_password_change" allocates memory that is stored into
"pi.pam_authtok".
sssd-1.12.90/src/sss_client/pam_sss.c:1498:13: alloc_arg: "prompt_password"
allocates memory that is stored into "pi->pam_authtok".
sssd-1.12.90/src/sss_client/pam_sss.c:1203:9: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.12.90/src/sss_client/pam_sss.c:1203:9: var_assign: Assigning:
"pi->pam_authtok" = "strdup(answer)".
sssd-1.12.90/src/sss_client/pam_sss.c:1638: leaked_storage: Variable "pi" going
out of scope leaks the storage "pi.pam_authtok" points to.
# 1636| D(("failed to get tokens for password change:
%s",
# 1637| pam_strerror(pamh, ret)));
# 1638|-> return ret;
# 1639| }
# 1640| if (pam_flags & PAM_PRELIM_CHECK) {
From 5ef4f44bd2feb221dbe70c3748fa09e522ac98a0 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Tue, 24 Mar 2015 15:35:01 +0100
Subject: [PATCH 09/14] sysdb: add sysdb_cache_password_ex()
---
src/db/sysdb.h | 9 +++++++++
src/db/sysdb_ops.c | 25 ++++++++++++++++++++---
src/tests/sysdb-tests.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 84 insertions(+), 3 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 1f84a60ba332d70529b2170c04415d7fc0704597..b4b2eb9fc2ba6055fcfc770601c04815464228cd
100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2234,9 +2234,11 @@ int sysdb_remove_group_member(struct sss_domain_info *domain,
/* =Password-Caching====================================================== */
-int sysdb_cache_password(struct sss_domain_info *domain,
- const char *username,
- const char *password)
+int sysdb_cache_password_ex(struct sss_domain_info *domain,
+ const char *username,
+ const char *password,
+ enum sss_authtok_type authtok_type,
+ size_t second_factor_len)
{
TALLOC_CTX *tmp_ctx;
struct sysdb_attrs *attrs;
@@ -2269,6 +2271,15 @@ int sysdb_cache_password(struct sss_domain_info *domain,
ret = sysdb_attrs_add_string(attrs, SYSDB_CACHEDPWD, hash);
if (ret) goto fail;
+ ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_TYPE, authtok_type);
+ if (ret) goto fail;
"enum sss_authtok_type" is stored as long but
read as int
sh$ $git grep SYSDB_CACHEDPWD_TYPE
src/db/sysdb.h-#define SYSDB_CACHEDPWD "cachedPassword"
src/db/sysdb.h:#define SYSDB_CACHEDPWD_TYPE "cachedPasswordType"
--
src/db/sysdb_ops.c-
src/db/sysdb_ops.c: ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_TYPE,
authtok_type);
--
src/db/sysdb_ops.c- cached_authtok_type = ldb_msg_find_attr_as_uint(ldb_msg,
src/db/sysdb_ops.c:
SYSDB_CACHEDPWD_TYPE,
--
src/db/sysdb_ops.c- "accountExpires",
SYSDB_FAILED_LOGIN_ATTEMPTS,
src/db/sysdb_ops.c: SYSDB_LAST_FAILED_LOGIN,
SYSDB_CACHEDPWD_TYPE,
--
src/tests/sysdb-tests.c- struct ldb_result *res;
src/tests/sysdb-tests.c: const char *attrs[] = { SYSDB_CACHEDPWD_TYPE,
SYSDB_CACHEDPWD_FA2_LEN,
--
src/tests/sysdb-tests.c-
src/tests/sysdb-tests.c: val = ldb_msg_find_attr_as_int(res->msgs[0],
SYSDB_CACHEDPWD_TYPE, 0);
--
src/tests/sysdb-tests.c-
src/tests/sysdb-tests.c: val = ldb_msg_find_attr_as_int(res->msgs[0],
SYSDB_CACHEDPWD_TYPE, 0);
C standard does not say exactly the size of enum, but int should be enough
and type should be the same for read and store
+
+ if (authtok_type == SSS_AUTHTOK_TYPE_2FA && second_factor_len > 0) {
+ ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_FA2_LEN,
+ second_factor_len);
The same applies here
sh$ $git grep -C1 SYSDB_CACHEDPWD_FA2_LEN
src/db/sysdb.h-#define SYSDB_CACHEDPWD_TYPE "cachedPasswordType"
src/db/sysdb.h:#define SYSDB_CACHEDPWD_FA2_LEN "cachedPasswordSecondFactorLen"
src/db/sysdb.h-
--
src/db/sysdb_ops.c- if (authtok_type == SSS_AUTHTOK_TYPE_2FA &&
second_factor_len > 0) {
src/db/sysdb_ops.c: ret = sysdb_attrs_add_long(attrs, SYSDB_CACHEDPWD_FA2_LEN,
src/db/sysdb_ops.c- second_factor_len);
--
src/db/sysdb_ops.c-
src/db/sysdb_ops.c: cached_fa2_len = ldb_msg_find_attr_as_uint(ldb_msg,
SYSDB_CACHEDPWD_FA2_LEN,
src/db/sysdb_ops.c- 0);
--
src/db/sysdb_ops.c- SYSDB_LAST_FAILED_LOGIN,
SYSDB_CACHEDPWD_TYPE,
src/db/sysdb_ops.c: SYSDB_CACHEDPWD_FA2_LEN, NULL };
src/db/sysdb_ops.c- struct ldb_message *ldb_msg;
--
src/tests/sysdb-tests.c- struct ldb_result *res;
src/tests/sysdb-tests.c: const char *attrs[] = { SYSDB_CACHEDPWD_TYPE,
SYSDB_CACHEDPWD_FA2_LEN,
src/tests/sysdb-tests.c- NULL };
--
src/tests/sysdb-tests.c-
src/tests/sysdb-tests.c: val = ldb_msg_find_attr_as_int(res->msgs[0],
SYSDB_CACHEDPWD_FA2_LEN, 0);
src/tests/sysdb-tests.c- fail_unless(val == 12,
+ if (ret) goto fail;
+ }
+
/* FIXME: should we use a different attribute for chache passwords ?? */
ret = sysdb_attrs_add_long(attrs, "lastCachedPasswordChange",
(long)time(NULL));
int sysdb_search_custom(TALLOC_CTX *mem_ctx,
From dc213d592ccb2e622127cafd9fd32eff1201de83 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Thu, 19 Mar 2015 13:12:11 +0100
Subject: [PATCH 14/14] PAM: add PAM responder unit test
---
Makefile.am | 32 ++
src/tests/cmocka/test_pam_srv.c | 907 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 939 insertions(+)
create mode 100644 src/tests/cmocka/test_pam_srv.c
diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
new file mode 100644
index 0000000000000000000000000000000000000000..13e808f3eb5bd0b5410c7d3e5659bc9ac31298bc
--- /dev/null
+++ b/src/tests/cmocka/test_pam_srv.c
@@ -0,0 +1,907 @@
+/*
+ Authors:
+ Sumit Bose <sbose(a)redhat.com>
+
+ Copyright (C) 2015 Red Hat
+
+ SSSD tests: PAM responder tests
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
+*/
+
+#include <security/pam_modules.h>
+#include <popt.h>
+
+#include "tests/cmocka/common_mock.h"
+#include "tests/cmocka/common_mock_resp.h"
+#include "responder/common/responder_packet.h"
+#include "responder/common/negcache.h"
+#include "responder/pam/pamsrv.h"
+#include "responder/pam/pam_helpers.h"
+#include "sss_client/pam_message.h"
+#include "sss_client/sss_cli.h"
+
+#include "util/crypto/nss/nss_util.h"
+
+#define TESTS_PATH "tests_pam"
+#define TEST_CONF_DB "test_pam_conf.ldb"
+#define TEST_DOM_NAME "pam_test"
+#define TEST_SUBDOM_NAME "test.subdomain"
+#define TEST_ID_PROVIDER "ldap"
+
+struct pam_test_ctx {
+ struct sss_test_ctx *tctx;
+ struct sss_domain_info *subdom;
+
+ struct resp_ctx *rctx;
+ struct cli_ctx *cctx;
+ struct sss_cmd_table *pam_cmds;
+ struct pam_ctx *pctx;
+
+ int ncache_hits;
+ int exp_pam_status;
+};
+
+/* Must be global because it is needed in some wrappers */
+struct pam_test_ctx *pam_test_ctx;
+
+struct pam_ctx *mock_pctx(TALLOC_CTX *mem_ctx)
+{
+ struct pam_ctx *pctx;
+ errno_t ret;
+
+ pctx = talloc_zero(mem_ctx, struct pam_ctx);
Please use asserts instean of null
check
We cannot recover anyway.
+ if (!pctx) {
+ return NULL;
+ }
+
+ ret = sss_ncache_init(pctx, &pctx->ncache);
The same here
+ if (ret != EOK) {
+ talloc_free(pctx);
+ return NULL;
+ }
+ pctx->neg_timeout = 10;
+
+ ret = sss_hash_create(pctx, 10, &pctx->id_table);
and here
+ if (ret != EOK) {
+ talloc_free(pctx);
+ return NULL;
+ }
+
+ return pctx;
+}
+
+int main(int argc, const char *argv[])
+{
+ int rv;
+ int no_cleanup = 0;
+ poptContext pc;
+ int opt;
+ struct poptOption long_options[] = {
+ POPT_AUTOHELP
+ SSSD_DEBUG_OPTS
+ {"no-cleanup", 'n', POPT_ARG_NONE, &no_cleanup, 0,
+ _("Do not delete the test database after a test run"), NULL },
+ POPT_TABLEEND
+ };
+
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test_setup_teardown(test_pam_authenticate,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_setcreds,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_acct_mgmt,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_open_session,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_close_session,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_chauthtok,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_chauthtok_prelim,
+ pam_test_setup, pam_test_teardown),
Really
good code coverage. +1 :-)
It would be good to also cover testcase
pd->pam_status == PAM_AUTHINFO_UNAVAIL
pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM (or SSS_PAM_CHAUTHTOK)
because the next one is covered
pd->pam_status == PAM_AUTHINFO_UNAVAIL
pd->cmd == SSS_PAM_AUTHENTICATE
+ cmocka_unit_test_setup_teardown(test_pam_preauth,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_offline_auth_no_hash,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_offline_auth_success,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_offline_auth_wrong_pw,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_offline_auth_success_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(test_pam_offline_auth_failed_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_success_2fa_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_failed_2fa_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_success_pw_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_failed_pw_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_success_combined_pw_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_failed_combined_pw_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
+ cmocka_unit_test_setup_teardown(
+ test_pam_offline_auth_failed_wrong_2fa_size_with_cached_2fa,
+ pam_test_setup, pam_test_teardown),
LS