389-ds-base

Clone
Source Code
GIT

#50997 Issue 50869 - Setting nsslapd-allowed-sasl-mechanisms truncates the value
Closed 3 years ago by spichugi. Opened 4 years ago by spichugi.
spichugi/389-ds-base i50869  into  master

file modified
+26 
@@ -176,6 +176,32 @@ 
 

      assert(set(final_mechs) == set(orig_mechs))
 

  

  

+ @pytest.mark.bz1816854
 

+ @pytest.mark.ds50869
 

+ def test_config_set_few_mechs(topology_st):
 

+     """Test that we can successfully set multiple values to nsslapd-allowed-sasl-mechanisms
 

+ 

+     :id: d7c3c58b-4fbe-42ab-a8d4-9dd362916d5f
 

+     :setup: Standalone instance
 

+     :steps:
 

+         1. Set nsslapd-allowed-sasl-mechanisms to "PLAIN GSSAPI"
 

+         2. Verify nsslapd-allowed-sasl-mechanisms has the values
 

+     :expectedresults:
 

+         1. Operation should be successful
 

+         2. Operation should be successful
 

+     """
 

+ 

+     standalone = topology_st.standalone
 

+ 

+     standalone.log.info("Set nsslapd-allowed-sasl-mechanisms to 'PLAIN GSSAPI'")
 

+     standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN GSSAPI')
 

+ 

+     standalone.log.info("Verify nsslapd-allowed-sasl-mechanisms has the values")
 

+     allowed_mechs = standalone.config.get_attr_val_utf8('nsslapd-allowed-sasl-mechanisms')
 

+     assert('PLAIN' in allowed_mechs)
 

+     assert('GSSAPI' in allowed_mechs)
 

+ 

+ 

  if __name__ == '__main__':
 

      # Run isolated
 

      # -s for DEBUG mode

file modified
+2 
@@ -325,6 +325,8 @@ 
 

  /*
 

   * extended version of str2charray lets you disallow
 

   * duplicate values into the array.
 

+  * Also, "char *str" should be a temporary string which is freed afterwards.
 

+  * the string is changed during this function execution
 

   */
 

  char **
 

  slapi_str2charray_ext(char *str, char *brkstr, int allow_dups)

file modified
+6 -1 
@@ -7339,9 +7339,12 @@ 
 

      /* During a reset, the value is "", so we have to handle this case. */
 

      if (strcmp(value, "") != 0) {
 

          char *nval = slapi_ch_strdup(value);
 

+         /* A separate variable is used because slapi_str2charray_ext can change it and nval'd become corrupted */
 

+         char *tmp_array_nval;
 

  

          /* cyrus sasl doesn't like comma separated lists */
 

          remove_commas(nval);
 

+         tmp_array_nval = slapi_ch_strdup(nval);
 

  

          if (invalid_sasl_mech(nval)) {
 

              slapi_log_err(SLAPI_LOG_ERR, "config_set_allowed_sasl_mechs",
 
@@ -7350,13 +7353,15 @@ 
 

                            "digits, hyphens, or underscores\n",
 

                            nval);
 

              slapi_ch_free_string(&nval);
 

+             slapi_ch_free_string(&tmp_array_nval);
 

              return LDAP_UNWILLING_TO_PERFORM;
 

          }
 

          CFG_LOCK_WRITE(slapdFrontendConfig);
 

          slapi_ch_free_string(&slapdFrontendConfig->allowed_sasl_mechs);
 

          slapi_ch_array_free(slapdFrontendConfig->allowed_sasl_mechs_array);
 

          slapdFrontendConfig->allowed_sasl_mechs = nval;
 

-         slapdFrontendConfig->allowed_sasl_mechs_array = slapi_str2charray_ext(nval, " ", 0);
 

+         slapdFrontendConfig->allowed_sasl_mechs_array = slapi_str2charray_ext(tmp_array_nval, " ", 0);
 

+         slapi_ch_free_string(&tmp_array_nval);
 

          CFG_UNLOCK_WRITE(slapdFrontendConfig);
 

      } else {
 

          /* If this value is "", we need to set the list to *all* possible mechs */

Bug Description: Adding multiple mechanisms to nsslapd-allowed-sasl-mechanisms ignores all but one of the mechanisms specified.

Fix Description: The issue happens because we use the same memory address
for 'char ' for slapdFrontendConfig->allowed_sasl_mechs and
for slapdFrontendConfig->allowed_sasl_mechs_array.
So when we split the 'char ' into the 'char **' with ' ' delimetr,
allowed_sasl_mechs has only the first element becuase ' ' is set to 0 now.

Define a separate 'char *' for the array.
Add a test for the issue.

https://pagure.io/389-ds-base/issue/50869

Reviewed by: ?

LGTM!

Ack from me too, thanks! (Apologies for william of the past for messing this up :) )

