[PATCH] s4:dsdb/common: samdb_result_parameters: fix bug in ldb_val to lsa_BinaryString conversation

[Andrew Bartlett]

On Tue, 2014-05-27 at 06:37 +0200, Stefan Gohmann wrote:
> Hi Karmen,
> 
> Am 27.05.2014 03:24, schrieb Kamen Mazdrashki:
> > Hi Stefan,
> > 
> > 
> > On Mon, May 26, 2014 at 11:03 PM, Stefan Gohmann <gohmann at univention.de
> > <mailto:gohmann at univention.de>> wrote:
> > 
> >     Hi Matthias,
> > 
> >     Am 26.05.2014 14 <tel:26.05.2014%2014>:09, schrieb Matthias Dieter
> >     Wallnöfer:
> >     > Hi Stefan,
> >     >
> >     > it seems that no other one responded. I think that this is rather a
> >     > problem with the "userParameters" attribute which we still do not
> >     parse
> >     > correctly I think.
> > 
> >     thanks. I've re-checked this issue.
> > 
> >     In my case the userParameters attribute is 105 bytes long. I think the
> >     problem is that the length is odd and so there is one byte missing.
> > 
> >     I was able to reproduce this issue while setting userParameters for a
> >     user to the following value:
> > 
> >     userParameters::
> >     Q3R4Q2ZnUHJlc2VudCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUAIaCAFDdHhDZmdQcmVzZW5045S15pSx5oiw44GiGAgBQ3R4Q2ZnRmxhZ3Mx44Cw44Gm44Cy44C5
> > 
> >     and run
> >       net user /domain username
> >     from a Windows client.
> > 
> >     Attached you'll find an updated patch which fixed the problem for me.
> > 
> > IMHO, the correct fix should be:
> > +s.array = talloc_array(mem_ctx, uint16_t, (val->length + 1) / 2);
> > so we don't allocate  an extra bytes in 'normal' scenario when length is
> > even.
> 
> you are right. Please find a new patch attached.

This may correctly fill in the array without overwriting memory, but it
just touches the tip of the iceburg here.  

For example, do we now leak 1 byte of uninitialised server memory?

How did an odd value get in here?  Probably via our DRS replication, but
possibly via a SAMR call.  

I have to look up the full history, but until Samba 4.1, we really
messed this up, because we treated the value as 'UTF8' in some places,
particularly while replicating.  The values being sent are not valid
UTF-16 sequences, so this would often fail. 

Then we added code to the schema to say that for this attribute, not to
convert it to/from UTF-8.  This means that our LDAP interface now
exposes the UTF-16 (or raw) formatted value.  This may or may not
matter, Microsoft assures us LDAP should not be relied on for this
attribute, but probably should change to a 'algorithmic UTF8' conversion
of the raw bytes. 

The copying of the memory from  to the uint16_t array is also a problem,
because this all assumes we never operate on a big-endian host.

Then there comes the issue of fixing up these 'utf8' formatted blobs in
the database.  We have thoughts on how we could detect these, but we
need to sit down and spend a day or two sorting it out in dbcheck,
preferably with tests.  We need to expand these 'UTF8' sequences out
into the raw bytes we now use as the native storage method.

We also need to sort out the classicupgrade codepath, and the script
metze has been using to migrate customers to AD.

Finally we need to have our tests read and write a real (and not just
short) value via the RPC-SAMR tests, and perhaps validate that with
LDAP.

It is a lot of work, but you have sadly seen what happens when we leave
this area as 'known broken' as we did for 4.1. 

I hope this clarifies this situation,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba