Setting 'nTSecurityDescriptor' via LDAP fails

[Zahari Z.]

Andrew Bartlett wrote:
> On Tue, 2009-03-10 at 14:34 +0100, Stefan (metze) Metzmacher wrote:
>   
>> Zahari Z. schrieb:
>>     
>>> Andrew Bartlett wrote:
>>>       
>>>> On Fri, 2009-03-06 at 15:11 +0200, Zahari Z. wrote:
>>>>  
>>>>         
>>>>> Hello Andrew and Samba4,
>>>>>
>>>>> I am raising this issue again. This is about sending ndr_packed()
>>>>> nTsecurityDescriptor object via LDAP connection.
>>>>>     
>>>>>           
>>>>  
>>>>         
>>>>> Hope the explanation is clear and you would be able to help us
>>>>> overcome this LDAP situation.
>>>>>     
>>>>>           
>>>> Does this test pass against Windows 2003 or 2008?
>>>>
>>>> Andrew Bartlett
>>>>
>>>>   
>>>>         
>>> Hello Andrew,
>>>
>>> It does not pass against Windows2003. It crushes with 'Constrain error'
>>> that resolves according to winerror.h this error sesolves to 'Invalid
>>> nTSecurityDescriptor'.
>>>
>>> See the error against Win2003:
>>>
>>> Traceback (most recent call last):
>>>  File "./lib/ldb/tests/python/acl-test.py", line 100, in test_acl_read
>>>    "ntSecurityDescriptor" : ndr_pack(x),
>>> LdbError: (19, 'LDAP error 19 LDAP_CONSTRAINT_VIOLATION -  <0000053A:
>>> AtrErr: DSID-03150B5E, #1:\n\t0: 0000053A: DSID-03150B5E, problem 1005
>>> (CONSTRAINT_ATT_TYPE), data 0, Att 20119 (nTSecurityDescriptor)\n> <>')
>>>
>>> My guess is that something happens at the moment of writing to database
>>> or while sending.
>>>       
>> I think you need to use the a control:
>> http://msdn.microsoft.com/library/default.asp?url=/library/en-us/ldap/ldap/ldap_server_sd_flags_oid.asp
>>     
>
> Once this is fixed, I think the issue may be due to different formats of
> the attribute (samba translates between text and binary).  Try printing
> the original value obtained over LDAP to see how it differs before you
> try parsing.
>
> Andrew Bartlett
>
>   
Hello Samba4,

We want to announce  :)   that we have successfully debugged and fixed 
the problem with ndr_pack(nTSecutityDescriptor) sending via LDAP using 
Samba4 ldb.add().

(1) What the problem really was?
 
The issue was that sending binary data using ldb.add() did not work. The 
issue was not raised before now as most of the properties that are set 
by ldb.add() are plain strings.We, however, tried using it to set 
ntSecurityDescriptor in binary format. In file 
source4/lib/ldb/pyldb.c:1276, function PyObject_AsMessageElement() where 
data from python objects is converter into am ldb_message, 
talloc_strndup is used to copy/duplicate data. When we tried to send 
binary data (nTSecurityDescriptor) it did not work
because there was a null character '\0' e.g. "bla\0foo". When 
talloc_strndup() reaches a null character it stops copying and data is 
transmitted with wrong value but correct size. Having wrong value 
transmitted through LDAP, Win2003 immediately throws "Constraint 
violation" due to data inconsistency.

(2) How did we end up fixing the problem?

What fixed the problem and did not raise more issues is: substitute 
talloc_strndup() with talloc_memdup(), allocate 1 more character and put 
null character ('\0') in the extra place so data copied is null terminated.

(3) What happens when we just changed talloc_strndup() to talloc_memdup()?

This is what we first tried. Unfortunately after double checking with 
"make test" we saw more errors were generated. The most obvious issue 
was source4/setup/newuser script was crashing with LDAP error because of 
data inconsistency. More characters were copied after talloc_memdup() 
than stated by its size. Got back to debugging and came to the 
conclusion that successive data handling is based on that data is null 
terminated string and strlen() defines the final length of the value not 
the real size passed along. After null terminating processed data came 
out of memdup() everything seems to be OK. Our concern is that null 
terminating works around a bigger problem with data size handling 
somewhere in the ldb library - size is being calculated with strlen() 
instead of using the value[].length field.

Diff:
------
diff --git a/source4/lib/ldb/pyldb.c b/source4/lib/ldb/pyldb.c
index 81b9609..aa3f02b 100644
--- a/source4/lib/ldb/pyldb.c
+++ b/source4/lib/ldb/pyldb.c
@@ -1273,9 +1273,11 @@ struct ldb_message_element 
*PyObject_AsMessageElement(TALLOC_CTX *mem_ctx,
               me->num_values = 1;
               me->values = talloc_array(me, struct ldb_val, 
me->num_values);
               me->values[0].length = PyString_Size(set_obj);
-              me->values[0].data = (uint8_t *)talloc_strndup(me->values,
+              me->values[0].data = (uint8_t *)talloc_memdup(me->values,
                                       PyString_AsString(set_obj),
-                                      me->values[0].length);
+                                      me->values[0].length + 1);
+              me->values[0].data[me->values[0].length] = '\0';
+
       } else if (PySequence_Check(set_obj)) {
               int i;
               me->num_values = PySequence_Size(set_obj);
diff --git a/source4/lib/ldb/tests/python/ldap.py 
b/source4/lib/ldb/tests/python/ldap.py
index a30273f..1824053 100755
--- a/source4/lib/ldb/tests/python/ldap.py
+++ b/source4/lib/ldb/tests/python/ldap.py
@@ -90,6 +90,20 @@ class BasicTests(unittest.TestCase):
        except LdbError, (num, _):             self.assertEquals(num, 
ERR_NO_SUCH_OBJECT)

+    def test_zero_byte_string(self):
+        """ Testing we do not get trapped in the '\0' byte in a 
property string"""
+        user_dn = "cn=ldaptestuser,cn=users," + self.base_dn
+        self.delete_force(self.ldb, user_dn)
+        ldb.add({
+            "dn" : user_dn,
+            "objectclass" : "user",
+            "cN" : "LDAPtestUSER",
+            "givenname" : "ldap",
+            "displayname" : "foo\0bar",
+        })
+        res = self.ldb.search( self.base_dn, expression="(dn=%s)" % 
user_dn )
+        self.assertEquals( "foo\0bar", res[0]["displayname"][0] )
+
    def test_all(self):
        """Basic tests"""

You can pull the fix from here:
--------------------------------
Repository: git://repo.or.cz/Samba/aatanasov.git
Branch: master_fix_in_PyObject_AsMessageElement
Commit: 2e6e9977576a890efda7dee1e54f66ba7c43d587

-Zahari, Sofia