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 <sb...@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 <sb...@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 <sb...@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 <s...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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
patches.tar.gz
Description: application/gzip
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
