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.
> >
> > 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.
> > 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.
LS