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.
bye, Sumit
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
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@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@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@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@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@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@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@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@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@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@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
On Wed, Apr 29, 2015 at 05:40:37PM +0200, Lukas Slebodnik wrote:
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 :-)
Thank you very much for the review and the fixes, I included most of your comments. For the few I skipped please find an explanation below.
- 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?
creat() uses O_CREAT|O_WRONLY|O_TRUNC, I found O_EXCL and O_NOFOLLOW important hence I used open()
- 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 = 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
unfortunately the overlap between the ldb attr and sysdb attr calls are quite minimal. ldb has int, unsigned int, int64_t and uint64_t while in sysdb we have long and uint32_t. So we have to add a new sysdb call, I'm just wondering what would be the most suitable one for enums. It looks like the expression that defines the value must be int, so maybe int might be the most useful choice. Do you have any suggestions here?
New version attached.
bye, Sumit
On (05/05/15 21:16), Sumit Bose wrote:
On Wed, Apr 29, 2015 at 05:40:37PM +0200, Lukas Slebodnik wrote:
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 :-)
Thank you very much for the review and the fixes, I included most of your comments. For the few I skipped please find an explanation below.
- 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?
creat() uses O_CREAT|O_WRONLY|O_TRUNC, I found O_EXCL and O_NOFOLLOW important hence I used open()
- 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 = 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
unfortunately the overlap between the ldb attr and sysdb attr calls are quite minimal. ldb has int, unsigned int, int64_t and uint64_t while in sysdb we have long and uint32_t. So we have to add a new sysdb call, I'm just wondering what would be the most suitable one for enums. It looks like the expression that defines the value must be int, so maybe int might be the most useful choice. Do you have any suggestions here?
OK
New version attached.
bye, Sumit
Please fix just small nitpick before pushing.
diff --git a/src/util/authtok-utils.c b/src/util/authtok-utils.c index b3be854..65fba90 100644 --- a/src/util/authtok-utils.c +++ b/src/util/authtok-utils.c @@ -55,7 +55,7 @@ errno_t sss_auth_pack_2fa_blob(const char *fa1, size_t fa1_len, }
*_2fa_blob_len = fa1_len + fa2_len + 2 + 2 * sizeof(uint32_t); - if (buf_len < *_2fa_blob_len) { + if (buf == NULL || buf_len < *_2fa_blob_len) { return EAGAIN; }
Otherwice clang expect that buf can be used as NULL. Because there is a condition at the begging of function if (fa1 == NULL || *fa1 == '\0' || fa1_len > UINT32_MAX || fa2 == NULL || *fa2 == '\0' || fa2_len > UINT32_MAX || (buf == NULL && buf_len != 0)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^ return EINVAL; }
I didn't noticed it before because it was reported in cofusing way Error: CLANG_WARNING: [#def6] sssd-1.12.90/src/util/authtok-utils.c:25: included_from: Included from here. sssd-1.12.90/src/sss_client/sss_cli.h:35: included_from: Included from here. sssd-1.12.90/src/util/util_safealign.h:51:5: warning: Null pointer passed as an argument to a 'nonnull' parameter # memcpy(dest, src, n); # ^~~~~~~~~~~~~~~~~~~~ # 49| safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) # 50| { # 51|-> memcpy(dest, src, n); # 52| if (counter) { # 53| *counter += n;
It was clear after preprocessing source code.
ACK http://sssd-ci.duckdns.org/logs/job/14/82/summary.html
LS
On Thu, May 07, 2015 at 08:00:23PM +0200, Lukas Slebodnik wrote:
On (05/05/15 21:16), Sumit Bose wrote:
On Wed, Apr 29, 2015 at 05:40:37PM +0200, Lukas Slebodnik wrote:
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 :-)
Thank you very much for the review and the fixes, I included most of your comments. For the few I skipped please find an explanation below.
- 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?
creat() uses O_CREAT|O_WRONLY|O_TRUNC, I found O_EXCL and O_NOFOLLOW important hence I used open()
- 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 = 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
unfortunately the overlap between the ldb attr and sysdb attr calls are quite minimal. ldb has int, unsigned int, int64_t and uint64_t while in sysdb we have long and uint32_t. So we have to add a new sysdb call, I'm just wondering what would be the most suitable one for enums. It looks like the expression that defines the value must be int, so maybe int might be the most useful choice. Do you have any suggestions here?
OK
New version attached.
bye, Sumit
Please fix just small nitpick before pushing.
diff --git a/src/util/authtok-utils.c b/src/util/authtok-utils.c index b3be854..65fba90 100644 --- a/src/util/authtok-utils.c +++ b/src/util/authtok-utils.c @@ -55,7 +55,7 @@ errno_t sss_auth_pack_2fa_blob(const char *fa1, size_t fa1_len, }
*_2fa_blob_len = fa1_len + fa2_len + 2 + 2 * sizeof(uint32_t);
- if (buf_len < *_2fa_blob_len) {
- if (buf == NULL || buf_len < *_2fa_blob_len) { return EAGAIN; }
Otherwice clang expect that buf can be used as NULL. Because there is a condition at the begging of function if (fa1 == NULL || *fa1 == '\0' || fa1_len > UINT32_MAX || fa2 == NULL || *fa2 == '\0' || fa2_len > UINT32_MAX || (buf == NULL && buf_len != 0)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^ return EINVAL; }
I didn't noticed it before because it was reported in cofusing way Error: CLANG_WARNING: [#def6] sssd-1.12.90/src/util/authtok-utils.c:25: included_from: Included from here. sssd-1.12.90/src/sss_client/sss_cli.h:35: included_from: Included from here. sssd-1.12.90/src/util/util_safealign.h:51:5: warning: Null pointer passed as an argument to a 'nonnull' parameter # memcpy(dest, src, n); # ^~~~~~~~~~~~~~~~~~~~ # 49| safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) # 50| { # 51|-> memcpy(dest, src, n); # 52| if (counter) { # 53| *counter += n;
It was clear after preprocessing source code.
ACK http://sssd-ci.duckdns.org/logs/job/14/82/summary.html
LS
Thank you very much for the thorough review.
Pushed to master: ea422c7061072c125eb53b40d7f3ca444d886913 bf6c3f07d653d474da9e43b2b7cced57fc4ea069 219f5b698fa72c0d5a8da2b0dd99daec3f924c94 2d0e7658198d1aa6e3926bf967ff683660249114 c5ae04b2da970a3991f21173acae3e892198ce0c 55b7fdd837a780ab0f71cbfaa2403f4626993922 932c3e22e3c59a9c33f30dcc09e6bef257e14320 e5698314b87e147c0223d0d8bcac206733dfae8c deb28a893c76f7c94b6cc8e596742665e23d97d5 4b1b2e60d0764fed289eada9a7afbfd1993cadcd fb045f6e5a9a7f8936ad6f89c28862dcd035a4fe ea98a7af0584d7667b6c07c19a4b22942c94ca5d bc052ea17d858c19f9cb9c9e2bc602e754f68831 80b5dbe123ec94c5a8fcb99f9a4953c1513deb58
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Apr 22, 2015 at 10:24:25AM +0200, Sumit Bose wrote:
+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)
Lukas would like to finish the review, so I'll just send the main question I had since I read the patches as well -- is it wise to use const char* here and not uint8_t? Are we sure there can't be NULL characters in either factors? If yes, then I guess it would make sense to use uint8_t...
On Wed, Apr 29, 2015 at 06:54:43PM +0200, Jakub Hrozek wrote:
On Wed, Apr 22, 2015 at 10:24:25AM +0200, Sumit Bose wrote:
+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)
Lukas would like to finish the review, so I'll just send the main question I had since I read the patches as well -- is it wise to use const char* here and not uint8_t? Are we sure there can't be NULL characters in either factors? If yes, then I guess it would make sense to use uint8_t...
Since both factors has to be entered at PAM password prompts I think we can be sure that they are real strings.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org