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. and commit says not existing user/group in simple_allow_users/simple_allow_groups should not imply access denied. >From 5bd29f76a8aee228b3ce07e2ec6453f7374a4acc Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek <[email protected]> >Date: Tue, 26 Jul 2016 12:13:43 +0200 >Subject: [PATCH 3/4] SIMPLE: Make the DP handlers testable > >To make it possible to call the whole DP handler in the unit test, not >just the evaluator part. >--- > Makefile.am | 1 + > src/providers/simple/simple_access.c | 5 ++-- > src/providers/simple/simple_access_pvt.h | 43 ++++++++++++++++++++++++++++++++ > 3 files changed, 47 insertions(+), 2 deletions(-) > create mode 100644 src/providers/simple/simple_access_pvt.h > >diff --git a/Makefile.am b/Makefile.am >index >3d84bd868a8d0b8f9df329aa8978c215ba750a45..5d1d671096f986d9387e6199112c017e9bf30e1b > 100644 >--- a/Makefile.am >+++ b/Makefile.am >@@ -665,6 +665,7 @@ dist_noinst_HEADERS = \ > src/providers/fail_over_srv.h \ > src/util/child_common.h \ > src/providers/simple/simple_access.h \ >+ src/providers/simple/simple_access_pvt.h \ > src/providers/krb5/krb5_auth.h \ > src/providers/krb5/krb5_common.h \ > src/providers/krb5/krb5_utils.h \ >diff --git a/src/providers/simple/simple_access.c >b/src/providers/simple/simple_access.c >index >577e8354e9b574764734248b2bde4ef06c6fb4fc..521beee84833b47b547bd1045c24e3384aa4d9a5 > 100644 >--- a/src/providers/simple/simple_access.c >+++ b/src/providers/simple/simple_access.c >@@ -22,6 +22,7 @@ > #include <security/pam_modules.h> > > #include "providers/simple/simple_access.h" >+#include "providers/simple/simple_access_pvt.h" > #include "util/sss_utf8.h" > #include "providers/backend.h" > #include "db/sysdb.h" >@@ -176,7 +177,7 @@ struct simple_access_handler_state { > > static void simple_access_handler_done(struct tevent_req *subreq); > >-static struct tevent_req * >+struct tevent_req * > simple_access_handler_send(TALLOC_CTX *mem_ctx, > struct simple_ctx *simple_ctx, > struct pam_data *pd, >@@ -265,7 +266,7 @@ done: > tevent_req_done(req); > } > >-static errno_t >+errno_t > simple_access_handler_recv(TALLOC_CTX *mem_ctx, > struct tevent_req *req, > struct pam_data **_data) >diff --git a/src/providers/simple/simple_access_pvt.h >b/src/providers/simple/simple_access_pvt.h >new file mode 100644 >index >0000000000000000000000000000000000000000..c133e1c5531be35861178e0b23aa7a09db9f7703 >--- /dev/null >+++ b/src/providers/simple/simple_access_pvt.h >@@ -0,0 +1,43 @@ >+/* >+ SSSD >+ >+ Simple access control >+ >+ Copyright (C) Sumit Bose <[email protected]> 2010 >+ Looks like a copy&paste issue. Should we use Author + copyright to RH for new files? >+ 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 __SIMPLE_ACCESS_PVT_H__ >+#define __SIMPLE_ACCESS_PVT_H__ >+ >+#include "providers/data_provider/dp.h" >+ >+/* We only 'export' the functions in a private header file to be able to call >+ * them from unit tests >+ */ >+struct tevent_req * >+simple_access_handler_send(TALLOC_CTX *mem_ctx, >+ struct simple_ctx *simple_ctx, >+ struct pam_data *pd, >+ struct dp_req_params *params); >+ >+errno_t >+simple_access_handler_recv(TALLOC_CTX *mem_ctx, >+ struct tevent_req *req, >+ struct pam_data **_data); >+ >+int simple_access_obtain_filter_lists(struct simple_ctx *ctx); >+ >+#endif /* __SIMPLE_ACCESS_PVT_H__ */ >-- >2.4.11 > >From c01d768243e16fba9d1078b8a6b64b7d7ee1f43a Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek <[email protected]> >Date: Tue, 26 Jul 2016 12:14:47 +0200 >Subject: [PATCH 4/4] TESTS: Use the DP handlers in simple provider tests, add > more tests > >Use the full simple access control handlers, just like SSSD does in the >tests. >--- Thank you for unit test. But it might change if we drop 2nd patch _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
