rpms/sssd
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Fixes for passkey authentication

Browse Source 
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This commit is contained in:
Iker Pedrosa 2023-10-03 13:01:09 +02:00
parent b3524fa5a8
commit cddbd22e06
4 changed files with 502 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

199
0001-passkey-kerberos-preauth-false-uv.patch Normal file
View File

@ -0,0 +1,199 @@
From 6cedcf217a10ff8b7bfc2d5a0af65e85a5434082 Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstephen@redhat.com>
Date: Fri, 8 Sep 2023 10:36:17 -0400
Subject: [PATCH 1/2] Passkey: Allow kerberos preauth for "false" UV
When IPA passkey configuration sets require-user-verification=false
then the user verification value will be 0. We need to allow this
configuration within the plugin.
---
src/krb5_plugin/passkey/passkey_utils.c | 2 +-
src/tests/cmocka/test_krb5_passkey_plugin.c | 10 ----------
2 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/src/krb5_plugin/passkey/passkey_utils.c b/src/krb5_plugin/passkey/passkey_utils.c
index 3e777aedc6..c41b9c00e7 100644
--- a/src/krb5_plugin/passkey/passkey_utils.c
+++ b/src/krb5_plugin/passkey/passkey_utils.c
@@ -211,7 +211,7 @@ sss_passkey_challenge_to_json_object(const struct sss_passkey_challenge *data)
/* These are required fields. */
if (data->domain == NULL || data->credential_id_list == NULL
- || data->user_verification == 0 || data->cryptographic_challenge == NULL) {
+ || data->cryptographic_challenge == NULL) {
return NULL;
}
diff --git a/src/tests/cmocka/test_krb5_passkey_plugin.c b/src/tests/cmocka/test_krb5_passkey_plugin.c
index 1a0c87adcb..9593a65468 100644
--- a/src/tests/cmocka/test_krb5_passkey_plugin.c
+++ b/src/tests/cmocka/test_krb5_passkey_plugin.c
@@ -120,16 +120,6 @@ void test_sss_passkey_message_encode__challenge(void **state)
str = sss_passkey_message_encode(&message);
assert_null(str);
- challenge.domain = discard_const("domain");
- challenge.credential_id_list = discard_const(id_list);
- challenge.user_verification = 0;
- challenge.cryptographic_challenge = discard_const("crypto-challenge");
- message.phase = SSS_PASSKEY_PHASE_CHALLENGE;
- message.state = discard_const("abcd");
- message.data.challenge = &challenge;
- str = sss_passkey_message_encode(&message);
- assert_null(str);
-
challenge.domain = discard_const("domain");
challenge.credential_id_list = discard_const(id_list);
challenge.user_verification = 1;
From dd8b46933858ab0fc1b11504ff88e0c74193cc2b Mon Sep 17 00:00:00 2001
From: Iker Pedrosa <ipedrosa@redhat.com>
Date: Mon, 18 Sep 2023 10:22:17 +0200
Subject: [PATCH 2/2] passkey: omit user-verification
If user-verification is disabled and the key doesn't support it, then
omit it. Otherwise, the authentication will produce an error and the
user will be unable to authenticate.
I have also added a unit-test to check this condition.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
(cherry picked from commit a8daf9790906b7321024fef8e636f9c1b14343ab)
---
Makefile.am | 1 +
src/passkey_child/passkey_child_devices.c | 12 +++++++++
src/tests/cmocka/test_passkey_child.c | 32 +++++++++++++++++++++++
3 files changed, 45 insertions(+)
diff --git a/Makefile.am b/Makefile.am
index 4bac10d86d..640acbd953 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3860,6 +3860,7 @@ test_passkey_LDFLAGS = \
-Wl,-wrap,fido_dev_open \
-Wl,-wrap,fido_dev_has_uv \
-Wl,-wrap,fido_dev_has_pin \
+ -Wl,-wrap,fido_dev_supports_uv \
-Wl,-wrap,fido_dev_make_cred \
-Wl,-wrap,fido_cred_x5c_ptr \
-Wl,-wrap,fido_cred_verify \
diff --git a/src/passkey_child/passkey_child_devices.c b/src/passkey_child/passkey_child_devices.c
index a91ab9ca11..2011b12f66 100644
--- a/src/passkey_child/passkey_child_devices.c
+++ b/src/passkey_child/passkey_child_devices.c
@@ -201,10 +201,12 @@ get_device_options(fido_dev_t *dev, struct passkey_data *_data)
{
bool has_pin;
bool has_uv;
+ bool supports_uv;
errno_t ret;
has_uv = fido_dev_has_uv(dev);
has_pin = fido_dev_has_pin(dev);
+ supports_uv = fido_dev_supports_uv(dev);
if (_data->user_verification == FIDO_OPT_TRUE && has_pin != true
&& has_uv != true) {
@@ -226,6 +228,16 @@ get_device_options(fido_dev_t *dev, struct passkey_data *_data)
goto done;
}
+ if (_data->user_verification == FIDO_OPT_FALSE
+ && supports_uv == false) {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "Policy disabled user-verification but the key doesn't support "
+ "it. Thus, omitting the user-verification.\n");
+ _data->user_verification = FIDO_OPT_OMIT;
+ ret = EOK;
+ goto done;
+ }
+
ret = EOK;
done:
diff --git a/src/tests/cmocka/test_passkey_child.c b/src/tests/cmocka/test_passkey_child.c
index 187b3c49ea..5003152c45 100644
--- a/src/tests/cmocka/test_passkey_child.c
+++ b/src/tests/cmocka/test_passkey_child.c
@@ -278,6 +278,16 @@ __wrap_fido_dev_has_pin(fido_dev_t *dev)
return ret;
}
+bool
+__wrap_fido_dev_supports_uv(fido_dev_t *dev)
+{
+ bool ret;
+
+ ret = mock();
+
+ return ret;
+}
+
int
__wrap_fido_dev_make_cred(fido_dev_t *dev, fido_cred_t *cred, const char *pin)
{
@@ -1090,6 +1100,7 @@ void test_get_device_options(void **state)
ts->data.user_verification = FIDO_OPT_TRUE;
will_return(__wrap_fido_dev_has_uv, true);
will_return(__wrap_fido_dev_has_pin, true);
+ will_return(__wrap_fido_dev_supports_uv, true);
ret = get_device_options(dev, &ts->data);
@@ -1106,12 +1117,30 @@ void test_get_device_options_user_verification_unmatch(void **state)
ts->data.user_verification = FIDO_OPT_TRUE;
will_return(__wrap_fido_dev_has_uv, false);
will_return(__wrap_fido_dev_has_pin, false);
+ will_return(__wrap_fido_dev_supports_uv, false);
ret = get_device_options(dev, &ts->data);
assert_int_equal(ret, EINVAL);
}
+void test_get_device_options_user_verification_false_not_supported(void **state)
+{
+ struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
+ fido_dev_t *dev = NULL;
+ errno_t ret;
+
+ ts->data.user_verification = FIDO_OPT_FALSE;
+ will_return(__wrap_fido_dev_has_uv, false);
+ will_return(__wrap_fido_dev_has_pin, false);
+ will_return(__wrap_fido_dev_supports_uv, false);
+
+ ret = get_device_options(dev, &ts->data);
+
+ assert_int_equal(ret, FIDO_OK);
+ assert_int_equal(ts->data.user_verification, FIDO_OPT_OMIT);
+}
+
void test_request_assert(void **state)
{
struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
@@ -1206,6 +1235,7 @@ void test_authenticate_integration(void **state)
}
will_return(__wrap_fido_dev_has_uv, false);
will_return(__wrap_fido_dev_has_pin, false);
+ will_return(__wrap_fido_dev_supports_uv, false);
will_return(__wrap_fido_assert_set_uv, FIDO_OK);
will_return(__wrap_fido_assert_set_clientdata_hash, FIDO_OK);
will_return(__wrap_fido_dev_has_uv, false);
@@ -1256,6 +1286,7 @@ void test_get_assert_data_integration(void **state)
}
will_return(__wrap_fido_dev_has_uv, false);
will_return(__wrap_fido_dev_has_pin, false);
+ will_return(__wrap_fido_dev_supports_uv, false);
will_return(__wrap_fido_assert_set_uv, FIDO_OK);
will_return(__wrap_fido_dev_has_uv, false);
will_return(__wrap_fido_dev_has_pin, false);
@@ -1358,6 +1389,7 @@ int main(int argc, const char *argv[])
cmocka_unit_test_setup_teardown(test_get_authenticator_data_multiple_keys_assert_not_found, setup, teardown),
cmocka_unit_test_setup_teardown(test_get_device_options, setup, teardown),
cmocka_unit_test_setup_teardown(test_get_device_options_user_verification_unmatch, setup, teardown),
+ cmocka_unit_test_setup_teardown(test_get_device_options_user_verification_false_not_supported, setup, teardown),
cmocka_unit_test_setup_teardown(test_request_assert, setup, teardown),
cmocka_unit_test_setup_teardown(test_verify_assert, setup, teardown),
cmocka_unit_test_setup_teardown(test_verify_assert_failed, setup, teardown),

