389-ds-base

Clone
Source Code
GIT

#51055 Issue 51054 - AddressSanitizer: heap-buffer-overflow in ldap_utf8prev
Closed 4 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base issue51054  into  master

file modified
+2 -2 
@@ -10,10 +10,10 @@ 
 

  

  import os
 

  import pytest
 

- 

  from lib389._constants import DEFAULT_SUFFIX
 

  from lib389.idm.domain import Domain
 

  from lib389.topologies import topology_st as topo
 

+ from lib389.utils import ds_is_older
 

  

  import ldap
 

  
@@ -192,7 +192,7 @@ 
 

             f'(all)userdn="ldap:///anyone";)'), ]
 

  

  

- @pytest.mark.xfail(reason='https://bugzilla.redhat.com/show_bug.cgi?id=1691473')
 

+ @pytest.mark.skipif(ds_is_older('1.4.3'), reason="Not implemented")
 

  @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):

file modified
+29 -5 
@@ -309,12 +309,18 @@ 
 

              tmpstr++;
 

              __acl_strip_leading_space(&tmpstr);
 

  

+             /* The first character is expected to be a double quote */
 

+             if (*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;
 

+             }
 

+ 

              /*
 

               * Trim off enclosing quotes and enclosing
 

               * superfluous brackets.
 

               * The result has been duped so it can be kept.
 

-             */
 

- 

+              */
 

              tmpstr = __acl_trim_filterstr(tmpstr);
 

  

              f = slapi_str2filter(tmpstr);
 
@@ -323,9 +329,10 @@ 
 

              aci_item->targetFilterStr = tmpstr;
 

  

          } else if ((strncmp(str, aci_target_to, target_to_len) == 0) || (strncmp(str, aci_target_from, target_from_len) == 0)) {
 

-             /* This is important to make this test before aci_targetdn
 

-                          * because aci_targetdn also match aci_target_to/aci_target_from
 

-                          *  */
 

+             /*
 

+              * This is important to make this test before aci_targetdn
 

+              * because aci_targetdn also match aci_target_to/aci_target_from
 

+              */
 

              char *tstr = NULL;
 

              size_t LDAP_URL_prefix_len = 0;
 

              size_t tmplen = 0;
 
@@ -351,6 +358,12 @@ 
 

                  value = s + 1;
 

                  __acl_strip_leading_space(&value);
 

                  __acl_strip_trailing_space(value);
 

+                 /* The first character is expected to be a double quote */
 

+                 if (*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;
 

+                 }
 

                  len = strlen(value);
 

                  /* strip double quotes */
 

                  if (*value == '"' && value[len - 1] == '"') {
 
@@ -404,6 +417,12 @@ 
 

                  value = s + 1;
 

                  __acl_strip_leading_space(&value);
 

                  __acl_strip_trailing_space(value);
 

+                 /* The first character is expected to be a double quote */
 

+                 if (*value != '"') {
 

+                     slapi_log_err(SLAPI_LOG_ERR, plugin_name,
 

+                             "__aclp__parse_aci - target has an invalid value (%s)\n", str);
 

+                     return ACL_SYNTAX_ERR;
 

+                 }
 

                  len = strlen(value);
 

                  /* strip double quotes */
 

                  if (*value == '"' && value[len - 1] == '"') {
 
@@ -1526,6 +1545,11 @@ 
 

              return ACL_SYNTAX_ERR;
 

          }
 

          s++; /* skip leading quote */
 

+     } else {
 

+         /* 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);
 

+         return ACL_SYNTAX_ERR;
 

      }
 

  

      str = s;

Bug Description:

Adding an invalid/double equal sign when setting the target/targetattr/targetfilter will cause a heap "underflow":

                    targetfilter=="(uid=*)"

Fix description:

Detect and reject these invalid ACI syntaxes before we "underflow". Simply check if the character after the first equal sign is a double quote, as that is the only possible next valid character in a valid ACI.

fixes: https://pagure.io/389-ds-base/issue/51054

What about targetfilter = "( uid )"? Shouldn't that be valid too?

What about targetfilter = "( uid )"? Shouldn't that be valid too?

That is technically invalid of course, and probably still goes undetected, but this heap underrun issue only occurs with "==", as that's how we separate the filter from the aci keyword. There are more details in the bugzilla, but for some reason all the comments are private (I think they thought this could of have been a security issue).

Ahhh okay, all good to me then.

rebased onto 13f8dc7
4 years ago

Pull-Request has been merged by mreynolds
4 years ago

Lot's of ACL tests are failing because now we're enforcing proper syntax: https://fedorapeople.org/groups/389ds/ci/nightly/2020/04/30/report-389-ds-base-1.4.4.1-20200429gitc7da66e.fc31.x86_64.html

We can fix the tests, of course. But this change has a big impact on existing installations that have invalid acis already. Existing invalid acis will be ignored:

[30/Apr/2020:07:37:54.864068234 +0000] - ERR - NSACLPlugin - __aclp__init_targetattr - targetattr has an invalid value (targetattr=*)
[30/Apr/2020:07:37:54.868470074 +0000] - ERR - NSACLPlugin - acllist_insert_aci_needsLock_ext - ACL PARSE ERR(rv=-5): (targetattr=*
[30/Apr/2020:07:37:54.872568916 +0000] - ERR - NSACLPlugin - __aclinit_handler - This  ((targetattr=*)(targetfilter="(objectClass=inetOrgPerson)")(version 3.0; acl "Test aci"; allow (read, search, compare)(userdn="ldap:///anyone");)) ACL will not be considered for evaluation because of syntax errors.

That also includes FreeIPA, as they have targetattr=* or unquoted values in a quite few places.

I suggest to roll back this change for now, sync with FreeIPA team so they can fix their acis.
We also need to provide a smooth upgrade path (healthcheck tool + fixup task?) and have a release note.

@vashirov - Do the aci's that use "targetattr=*" work correctly? I can change the fix to just reduce the multiple equal signs into a single one - that would also resolve this.

DS complains about them too and they get ignored:
https://pagure.io/freeipa/issue/8301
https://pagure.io/dogtagpki/issue/3173

I filed PRs for freeipa and dogtag to address these issues:
https://github.com/freeipa/freeipa/pull/4623
https://github.com/dogtagpki/pki/pull/397

I think that in the end we should enforce the correct syntax. But we also need to help users to identify incorrect ACIs (via https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/aci.py#_62 for example) and fix them before we do the enforcement.

Plenty of history says people won't read the release notes, so I think maybe we should check for a leading " or * instead?

Plenty of history says people won't read the release notes, so I think maybe we should check for a leading " or * instead?

@firstyear - I think you meant to update a different PR :-)

Anyway, that's not good enough because you can use a single attribute: targetattr=cn In code we explicitly state you don't need quotes for single values, although in the docs we ALWAYS use quotes - so that is the syntax we are striving for, but in order to not break any existing deployments we need to allow:

  • targetattr=*
  • targetattr=ABCabc

The next plan is add the ACI checks into healthcheck to find these ACI's and let customers know they are "invalid" and should be changed. Then at a later date/release we can flip the switch to enforce proper syntax.

No correct PR :) https://pagure.io/389-ds-base/pull-request/51055#comment-118222 this comment viktor mentions to release note this breaking change.

I think having this in the healthcheck is a better place than just release notes, and perhaps even in an upgrade/migration guide we require that people run a healthcehck before they start?

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4108

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
4 years ago
Metadata
None
No Tags
Changes Summary 2
+2 -2
file changed
dirsrvtests/tests/suites/acl/syntax_test.py
+29 -5
file changed
ldap/servers/plugins/acl/aclparse.c
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.