From 916d13bc230f5edffd8f4fb9da0a30fcde3eb573 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: May 08 2020 13:43:53 +0000 Subject: Issue 51054 - Revise ACI target syntax checking Bug Description: The previous commit enforced a strict syntax that was previously allowed. This is causing regressions for customers and community members. Fix Description: Reject ACI's that use more than one equal sign between the target keyword and the value, but do not enforce that the values are quoted. A flag was added that we can turn on strict syntax at a later date, but for now we will continue allow values without quotes. relates: https://pagure.io/389-ds-base/issue/51054 Reviewed by: firstyear & spichugi(Thanks!!) --- diff --git a/dirsrvtests/tests/suites/acl/syntax_test.py b/dirsrvtests/tests/suites/acl/syntax_test.py index cf08c65..c143ff7 100644 --- a/dirsrvtests/tests/suites/acl/syntax_test.py +++ b/dirsrvtests/tests/suites/acl/syntax_test.py @@ -192,7 +192,7 @@ FAILED = [('test_targattrfilters_18', f'(all)userdn="ldap:///anyone";)'), ] -@pytest.mark.skipif(ds_is_older('1.4.3'), reason="Not implemented") +@pytest.mark.xfail(reason='https://bugzilla.redhat.com/show_bug.cgi?id=1691473') @pytest.mark.parametrize("real_value", [a[1] for a in FAILED], ids=[a[0] for a in FAILED]) def test_aci_invalid_syntax_fail(topo, real_value): diff --git a/ldap/servers/plugins/acl/aclparse.c b/ldap/servers/plugins/acl/aclparse.c index d5edaf5..577251e 100644 --- a/ldap/servers/plugins/acl/aclparse.c +++ b/ldap/servers/plugins/acl/aclparse.c @@ -34,6 +34,10 @@ static int acl_verify_exactly_one_attribute(char *attr_name, Slapi_Filter *f); static int type_compare(Slapi_Filter *f, void *arg); static int acl_check_for_target_macro(aci_t *aci_item, char *value); static int get_acl_rights_as_int(char *strValue); + +/* Enforce strict aci syntax */ +#define STRICT_SYNTAX_CHECK 0 + /*************************************************************************** * * acl_parse @@ -306,11 +310,20 @@ __aclp__parse_aci(char *str, aci_t *aci_item, char **errbuf) if (NULL == tmpstr) { return ACL_SYNTAX_ERR; } + tmpstr++; + /* Consecutive equals are not allowed */ + if (*tmpstr == '=') { + slapi_log_err(SLAPI_LOG_ERR, plugin_name, + "__aclp__parse_aci - target filter has an invalid syntax, " + "do not use more than one '=' between the targetfilter keyword and its value: (%s)\n", + str); + return ACL_SYNTAX_ERR; + } __acl_strip_leading_space(&tmpstr); /* The first character is expected to be a double quote */ - if (*tmpstr != '"') { + if (STRICT_SYNTAX_CHECK && *tmpstr != '"') { slapi_log_err(SLAPI_LOG_ERR, plugin_name, "__aclp__parse_aci - target filter has an invalid value (%s)\n", str); return ACL_SYNTAX_ERR; @@ -355,11 +368,20 @@ __aclp__parse_aci(char *str, aci_t *aci_item, char **errbuf) strncpy(s, single_space, 1); } if ((s = strchr(str, '=')) != NULL) { - value = s + 1; + s++; + if (*s == '=') { + /* Consecutive equals are not allowed */ + slapi_log_err(SLAPI_LOG_ERR, plugin_name, + "__aclp__parse_aci - target to/from has an invalid syntax, " + "do not use more than one '=' between the target to/from keyword and its value: (%s)\n", + str); + return ACL_SYNTAX_ERR; + } + value = s; __acl_strip_leading_space(&value); __acl_strip_trailing_space(value); /* The first character is expected to be a double quote */ - if (*value != '"') { + if (STRICT_SYNTAX_CHECK && *value != '"') { slapi_log_err(SLAPI_LOG_ERR, plugin_name, "__aclp__parse_aci - target to/from has an invalid value (%s)\n", str); return ACL_SYNTAX_ERR; @@ -414,11 +436,20 @@ __aclp__parse_aci(char *str, aci_t *aci_item, char **errbuf) strncpy(s, single_space, 1); } if ((s = strchr(str, '=')) != NULL) { - value = s + 1; + s++; + if (*s == '=') { + /* Consecutive equals are not allowed */ + slapi_log_err(SLAPI_LOG_ERR, plugin_name, + "__aclp__parse_aci - target has an invalid syntax, " + "do not use more than one '=' between the target keyword and its value: (%s)\n", + str); + return ACL_SYNTAX_ERR; + } + value = s; __acl_strip_leading_space(&value); __acl_strip_trailing_space(value); /* The first character is expected to be a double quote */ - if (*value != '"') { + if (STRICT_SYNTAX_CHECK && *value != '"') { slapi_log_err(SLAPI_LOG_ERR, plugin_name, "__aclp__parse_aci - target has an invalid value (%s)\n", str); return ACL_SYNTAX_ERR; @@ -1520,14 +1551,22 @@ __aclp__init_targetattr(aci_t *aci, char *attr_val, char **errbuf) return ACL_SYNTAX_ERR; } s++; + if (*s == '=') { + /* Consecutive equals are not allowed */ + slapi_log_err(SLAPI_LOG_ERR, plugin_name, + "__aclp__init_targetattr - targetattr has an invalid syntax, " + "do not use more than one '=' between the targetattr and its value: (%s)\n", + attr_val); + return ACL_SYNTAX_ERR; + } __acl_strip_leading_space(&s); __acl_strip_trailing_space(s); len = strlen(s); - /* Simple targetattr statements may not be quoted e.g. - targetattr=* or targetattr=userPassword - if it begins with a quote, it must end with one as well - */ + /* + * If it begins with a quote, it must end with one as well + */ if (*s == '"') { + if (s[len - 1] == '"') { s[len - 1] = '\0'; /* trim trailing quote */ } else { @@ -1545,7 +1584,7 @@ __aclp__init_targetattr(aci_t *aci, char *attr_val, char **errbuf) return ACL_SYNTAX_ERR; } s++; /* skip leading quote */ - } else { + } else if (STRICT_SYNTAX_CHECK) { /* The first character is expected to be a double quote */ slapi_log_err(SLAPI_LOG_ERR, plugin_name, "__aclp__init_targetattr - targetattr has an invalid value (%s)\n", attr_val);