On 04/28/2016 12:23 PM, Sumit Bose wrote:
On Thu, Apr 28, 2016 at 11:26:19AM +0200, Lukas Slebodnik wrote:
On (19/04/16 11:41), Michal Židek wrote:
On 04/13/2016 04:59 PM, Michal Židek wrote:
...
From e41fb64fb2a0f326095e9712c62b1b9f9ce4782e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 16:46:24 +0200 Subject: [PATCH] GPO: ignore non-KVP lines if possible
Ticket: https://fedorahosted.org/sssd/ticket/2751
Non-KVP break GPO processing. They are used for values we are not interested in so it is safe to ignore them.
src/providers/ad/ad_gpo.c | 3 ++- src/providers/ad/ad_gpo_child.c | 3 ++- src/util/util.h | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 3bd9ab0..859c5df 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -1136,7 +1136,8 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain, goto done; }
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0,
INI_PARSE_IGNORE_NON_KVP, ini_config);It will fix the bug with #2751. But we will ignore parsing errors in all sections. and I worry about security bugs due to deny rules.
We actually do have a security issue now. If we fail to parse the GPO because of the non key-value pairs (which are not incorrect GPO syntax) then no rules from the GPO are used. These can be any rules. In this case we write a critical error in the logs, but we do not deny access. Even if we did the opposite and denied all access, we would also have an issue that would prevent users from logging into their machines.
IIRC Sumit proposed to ignore such failures only in special sections or to parse only special section for GPO. It does not meand that ding-libs patches are not useful.
yes, but if I understand it correctly it is currently not easy to only parse a single section.
I share your concern about potential issues. But at least for the deny rules case I think the risk is acceptable because the deny options are proper =-separated key-value pairs according to the MSFT docs so it should be always possible to parse them. If the = is missing there is either an issue in AD when generating the files or someone was able to modify the file on the AD side or in the gpo cache. The first has imo a quite low probability and in the second case we have a much larger issue and the attacker would have been able to modify allow rules as well.
Maybe it would help to first call ini_config_parse() without INI_PARSE_IGNORE_NON_KVP and in the case to an error call it with INI_PARSE_IGNORE_NON_KVP? If parsing then succeeds we can still log a warning that there were issues while parsing the GPO and a concerned user should check if the "Privilege Rights" section only contains proper entries
This sounds like a good compromise.
Nevertheless I think it is still worth to work on a ini_config_parse_single_section() call which we can use for gpo processing without setting INI_PARSE_IGNORE_NON_KVP.
Ok, I can revive the ticket after 7.3 when there is more time. Until then, I would prefer to use the solution with INI_PARSE_IGNORE_NON_KVP flag.
bye, Sumit
Other opinions are welcomed.
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c index e37abe3..fcb8dca 100644 --- a/src/providers/ad/ad_gpo_child.c +++ b/src/providers/ad/ad_gpo_child.c @@ -469,7 +469,8 @@ ad_gpo_parse_ini_file(const char *smb_path, goto done; }
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0,
INI_PARSE_IGNORE_NON_KVP, ini_config);diff --git a/src/util/util.h b/src/util/util.h index 05ee875..f070a84 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -96,6 +96,11 @@ #endif #endif
+/* If INI_PARSE_IGNORE_NON_KVP is not defined, use 0 (no effect) */ +#ifndef INI_PARSE_IGNORE_NON_KVP +#define INI_PARSE_IGNORE_NON_KVP 0 +#endif
If we decide to use approach with INI_PARSE_IGNORE_NON_KVP Then I would rather fail if we cannot detect INI_PARSE_IGNORE_NON_KVP. or at least print warning. (the same as with robust mutexes)
I would prefer not to fail. Warning sounds good to me.
$git grep -n "#warning" src/sss_client/common.c:1101:#warning Robust mutexes are not supported on this platform. src/sss_client/common.c:1113:#warning Robust mutexes are not supported on this platform.
Or rather mlso move this to gpo code.
I do not understand how this would help in the GPO code.
We needn't print a warning if sssd is built without samba.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org