[SSSD] [PATCHES] Add special PAM prompting for 2FA authentication

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 29 15:40:37 UTC 2015


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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tar.gz
Type: application/gzip
Size: 8382 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150429/fa6f7bd3/attachment.bin>


More information about the sssd-devel mailing list