On 04/28/2016 01:38 PM, Lukas Slebodnik wrote:
On (28/04/16 13:06), Michal Židek wrote:
> 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.
That's not true.
We deny access even though it should be allowed.
Because we return PAM_SYSTEM_ERROR
https://fedorahosted.org/sssd/attachment/ticket/2751/GPO_failed_1.13.1_al...
> 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.
That's just a bug and not security issue.
And it's current state.
Are you sure?
https://bugzilla.redhat.com/show_bug.cgi?id=1316164
>
>>>
>>> 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.
>>
I thing that there should not be a problem if GPO is created with
GUI in AD.
But users can import GPO from command line (powershell)
Set-Location 'c:\scripts' ;
Import-Module 'GroupPolicy' ;
Import-GPO -Domain \
...
-MigrationTable 'c:\scripts\migtable.xml'
It might be good idea to check whether AD validate such input.
If yes, than we can safely use INI_PARSE_IGNORE_NON_KVP.
+ file bugs to samba-dc if behaves differently :-)
>> 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.
>
Please at least file a ticket
and maybe there will be a time to implement it.
Feel free to reopen
https://fedorahosted.org/sssd/ticket/2986
>>> 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.
>
The idea was to not have conditional definition in util.h.
I realized that we will have a strict requirement for
new ding-libs due to validation feature.
So this change is just a temporary and we might remove it later.
That makes sense, I will do it only in the GPO code.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org