On (29/07/16 11:44), Lukas Slebodnik wrote: >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 <[email protected]> >>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 <[email protected]> >>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=79f128801d598ca57a6acebade01136525a47e00 > >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.
http://sssd-ci.duckdns.org/logs/job/51/33/summary.html ACK LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
