On Fri, Jul 29, 2016 at 02:59:46PM +0200, Lukas Slebodnik wrote:
> On (29/07/16 13:01), Jakub Hrozek wrote:
> >On Fri, Jul 29, 2016 at 11:44:53AM +0200, 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 <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.
> >
> >If you prefer, I can return an error code only from failures parsing the
> >deny list, but according to my testing without this patch, a user was
> >allowed access if an unparseable name was in the deny list. Try to
> >remove this hunk and run the tests from the last patch..
> >
> I tested only with the 1st patch and all simple access test for AD passed.
> But it's true that we previously failed in initialisation of access provider
> and we had resolved all domains. So refresh of filter_* could not cause
> a problem.
>
Yes, so what should we do with this patch? Keep it as is or change to
only error out on typos in deny?