On (26/07/16 15:00), Jakub Hrozek wrote:
>Hi,
>
>please see the attached patches. I'm not sure how this bug got in,
>because in the patch that broke the functionality
>(eef359b508b898ae99d2bf292a43f0f295a2ba5e) I said in the commit message
>that I did the change that is only implemented in the first attached
>patch. My guess is that the rebasing after the DP patches were merged
>went wrong.
>
>To make sure we don't regress, I added more tests and switched the tests
>to calling the DP handler.
>From 9a60d3ed8bd2b0eeb51dff2c6f78771e0d29245e Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Thu, 21 Jul 2016 12:18:01 +0200
>Subject: [PATCH 1/4] SIMPLE: Do not parse names on startup
>
>It's not required to parse names on SSSD startup in the simple access
>provider. We can instead just parse the name when the access request is
>processed.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3101
>---
> src/providers/simple/simple_access.c | 7 -------
> 1 file changed, 7 deletions(-)
>
>diff --git a/src/providers/simple/simple_access.c
b/src/providers/simple/simple_access.c
>index
cb72ada20727c63452936647876ef297106e17b0..ae90215351fe7db834898067d3b4bad71015ec5f 100644
>--- a/src/providers/simple/simple_access.c
>+++ b/src/providers/simple/simple_access.c
>@@ -284,7 +284,6 @@ errno_t sssm_simple_access_init(TALLOC_CTX *mem_ctx,
> struct dp_method *dp_methods)
> {
> struct simple_ctx *ctx;
>- errno_t ret;
>
> ctx = talloc_zero(mem_ctx, struct simple_ctx);
> if (ctx == NULL) {
>@@ -296,12 +295,6 @@ errno_t sssm_simple_access_init(TALLOC_CTX *mem_ctx,
> ctx->be_ctx = be_ctx;
> ctx->last_refresh_of_filter_lists = 0;
>
>- ret = simple_access_obtain_filter_lists(ctx);
>- if (ret != EOK) {
>- talloc_free(ctx);
>- return ret;
>- }
>-
> dp_set_method(dp_methods, DPM_ACCESS_HANDLER,
> simple_access_handler_send, simple_access_handler_recv, ctx,
> struct simple_ctx, struct pam_data, struct pam_data *);
>--
>2.4.11
>
ACK
>From 5aeeedbb85e068ff1241868cf91596817540b009 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Thu, 21 Jul 2016 13:33:18 +0200
>Subject: [PATCH 2/4] SIMPLE: Fail on any error parsing the access control list
>
>Luckily this error was hidden by the fact that SSSD didn't start at all
>when an unparseable name was encountered after startup. Otherwise, this
>would have been a security issue.
>
>Nonetheless, we should just fail and deny access if we can't parse a
>name in a simple access list.
>---
> src/providers/simple/simple_access.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/src/providers/simple/simple_access.c
b/src/providers/simple/simple_access.c
>index
ae90215351fe7db834898067d3b4bad71015ec5f..577e8354e9b574764734248b2bde4ef06c6fb4fc 100644
>--- a/src/providers/simple/simple_access.c
>+++ b/src/providers/simple/simple_access.c
>@@ -211,7 +211,10 @@ simple_access_handler_send(TALLOC_CTX *mem_ctx,
>
> ret = simple_access_obtain_filter_lists(simple_ctx);
> if (ret != EOK) {
>- DEBUG(SSSDBG_MINOR_FAILURE, "Failed to refresh filter
lists\n");
>+ DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Failed to refresh filter lists, denying all access\n");
>+ pd->pam_status = PAM_PERM_DENIED;
>+ goto immediately;
> }
I didn't test but Do we really need it.
I think that unparsable names are covered by #2519
@see
https://git.fedorahosted.org/cgit/sssd.git/commit/?id=79f128801d598ca57a6...
IIRC the intention of #2519 was to be strict only for deny rules.
There might be typos in allow rules because it isn't a security bug.
I checked the test case for #2519 and it passed.
The bug #3101 is fixed as well.