Hello!
See patches in attachment that are part of solution for https://fedorahosted.org/sssd/ticket/2751
There are few ding libs patches. The first is unrelated, but I just added some missing trace info.
The second is a patch for new parser flag that Dmitri wrote in discussion for ticket https://fedorahosted.org/sssd/ticket/2986
I modified the patch a little so I can not ack it (the check was necessary on one more place, so I added it there).
The third ding libs patch ads unit test for the new flag. I decided to add a new check based test rather than adding the test to ini_parse_ut.c. So it is the only test in the suite for now.
There is also a patch for SSSD attached that can take advantage of this flag to solve GPO issues once the new version of ding libs is released.
Michal
On 04/13/2016 04:59 PM, Michal Židek wrote:
Hello!
See patches in attachment that are part of solution for https://fedorahosted.org/sssd/ticket/2751
There are few ding libs patches. The first is unrelated, but I just added some missing trace info.
The second is a patch for new parser flag that Dmitri wrote in discussion for ticket https://fedorahosted.org/sssd/ticket/2986
I modified the patch a little so I can not ack it (the check was necessary on one more place, so I added it there).
The third ding libs patch ads unit test for the new flag. I decided to add a new check based test rather than adding the test to ini_parse_ut.c. So it is the only test in the suite for now.
There is also a patch for SSSD attached that can take advantage of this flag to solve GPO issues once the new version of ding libs is released.
Michal
I forgot to put ticket link to description of the ding-libs patches. New patches attached. Codewise the patches are the same.
Michal
On (19/04/16 11:41), Michal Židek wrote:
On 04/13/2016 04:59 PM, Michal Židek wrote:
Hello!
See patches in attachment that are part of solution for https://fedorahosted.org/sssd/ticket/2751
There are few ding libs patches. The first is unrelated, but I just added some missing trace info.
The second is a patch for new parser flag that Dmitri wrote in discussion for ticket https://fedorahosted.org/sssd/ticket/2986
I modified the patch a little so I can not ack it (the check was necessary on one more place, so I added it there).
The third ding libs patch ads unit test for the new flag. I decided to add a new check based test rather than adding the test to ini_parse_ut.c. So it is the only test in the suite for now.
There is also a patch for SSSD attached that can take advantage of this flag to solve GPO issues once the new version of ding libs is released.
Michal
I forgot to put ticket link to description of the ding-libs patches. New patches attached. Codewise the patches are the same.
Michal
From 5d97907f4aba09838d33e31ab85f7bd2adf28024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 11:10:22 +0200 Subject: [PATCH 1/3] ini_parse: Add missing TRACE_FLOW_EXIT
Some TRACE_FLOW_EXIT macros were missing in handle_kvp() before return.
ini/ini_parse.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ini/ini_parse.c b/ini/ini_parse.c index 91525a0..0de4e35 100644 --- a/ini/ini_parse.c +++ b/ini/ini_parse.c
LGTM
From 235156822079a6c68c7fa8e2ff13a1fa5803cfa7 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Wed, 13 Apr 2016 11:44:44 +0200 Subject: [PATCH 2/3] ini: Add INI_PARSE_IGNORE_NON_KVP flag
Ticket: https://fedorahosted.org/sssd/ticket/2751
Add INI_PARSE_IGNORE_NON_KVP flag to allow skipping lines that are not key-value pairs.
ini/ini_configobj.h | 2 ++ ini/ini_parse.c | 29 ++++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h index 6f2d692..12920db 100644 --- a/ini/ini_configobj.h +++ b/ini/ini_configobj.h @@ -363,6 +363,8 @@ enum ERR_PARSE { #define INI_PARSE_NOTAB 0x0004 /** @brief Do not allow C-style comments */ #define INI_PARSE_NO_C_COMMENTS 0x0008 +/** @brief Skip lines that are not KVPs */ +#define INI_PARSE_IGNORE_NON_KVP 0x0010
/**
- @}
diff --git a/ini/ini_parse.c b/ini/ini_parse.c index 0de4e35..e5baeca 100644 --- a/ini/ini_parse.c +++ b/ini/ini_parse.c @@ -966,8 +966,18 @@ static int handle_kvp(struct parser_obj *po, uint32_t *action) /* Check if we have the key */ if (*(str) == '=') { TRACE_ERROR_STRING("No key", str);
po->last_error = ERR_NOKEY;*action = PARSE_ERROR;
if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {/* Clean everything as if nothing happened */free(po->last_read);po->last_read = NULL;po->last_read_len = 0;*action = PARSE_READ;} else {po->last_error = ERR_NOKEY;*action = PARSE_ERROR;}TRACE_FLOW_EXIT(); return EOK;@@ -975,9 +985,18 @@ static int handle_kvp(struct parser_obj *po, uint32_t *action) /* Find "=" */ eq = strchr(str, '='); if (eq == NULL) {
TRACE_ERROR_STRING("No equal sign", str);po->last_error = ERR_NOEQUAL;*action = PARSE_ERROR;
if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {/* Clean everything as if nothing happened */free(po->last_read);po->last_read = NULL;po->last_read_len = 0;*action = PARSE_READ;} else {TRACE_ERROR_STRING("No equal sign", str);po->last_error = ERR_NOEQUAL;*action = PARSE_ERROR;}TRACE_FLOW_EXIT(); return EOK;-- 2.5.0
From 0d4ae5f1a4b062b28305226e44df1c3d1886ac17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 15:57:34 +0200 Subject: [PATCH 3/3] Add unit test for INI_PARSE_IGNORE_NON_KVP
Ticket: https://fedorahosted.org/sssd/ticket/2751
Makefile.am | 10 +++- ini/ini_parse_ut_check.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 ini/ini_parse_ut_check.c
diff --git a/Makefile.am b/Makefile.am index 5b1da33..c162580 100644 --- a/Makefile.am +++ b/Makefile.am @@ -69,8 +69,8 @@ libpath_utils_la_LDFLAGS = \ -Wl,--version-script=$(top_srcdir)/path_utils/libpath_utils.sym
if HAVE_CHECK
- check_PROGRAMS += path_utils_ut ini_configmod_ut_check
- TESTS += path_utils_ut ini_configmod_ut_check
- check_PROGRAMS += path_utils_ut ini_configmod_ut_check ini_parse_ut_check
- TESTS += path_utils_ut ini_configmod_ut_check ini_parse_ut_check
It might be a good idea to use the same approach as in sssd one test paer line + $(NULL) at the end of list
BTW thank you for unit test.
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.
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.
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)
$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. We needn't print a warning if sssd is built without samba.
LS
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.
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
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.
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)
$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. 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
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
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@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.
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_alpha...
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);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
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@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_alpha...
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@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On (28/04/16 14:15), Michal Židek wrote:
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@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_alpha...
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
Yes, I am. (Mon Feb 29 15:48:10 2016) [sssd[be[example.com]]] [ad_gpo_access_done] (0x0040): Ignoring error: [5](Input/output error); GPO-based access control failed, but GPO is not in enforcing mode. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ If GPO is not in enforcing mode then we do not reject the access. The same as SELinux in permissive mode.
LS
On 04/28/2016 02:38 PM, Lukas Slebodnik wrote:
On (28/04/16 14:15), Michal Židek wrote:
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@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_alpha...
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
Yes, I am. (Mon Feb 29 15:48:10 2016) [sssd[be[example.com]]] [ad_gpo_access_done] (0x0040): Ignoring error: [5](Input/output error); GPO-based access control failed, but GPO is not in enforcing mode. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ If GPO is not in enforcing mode then we do not reject the access. The same as SELinux in permissive mode.
LS
Ok, thanks for the info.
New patches are attached.
SSSD patch changes: -I changed the behavior to only try parsing with the new flag when it previously failed as proposed by Sumit.
-Existence of the flag is now checked in the GPO code, not in util.h as Lukas suggested
-I do not use the flag in the GPO child anymore (we parse different file in the child).
Some nitpicks were also fixed in the last libini patch.
Michal
On (05/05/16 16:28), Michal Židek wrote:
New patches are attached.
SSSD patch changes: -I changed the behavior to only try parsing with the new flag when it previously failed as proposed by Sumit.
-Existence of the flag is now checked in the GPO code, not in util.h as Lukas suggested
-I do not use the flag in the GPO child anymore (we parse different file in the child).
Some nitpicks were also fixed in the last libini patch.
Michal
ACK to libini_patches
ACK to SSSD patch.
LS
On Wed, Jun 22, 2016 at 09:40:47AM +0200, Lukas Slebodnik wrote:
On (05/05/16 16:28), Michal Židek wrote:
New patches are attached.
SSSD patch changes: -I changed the behavior to only try parsing with the new flag when it previously failed as proposed by Sumit.
-Existence of the flag is now checked in the GPO code, not in util.h as Lukas suggested
-I do not use the flag in the GPO child anymore (we parse different file in the child).
Some nitpicks were also fixed in the last libini patch.
Michal
ACK to libini_patches
Pushed to ding-libs master as: * fbaaf491afd199e2c401037a448052494d0f5b40 * 9591b1d8adbf195c40c123b1b5125db82e049a56 * 8481bb7eebe761ceaedadb73d84045d86723d156
ACK to SSSD patch.
Pushed to sssd master as: * 21a28c92d56a0fa3b62a951eb64bf0c48034fe5e
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Sumit Bose