@spichugi, I am sorry but I miss how your fix fixes the issue :(
allowed_sasl_mechs was/is set to nval (list of space separated value).
allowed_sasl_mechs_array was set to a array of value built from nval.
So the two fields were not pointing to the same structure. I miss how the patch changes that.

@spichugi, I am sorry but I miss how your fix fixes the issue :(
allowed_sasl_mechs was/is set to nval (list of space separated value).
allowed_sasl_mechs_array was set to a array of value built from nval.
So the two fields were not pointing to the same structure. I miss how the patch changes that.

Yeah, it was my initial thought too. So I went checking all other parts and spent some time with it...
But everything else was okay.

And then, assuming that slapi_str2charray_ext makes a copy of str *, I've decided to awatch the slapdFrontendConfig->allowed_sasl_mechs piece of memory to see where it is modified and truncated (because I thought that something may happen cuncurrently when we unlock slapdFrontendConfig).

But it's turned out that the issue happens in slapi_str2charray_ext where we work with a pointer to str (our string we want to convert to an array)
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/charray.c#_347

So when we find a separator (i.e. " "), we get the pointer and we set it to 0.
https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/utf8.c#_306

So basically, the array forallowed_sasl_mechs_array that we get from slapi_str2charray_ext is a former nval string. The same char * we set to allowed_sasl_mechs.

slapi_str2charray_ext is called with 'nval'. It will create an array of value that is different (address) than 'nval'. BUT it modifies 'nval' (during ldap_utf8strtok_r).

for example before the call slapi_str2charray_ext(nval, " ", 0)
slapdFrontendConfig->allowed_sasl_mechs = "PLAIN GSSAPI" slapdFrontendConfig->allowed_sasl_mechs_array = NULL

After the call
slapdFrontendConfig->allowed_sasl_mechs = "PLAIN"
slapdFrontendConfig->allowed_sasl_mechs_array = {"PLAIN" , "GSSAPI"}
but actually slapdFrontendConfig->allowed_sasl_mechs = "PLAIN\0GSSAPI"

Is the bug trigger by the shorten value of slapdFrontendConfig->allowed_sasl_mechs ?

IMHO a problem is in slapi_str2charray_ext that should duplicate 'str' at the beginning of the function because it may corrupt it.

IMHO a problem is in slapi_str2charray_ext that should duplicate 'str' at the beginning of the function because it may corrupt it.

I get the impression that it is an intended behavior... For example, here we work with slapi_str2charray_ext function using tmp_string - https://pagure.io/389-ds-base/blob/master/f/ldap/servers/plugins/replication/repl5_agmt.c#_535

So I didn't want to modify the existing logic in other places where the function is used.

I am not opposed to changing the function's behavior if you think that it is justified.
I really think it is about what we decide and how we define its interface.

But maybe we should create a separate issue for that? (as the function is used with a tmp_string in other places and it looks like it's intended)

All the others places using slapi_str2charray(_ext) are calling it with temporary string, freed immediately after.

I am fine keeping slapi_str2charray_ext with the current behavior.
Please add warning comment in the head of slapi_str2charray_ext stating that it can change the input string.
Also in your patch you may comment that tmp_array_nval is used (instead of nval) because slapi_str2charray can change it.

rebased onto dd266da
4 years ago

Thanks! I added the comments.

Thanks. This looks good to me as well. Ack

Pull-Request has been merged by spichugi
4 years ago

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/4050

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
3 years ago
Metadata
None
No Tags
Changes Summary 3
+26 -0
file changed
dirsrvtests/tests/suites/sasl/allowed_mechs_test.py
+2 -0
file changed
ldap/servers/slapd/charray.c
+6 -1
file changed
ldap/servers/slapd/libglobs.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.