[PATCH] s4:dsdb/common: samdb_result_parameters: fix bug in ldb_val to lsa_BinaryString conversation
Andrew Bartlett
abartlet at samba.org
Fri May 30 13:57:48 MDT 2014
On Fri, 2014-05-30 at 16:54 +0200, Stefan Gohmann wrote:
> Am 30.05.2014 01:29, schrieb 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
> >
>
> Thanks for your explanation.
>
> I did some more tests. If I use a plain Active Directory (Windows 2008
> Server R2) without any Samba 4 connection, create a test user and set
> the Windows Terminal Server Profile Path to '\\testserver\path', I get
> the following value for userParameters:
>
>
> ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUAUaCAFDdHhDZmdQcmVzZW5045S15pSx5oiw44GiGAgBQ3R4Q2ZnRmxhZ3Mx44Cw44Gm44Cy44C5EggBQ3R4U2hhZG9344Sw44Cw44Cw44CwKgIBQ3R4TWluRW5jcnlwdGlvbkxldmVs44SwICQBQ3R4V0ZQcm9maWxlUGF0aOaMteaMteOQt+OUtuOMt+OQt+OMt+OUtuOIt+OYt+OUtuOIt+aMteOAt+OEtuOQt+OgtuOAsA==
>
> Decoding via python shows the following:
>
> >>>
> len(base64.decodestring('ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUAUaCAFDdHhDZmdQcmVzZW5045S15pSx5oiw44GiGAgBQ3R4Q2ZnRmxhZ3Mx44Cw44Gm44Cy44C5EggBQ3R4U2hhZG9344Sw44Cw44Cw44CwKgIBQ3R4TWluRW5jcnlwdGlvbkxldmVs44SwICQBQ3R4V0ZQcm9maWxlUGF0aOaMteaMteOQt+OUtuOMt+OQt+OMt+OUtuOIt+OYt+OUtuOIt+aMteOAt+OEtuOQt+OgtuOAsA=='))
> 229
> >>>
> base64.decodestring('ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUAUaCAFDdHhDZmdQcmVzZW5045S15pSx5oiw44GiGAgBQ3R4Q2ZnRmxhZ3Mx44Cw44Gm44Cy44C5EggBQ3R4U2hhZG9344Sw44Cw44Cw44CwKgIBQ3R4TWluRW5jcnlwdGlvbkxldmVs44SwICQBQ3R4V0ZQcm9maWxlUGF0aOaMteaMteOQt+OUtuOMt+OQt+OMt+OUtuOIt+OYt+OUtuOIt+aMteOAt+OEtuOQt+OgtuOAsA==')
> '
> P\x05\x1a\x08\x01CtxCfgPresent\xe3\x94\xb5\xe6\x94\xb1\xe6\x88\xb0\xe3\x81\xa2\x18\x08\x01CtxCfgFlags1\xe3\x80\xb0\xe3\x81\xa6\xe3\x80\xb2\xe3\x80\xb9\x12\x08\x01CtxShadow\xe3\x84\xb0\xe3\x80\xb0\xe3\x80\xb0\xe3\x80\xb0*\x02\x01CtxMinEncryptionLevel\xe3\x84\xb0
> $\x01CtxWFProfilePath\xe6\x8c\xb5\xe6\x8c\xb5\xe3\x90\xb7\xe3\x94\xb6\xe3\x8c\xb7\xe3\x90\xb7\xe3\x8c\xb7\xe3\x94\xb6\xe3\x88\xb7\xe3\x98\xb7\xe3\x94\xb6\xe3\x88\xb7\xe6\x8c\xb5\xe3\x80\xb7\xe3\x84\xb6\xe3\x90\xb7\xe3\xa0\xb6\xe3\x80\xb0'
> >>>
> base64.decodestring('ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUAUaCAFDdHhDZmdQcmVzZW5045S15pSx5oiw44GiGAgBQ3R4Q2ZnRmxhZ3Mx44Cw44Gm44Cy44C5EggBQ3R4U2hhZG9344Sw44Cw44Cw44CwKgIBQ3R4TWluRW5jcnlwdGlvbkxldmVs44SwICQBQ3R4V0ZQcm9maWxlUGF0aOaMteaMteOQt+OUtuOMt+OQt+OMt+OUtuOIt+OYt+OUtuOIt+aMteOAt+OEtuOQt+OgtuOAsA==').encode('hex')
> '20202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202050051a080143747843666750726573656e74e394b5e694b1e688b0e381a2180801437478436667466c61677331e380b0e381a6e380b2e380b9120801437478536861646f77e384b0e380b0e380b0e380b02a02014374784d696e456e6372797074696f6e4c6576656ce384b0202401437478574650726f66696c6550617468e68cb5e68cb5e390b7e394b6e38cb7e390b7e38cb7e394b6e388b7e398b7e394b6e388b7e68cb5e380b7e384b6e390b7e3a0b6e380b0'
> >>>
>
> Opening the user in MMC shows an unicode value for the attribute
> userParameters in the attribute editor. And the substring
> CtxWFProfilePath doesn't look like UTF-16.
>
> I think if I join Samba 4 into this AD domain and try to login against
> Samba 4, the rpc server will get stuck. The patch should fix this case.
> Or am I missing something?
What you are missing is that access over LDAP is not a view of the
'on-disk' data in AD (you only see the on-disk format over DRS). It is
the on-disk data converted to 'UTF8'.
In the past, Samba always strictly matched the LDAP view with the LDB
view, and made that match Microsoft's AD, but this is no longer the case
for userParameters, which now attempts to match SAMR instead.
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
More information about the samba-technical
mailing list