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(a)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);
>> if (ret != 0) {
>> int lret;
>> char **errors;
>
> 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);
>> if (ret != 0) {
>> int lret;
>> char **errors;
>> 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(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org