268
0002-increase-child-buffer-size.patch Normal file
View File

@ -0,0 +1,268 @@
From efee73c522f7ba755fe97ccb31add08e3d4c371f Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstephen@redhat.com>
Date: Fri, 22 Sep 2023 10:37:41 -0400
Subject: [PATCH 1/2] tests: Improve read write pipe child tests
Add test for multiple reads with a large message, and
add tests for child read/write safe calls.
---
src/tests/cmocka/test_child_common.c | 91 +++++++++++++++++++++++-----
1 file changed, 77 insertions(+), 14 deletions(-)
diff --git a/src/tests/cmocka/test_child_common.c b/src/tests/cmocka/test_child_common.c
index 189d62cd93..18d5b0b6a2 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -30,6 +30,8 @@
#define TEST_BIN "dummy-child"
#define ECHO_STR "Hello child"
+#define ECHO_LARGE_STR "Lorem ipsum dolor sit amet consectetur adipiscing elit, urna consequat felis vehicula class ultricies mollis dictumst, aenean non a in donec nulla. Phasellus ante pellentesque erat cum risus consequat imperdiet aliquam, integer placerat et turpis mi eros nec lobortis taciti, vehicula nisl litora tellus ligula porttitor metus. Vivamus integer non suscipit taciti mus etiam at primis tempor sagittis sit, euismod libero facilisi aptent elementum felis blandit cursus gravida sociis erat ante, eleifend lectus nullam dapibus netus feugiat curae curabitur est ad. Massa curae fringilla porttitor quam sollicitudin iaculis aptent leo ligula euismod dictumst, orci penatibus mauris eros etiam praesent erat volutpat posuere hac. Metus fringilla nec ullamcorper odio aliquam lacinia conubia mauris tempor, etiam ultricies proin quisque lectus sociis id tristique, integer phasellus taciti pretium adipiscing tortor sagittis ligula. Mollis pretium lorem primis senectus habitasse lectus scelerisque donec, ultricies tortor suspendisse adipiscing fusce morbi volutpat pellentesque, consectetur mi risus molestie curae malesuada cum. Dignissim lacus convallis massa mauris enim ad mattis magnis senectus montes, mollis taciti phasellus accumsan bibendum semper blandit suspendisse faucibus nibh est, metus lobortis morbi cras magna vivamus per risus fermentum. Dapibus imperdiet praesent magnis ridiculus congue gravida curabitur dictum sagittis, enim et magna sit inceptos sodales parturient pharetra mollis, aenean vel nostra tellus commodo pretium sapien sociosqu."
+
static int destructor_called;
@@ -220,7 +222,8 @@ void test_exec_child_only_extra_args_neg(void **state)
struct tevent_req *echo_child_write_send(TALLOC_CTX *mem_ctx,
struct child_test_ctx *child_tctx,
struct child_io_fds *io_fds,
- const char *input);
+ const char *input,
+ bool safe);
static void echo_child_write_done(struct tevent_req *subreq);
static void echo_child_read_done(struct tevent_req *subreq);
@@ -317,9 +320,11 @@ void test_child_cb(int child_status,
child_ctx->test_ctx->done = true;
}
-/* Test that writing to the pipes works as expected */
-void test_exec_child_echo(void **state)
+void test_exec_child_echo(void **state,
+ const char *msg,
+ bool safe)
{
+
errno_t ret;
pid_t child_pid;
struct child_test_ctx *child_tctx = talloc_get_type(*state,
@@ -359,7 +364,7 @@ void test_exec_child_echo(void **state)
NULL, NULL, NULL);
assert_int_equal(ret, EOK);
- req = echo_child_write_send(child_tctx, child_tctx, io_fds, ECHO_STR);
+ req = echo_child_write_send(child_tctx, child_tctx, io_fds, msg, safe);
assert_non_null(req);
ret = test_ev_loop(child_tctx->test_ctx);
@@ -367,16 +372,41 @@ void test_exec_child_echo(void **state)
assert_int_equal(ret, EOK);
}
+/* Test that writing a small message to the pipes works as expected */
+void test_exec_child_echo_small(void **state)
+{
+ test_exec_child_echo(state, ECHO_STR, false);
+}
+
+void test_exec_child_echo_small_safe(void **state)
+{
+ test_exec_child_echo(state, ECHO_STR, true);
+}
+
+/* Test that writing a large message to the pipes works as expected,
+ * test will still fail if message exceeds IN_BUF_SIZE */
+void test_exec_child_echo_large(void **state)
+{
+ test_exec_child_echo(state, ECHO_LARGE_STR, false);
+}
+
+void test_exec_child_echo_large_safe(void **state)
+{
+ test_exec_child_echo(state, ECHO_LARGE_STR, true);
+}
+
struct test_exec_echo_state {
struct child_io_fds *io_fds;
struct io_buffer buf;
struct child_test_ctx *child_test_ctx;
+ bool safe;
};
struct tevent_req *echo_child_write_send(TALLOC_CTX *mem_ctx,
struct child_test_ctx *child_tctx,
struct child_io_fds *io_fds,
- const char *input)
+ const char *input,
+ bool safe)
{
struct tevent_req *req;
struct tevent_req *subreq;
@@ -391,11 +421,18 @@ struct tevent_req *echo_child_write_send(TALLOC_CTX *mem_ctx,
assert_non_null(echo_state->buf.data);
echo_state->buf.size = strlen(input) + 1;
echo_state->io_fds = io_fds;
+ echo_state->safe = safe;
DEBUG(SSSDBG_TRACE_INTERNAL, "Writing..\n");
- subreq = write_pipe_send(child_tctx, child_tctx->test_ctx->ev,
- echo_state->buf.data, echo_state->buf.size,
- echo_state->io_fds->write_to_child_fd);
+ if (echo_state->safe) {
+ subreq = write_pipe_safe_send(child_tctx, child_tctx->test_ctx->ev,
+ echo_state->buf.data, echo_state->buf.size,
+ echo_state->io_fds->write_to_child_fd);
+ } else {
+ subreq = write_pipe_send(child_tctx, child_tctx->test_ctx->ev,
+ echo_state->buf.data, echo_state->buf.size,
+ echo_state->io_fds->write_to_child_fd);
+ }
assert_non_null(subreq);
tevent_req_set_callback(subreq, echo_child_write_done, req);
@@ -411,7 +448,12 @@ static void echo_child_write_done(struct tevent_req *subreq)
req = tevent_req_callback_data(subreq, struct tevent_req);
echo_state = tevent_req_data(req, struct test_exec_echo_state);
- ret = write_pipe_recv(subreq);
+ if (echo_state->safe) {
+ ret = write_pipe_safe_recv(subreq);
+ } else {
+ ret = write_pipe_recv(subreq);
+ }
+
DEBUG(SSSDBG_TRACE_INTERNAL, "Writing OK\n");
talloc_zfree(subreq);
assert_int_equal(ret, EOK);
@@ -420,9 +462,16 @@ static void echo_child_write_done(struct tevent_req *subreq)
echo_state->io_fds->write_to_child_fd = -1;
DEBUG(SSSDBG_TRACE_INTERNAL, "Reading..\n");
- subreq = read_pipe_send(echo_state,
- echo_state->child_test_ctx->test_ctx->ev,
- echo_state->io_fds->read_from_child_fd);
+ if (echo_state->safe) {
+ subreq = read_pipe_safe_send(echo_state,
+ echo_state->child_test_ctx->test_ctx->ev,
+ echo_state->io_fds->read_from_child_fd);
+ } else {
+ subreq = read_pipe_send(echo_state,
+ echo_state->child_test_ctx->test_ctx->ev,
+ echo_state->io_fds->read_from_child_fd);
+ }
+
assert_non_null(subreq);
tevent_req_set_callback(subreq, echo_child_read_done, req);
}
@@ -438,7 +487,12 @@ static void echo_child_read_done(struct tevent_req *subreq)
req = tevent_req_callback_data(subreq, struct tevent_req);
echo_state = tevent_req_data(req, struct test_exec_echo_state);
- ret = read_pipe_recv(subreq, echo_state, &buf, &len);
+ if (echo_state->safe) {
+ ret = read_pipe_safe_recv(subreq, echo_state, &buf, &len);
+ } else {
+ ret = read_pipe_recv(subreq, echo_state, &buf, &len);
+ }
+
talloc_zfree(subreq);
DEBUG(SSSDBG_TRACE_INTERNAL, "Reading OK\n");
assert_int_equal(ret, EOK);
@@ -524,7 +578,16 @@ int main(int argc, const char *argv[])
cmocka_unit_test_setup_teardown(test_exec_child_handler,
child_test_setup,
child_test_teardown),
- cmocka_unit_test_setup_teardown(test_exec_child_echo,
+ cmocka_unit_test_setup_teardown(test_exec_child_echo_small,
+ child_test_setup,
+ child_test_teardown),
+ cmocka_unit_test_setup_teardown(test_exec_child_echo_large,
+ child_test_setup,
+ child_test_teardown),
+ cmocka_unit_test_setup_teardown(test_exec_child_echo_small_safe,
+ child_test_setup,
+ child_test_teardown),
+ cmocka_unit_test_setup_teardown(test_exec_child_echo_large_safe,
child_test_setup,
child_test_teardown),
cmocka_unit_test_setup_teardown(test_sss_child,
From b1e5beeafe8145a73094ed2b1752f3d719dd3752 Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstephen@redhat.com>
Date: Fri, 22 Sep 2023 10:42:38 -0400
Subject: [PATCH 2/2] util: Realloc buffer size for atomic safe read
Realloc and increase the buffer size when safe read returns more
than CHILD_MSG_CHUNK size bytes.
This handles multiple passkey mappings returned from the krb5 child
in kerberos pre-authentication.
---
src/util/atomic_io.c | 6 +++++-
src/util/child_common.c | 18 ++++++++++++++++--
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/src/util/atomic_io.c b/src/util/atomic_io.c
index 589dfa4d4e..7cc4304d18 100644
--- a/src/util/atomic_io.c
+++ b/src/util/atomic_io.c
@@ -87,7 +87,11 @@ ssize_t sss_atomic_read_safe_s(int fd, void *buf, size_t buf_len, size_t *_len)
}
if (ulen > buf_len) {
- return ERANGE;
+ if (_len != NULL) {
+ *_len = ulen;
+ }
+ errno = ERANGE;
+ return -1;
}
if (_len != NULL) {
diff --git a/src/util/child_common.c b/src/util/child_common.c
index 6f950614f0..1dbf95b7a7 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -509,7 +509,7 @@ static void _read_pipe_handler(struct tevent_context *ev,
struct _read_pipe_state *state;
ssize_t size;
errno_t err;
- uint8_t buf[CHILD_MSG_CHUNK];
+ uint8_t *buf;
size_t len = 0;
state = tevent_req_data(req, struct _read_pipe_state);
@@ -521,8 +521,23 @@ static void _read_pipe_handler(struct tevent_context *ev,
return;
}
+ buf = talloc_array(state, uint8_t, CHILD_MSG_CHUNK);
+ if (buf == NULL) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
if (state->safe) {
size = sss_atomic_read_safe_s(state->fd, buf, CHILD_MSG_CHUNK, &len);
+ if (size == -1 && errno == ERANGE) {
+ buf = talloc_realloc(state, buf, uint8_t, len);
+ if(!buf) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
+ size = sss_atomic_read_s(state->fd, buf, len);
+ }
} else {
size = sss_atomic_read_s(state->fd, buf, CHILD_MSG_CHUNK);
}
@@ -532,7 +547,6 @@ static void _read_pipe_handler(struct tevent_context *ev,
"read failed [%d][%s].\n", err, strerror(err));
tevent_req_error(req, err);
return;
-
} else if (size > 0) {
state->buf = talloc_realloc(state, state->buf, uint8_t,
state->len + size);

28
0003-increase-conv-message-size.patch Normal file
View File

@ -0,0 +1,28 @@
From 3aab4ad34b8a9b5b87f80a5a11170c4fd79e49dd Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstephen@redhat.com>
Date: Tue, 26 Sep 2023 16:24:16 -0400
Subject: [PATCH] Passkey: Increase conv message size for prompting
Size needs to handle the prompts for interactive, touch, pin prompt, and
kerberos pre-auth warning message which could all be displayed.
---
src/sss_client/pam_sss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index 2cc43c3ff7..a1c3536048 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -1849,8 +1849,8 @@ static int prompt_passkey(pam_handle_t *pamh, struct pam_items *pi,
{
int ret;
const struct pam_conv *conv;
- const struct pam_message *mesg[3] = { NULL, NULL, NULL };
- struct pam_message m[3] = { {0}, {0}, {0} };
+ const struct pam_message *mesg[4] = { NULL, NULL, NULL, NULL };
+ struct pam_message m[4] = { {0}, {0}, {0}, {0} };
struct pam_response *resp = NULL;
bool kerberos_preauth;
bool prompt_pin;

8
sssd.spec
View File

@ -43,13 +43,16 @@
Name: sssd
Version: 2.9.2
Release: 1%{?dist}
Release: 2%{?dist}
Summary: System Security Services Daemon
License: GPL-3.0-or-later
URL: https://github.com/SSSD/sssd/
Source0: https://github.com/SSSD/sssd/releases/download/2.9.2/sssd-2.9.2.tar.gz
### Patches ###
Patch0001: 0001-passkey-kerberos-preauth-false-uv.patch
Patch0002: 0002-increase-child-buffer-size.patch
Patch0003: 0003-increase-conv-message-size.patch
### Dependencies ###
@ -1058,6 +1061,9 @@ fi
%systemd_postun_with_restart sssd.service
%changelog
* Tue Oct 03 2023 Iker Pedrosa <ipedrosa@redhat.com> - 2.9.2-2
- Fixes for passkey authentication
* Thu Sep 07 2023 Pavel Březina <pbrezina@redhat.com> - 2.9.2-1
- Rebase to SSSD 2.9.2