Bug 9147 - winbind can't fetch user or group info from AD via LDAP
winbind can't fetch user or group info from AD via LDAP
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: Winbind
3.6.7
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-07 19:17 UTC by Sergey Korsak
Modified: 2012-09-17 09:34 UTC (History)
1 user (show)

See Also:


Attachments
Test patch. (838 bytes, patch)
2012-09-08 00:23 UTC, Jeremy Allison
no flags Details
Fixed test patch :-) (838 bytes, patch)
2012-09-08 00:25 UTC, Jeremy Allison
no flags Details
One more fix.. (1.69 KB, patch)
2012-09-08 00:30 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.x (2.20 KB, patch)
2012-09-10 22:17 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.next (2.23 KB, patch)
2012-09-10 23:08 UTC, Jeremy Allison
no flags Details
List of SIDs in textual format (11.39 KB, text/plain)
2012-09-11 19:39 UTC, Sergey Korsak
no flags Details
List of SIDs in bianry format (21.25 KB, text/plain)
2012-09-11 19:41 UTC, Sergey Korsak
no flags Details
objectSid.text.txt (13.25 KB, text/plain)
2012-09-11 19:42 UTC, Sergey Korsak
no flags Details
objectSid.bin.txt (13.25 KB, text/plain)
2012-09-11 19:42 UTC, Sergey Korsak
no flags Details
git-am fix for 3.6.next (3.30 KB, patch)
2012-09-11 20:23 UTC, Jeremy Allison
no flags Details
(correct) git-am fix for 3.6.next. (2.59 KB, patch)
2012-09-11 20:26 UTC, Jeremy Allison
obnox: review+
Details
git-am fix for 3.5.next. (2.59 KB, patch)
2012-09-11 20:32 UTC, Jeremy Allison
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Korsak 2012-09-07 19:17:46 UTC
...
As a result, "wbinfo --user-info", "wbinfo --group-info", "getent passwd" 'domain\user' fail, "getent group" shows empty domain groups, "valid users = +domain\group" doesn't work.

The reason is winbindd incorrectly construct (objectSid=%s) filter while queris AD LDAP.
It sends something like
objectSid=\01\05\00\00\00\00\00\05\15\00\00\00²\05╔\0EО┌\09▒HM\29\0B\22\10\00\00
instead of
objectSid=S-1-5-21-245695901-2433319663-187256136-4130
So, LDAP answers with 0 objects.

//////////////////////// My patch is: /////////////////////////
--- source3/winbindd/winbindd_ads.c     2012-09-07 20:41:57.000000000 +0300
+++ source3/winbindd/winbindd_ads.c     2012-09-07 21:27:39.000000000 +0300
@@ -549,7 +549,7 @@
                return NT_STATUS_SERVER_DISABLED;
        }
 
-       sidstr = ldap_encode_ndr_dom_sid(talloc_tos(), sid);
+       sidstr = sid_string_talloc(talloc_tos(), sid);
 
        ret = asprintf(&ldap_exp, "(objectSid=%s)", sidstr);
        TALLOC_FREE(sidstr);
@@ -1059,7 +1059,7 @@
                goto done;
        }
 
-       if ((sidbinstr = ldap_encode_ndr_dom_sid(talloc_tos(), group_sid)) == NULL) {
+       if ((sidbinstr = sid_string_talloc(talloc_tos(), group_sid)) == NULL) {
                status = NT_STATUS_NO_MEMORY;
                goto done;
        }
//////////////////////// END of patch /////////////////////////

Some more pieces of code possibly need to be patched in the same manner, I did'n ispected more than troubled me.
Comment 1 Jeremy Allison 2012-09-07 21:22:34 UTC
This page:

http://www.pcreview.co.uk/forums/re-objectsid-ldap-search-t1458615.html

disagrees with you. The relevant part:

--------------------------------------------------------
The objectSid attribute is binary-valued, so to search on it, you have to
use the binary value of the SID. Binary values are represented in LDAP
search filters as \xx, where "xx" are two hexadecimal digits. The details
of LDAP search filters are covered in RFC 2254 (available at
http://www.ietf.org/rfc/rfc2254.txt).

For example, suppose your SID in string form was
S-1-5-21-2562418665-3218585558-1813906818-1576. In binary form, this is
{01,05,00,00,00,00,00,05,15,00,00,00,e9,67,bb,98,d6,b7,d7,bf,82,05,1e,6c,28,06,00,00},
so the LDAP search filter would be:

(objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\e9\67\bb\98\d6\b7\d7\bf\82\05\1e\6c\28\06\00\00)

Thanks,
Matthew Rimer [MSFT]
--------------------------------------------------------

So we'll need to do some testing here.

Jeremy.
Comment 2 Jeremy Allison 2012-09-07 22:05:03 UTC
Here is the function that is actually encoding the SID binary blob into a string for the LDAP query:

/*
   encode a blob as a RFC2254 binary string, escaping any
   non-printable or '\' characters
*/
char *ldb_binary_encode(void *mem_ctx, struct ldb_val val)
{
        size_t i;
        char *ret;
        size_t len = val.length;
        unsigned char *buf = val.data;

        for (i=0;i<val.length;i++) {
                if (!isprint(buf[i]) || strchr(" *()\\&|!\"", buf[i])) {
                        len += 2;
                }
        }
        ret = talloc_array(mem_ctx, char, len+1);
        if (ret == NULL) return NULL;

        len = 0;
        for (i=0;i<val.length;i++) {
                if (!isprint(buf[i]) || strchr(" *()\\&|!\"", buf[i])) {
                        snprintf(ret+len, 4, "\\%02X", buf[i]);
                        len += 3;
                } else {
                        ret[len++] = buf[i];
                }
        }

        ret[len] = 0;

        return ret;     
}

I'm suspicious of the isprint() function on your platform, as it seems to be returning "true" for characters like ² ╔ etc., which probably should not be printable.
Comment 3 Jeremy Allison 2012-09-07 22:11:51 UTC
Although according to :

https://www.ietf.org/rfc/rfc2254.txt

the only characters we must encode are:

           Character       ASCII value
           ---------------------------
           *               0x2a
           (               0x28
           )               0x29
           \               0x5c
           NUL             0x00
Comment 4 Sergey Korsak 2012-09-07 22:46:01 UTC
> Here is the function that is actually encoding the SID binary blob into a
> string for the LDAP query:
> 
> /*
>    encode a blob as a RFC2254 binary string, escaping any
>    non-printable or '\' characters
> */
> char *ldb_binary_encode(void *mem_ctx, struct ldb_val val)
> {
...
> }
> 
> I'm suspicious of the isprint() function on your platform, as it seems to be
> returning "true" for characters like ² ╔ etc., which probably should not be
> printable.

1. ОК, but how do we explain that the changes I've done solves the problem?
2. Why
LDAPTLS_REQCERT=never ldapsearch -H ldaps://dc1.1plus1.corp/ -b
dc=1plus1,dc=corp -D s.korsak@1plus1.corp -W
'(objectSid=S-1-5-21-245695901-2433319663-187256136-500)'
returns whole Administrator's record inspite of STRING (not BINARY) syntax in
the filter?

May be my DCs configuration is screwed and does not accept binary objectSids, but only ascii?

The platform is FreeBSD/amd64 8.3-RELEASE.
I'll check tomorrow what isprint() returns. Man page is here:
http://www.freebsd.org/cgi/man.cgi?query=isprint&apropos=0&sektion=3&manpath=FreeBSD+8.3-RELEASE&arch=amd64&format=html
I'll also check what
LDAPTLS_REQCERT=never net ads -k search '(objectSid=S-1-5-21-245695901-2433319663-187256136-500)'
sends do LDAP server. I beieve it sends string "S-...", because it works with default installation, while winbindd does not.
Comment 5 Jeremy Allison 2012-09-07 23:45:29 UTC
Well you are correct... Hmmm.

I used a simple bind ldapsearch request using a text based query:

ldapsearch -x -LLL -H ldap://172.27.69.195 -b 'dc=AAA,dc=BBB,dc=CCC,dc=com' -D 'Administrator@AAA.BBB.CCC.com' -w 'password' '(objectSid=S-1-5-32-548)'

and did a wireshark capture of the packets, and it does clearly show the SID being sent as plaintext over the wire.

I got back:

dn: CN=Account Operators,CN=Builtin,DC=AAA,DC=BBB,DC=CCC,DC=com
objectClass: top
objectClass: group
cn: Account Operators
description: Members can administer domain user and group accounts
distinguishedName: CN=Account Operators,CN=Builtin,DC=AAA,DC=BBB,DC=CCC,DC=com
instanceType: 4
whenCreated: 20120320161211.0Z
whenChanged: 20120320162722.0Z
uSNCreated: 12360
uSNChanged: 12708
name: Account Operators
objectGUID:: 3TFx+gv1MkKJ1XsC/TExPQ==
objectSid:: AQIAAAAAAAUgAAAAJAIAAA==

The objectSID reply is coming back as the binary representation, but it certainly seems the query for objectSid should be done as text.
Comment 6 Jeremy Allison 2012-09-07 23:54:30 UTC
Oh, not so fast. Looks like the query can be done by both text, *OR* by binary representation.

Sid "S-1-5-32-548" is a binary string of: "\01\02\00\00\00\00\00\05\20\00\00\00\24\02\00\00"

(as read from the wireshark return). So doing the query as:

ldapsearch -x -LLL -H ldap://172.27.69.195 -b 'dc=AAA,dc=BBB,dc=CCC,dc=com' -D
'Administrator@AAA.BBB.CCC.com' -w 'password' '(objectSid=\01\02\00\00\00\00\00\05\20\00\00\00\24\02\00\00)'

returns exactly the same result:

dn: CN=Account Operators,CN=Builtin,DC=AAA,DC=BBB,DC=CCC,DC=com
objectClass: top
objectClass: group
cn: Account Operators
description: Members can administer domain user and group accounts
distinguishedName: CN=Account Operators,CN=Builtin,DC=AAA,DC=BBB,DC=CCC,DC=com
instanceType: 4
whenCreated: 20120320161211.0Z
whenChanged: 20120320162722.0Z
uSNCreated: 12360
uSNChanged: 12708
name: Account Operators
objectGUID:: 3TFx+gv1MkKJ1XsC/TExPQ==
objectSid:: AQIAAAAAAAUgAAAAJAIAAA==

So what we're doing in winbindd should certainly work.
Comment 7 Jeremy Allison 2012-09-08 00:06:35 UTC
Yep, just to confirm I tried with a mix of \XX and ascii values and was able to retrieve my own account details via objectSid query.

You need to look closely at the encoding of what is going over the wire on your platform. I can give you a simple patch that will force \XX encoding of all values, but they certainly should be allowed.
Comment 8 Jeremy Allison 2012-09-08 00:09:47 UTC
Yep, on this DC my SID is:

objectSid=S-1-5-21-4068062641-1244396112-413245233-1103

which I can also retrieve as :

objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\b1\b5y\f2P\fa\2bJ\31\9f\a1\18O\04\00\00
Comment 9 Sergey Korsak 2012-09-08 00:15:13 UTC
(In reply to comment #8)
> Yep, on this DC my SID is:
> 
> objectSid=S-1-5-21-4068062641-1244396112-413245233-1103
> 
> which I can also retrieve as :
> 
> objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\b1\b5y\f2P\fa\2bJ\31\9f\a1\18O\04\00\00

So can I. Both variants returns "Account Operators" record in my
environment, this:
LDAPTLS_REQCERT=never ldapsearch -H ldaps://dc1.1plus1.corp/ -b
dc=1plus1,dc=corp -D s.korsak@1plus1.corp -W '(objectSid=S-1-5-32-548)'
and this:
LDAPTLS_REQCERT=never ldapsearch -H ldaps://dc1.1plus1.corp/ -b
dc=1plus1,dc=corp -D s.korsak@1plus1.corp -W
'(objectSid=\01\02\00\00\00\00\00\05\20\00\00\00\24\02\00\00)'

So the only suspicion I have now is ldb_binary_encode() returns somewhat wrong
"binary string". Maybe it does not convert some characters because of LC_xxx
environment. I'll research this tomorrow.
Comment 10 Jeremy Allison 2012-09-08 00:18:22 UTC
Oh, I think I know what this is !!!

Your isprint() is returning true for the characters like ² ╔, and I wonder if Windows is mapping them incorrectly !

I have a quick patch which might help. Hang on a minute.
Comment 11 Jeremy Allison 2012-09-08 00:23:56 UTC
Created attachment 7864 [details]
Test patch.

Give this a try. If forces all characters outside of the US-ASCII range (and the prohibited ones) to be encoded as \XX.
Comment 12 Jeremy Allison 2012-09-08 00:25:54 UTC
Created attachment 7865 [details]
Fixed test patch :-)

Got 0x32 and 0x20 mixed up for space char :-). Try this.
Comment 13 Jeremy Allison 2012-09-08 00:30:59 UTC
Created attachment 7866 [details]
One more fix..

Belt and braces :-). Fixes both implementations of ldb_binary_encode() to avoid using isprint().
Comment 14 Sergey Korsak 2012-09-08 06:58:38 UTC
(In reply to comment #13)
> Created attachment 7866 [details]
> One more fix..
> 
> Belt and braces :-). Fixes both implementations of ldb_binary_encode() to avoid
> using isprint().

Unpatched winbindd works OK if started with LC_ALL=C environment. It founds user:
Search for (objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\9D\05\A5\0E\EF\82\09\91HM\29\0B\A4\14\00\00) in <dc=1PLUS1,dc=CORP> gave 1 replies

If LC_ALL=uk_UA.CP1251, it fails:
Search for (objectSid=\01\05\00\00\00\00\00\05\15\00\00\00²\05╔\0EО┌\09▒HM\29\0Bє\14\00\00) in <dc=1PLUS1,dc=CORP> gave 0 replies

Patched winbindd now works OK inspite of locale settings.

Thank you, Jeremy!
Comment 15 Jeremy Allison 2012-09-10 22:17:55 UTC
Created attachment 7874 [details]
git-am fix for 3.6.x
Comment 16 Jeremy Allison 2012-09-10 22:18:17 UTC
Comment on attachment 7874 [details]
git-am fix for 3.6.x

Same code went into master.
Comment 17 Jeremy Allison 2012-09-10 23:08:34 UTC
Created attachment 7876 [details]
git-am fix for 3.5.next

Same fix for 3.5.next.
Comment 18 Michael Adam 2012-09-11 08:02:35 UTC
Jeremy, sorry, but this patch seems subobtimal to me.
The check against concrete numbers is repeated verbatim in multiple places.

And if you want to restrict to ascii, why don't you use "isascii()" ?
That would read much less arbitrary.

Cheers - Michael
Comment 19 Michael Adam 2012-09-11 08:41:08 UTC
Hmm, ok isascii is reported deprecated and not portable in the manpages...

Is there an alternative existing function to provide what you want to achieve?
Or could we at least create our own function?

Cheers - Michael
Comment 20 Jeremy Allison 2012-09-11 16:08:07 UTC
It's a fair cop :-). I've factored out to a function in master, I'll re-spin the patches to do the same. FYI - can't use isascii as the restriction we need isn't the same as that set.

Cheers,

Jeremy.
Comment 21 Sergey Korsak 2012-09-11 19:39:54 UTC
Created attachment 7879 [details]
List of SIDs in textual format
Comment 22 Sergey Korsak 2012-09-11 19:41:12 UTC
Created attachment 7880 [details]
List of SIDs in bianry format
Comment 23 Sergey Korsak 2012-09-11 19:42:21 UTC
Created attachment 7881 [details]
objectSid.text.txt
Comment 24 Sergey Korsak 2012-09-11 19:42:58 UTC
Created attachment 7882 [details]
objectSid.bin.txt
Comment 25 Sergey Korsak 2012-09-11 19:45:01 UTC
I've created list of SIDs in my AD that differ by LSbyte, 0 to 255.
The textual version is in sids0toff.text.txt (the 1st attach),
binary version is in objectSid.bin.txt (the 2nd attach).

Then I've executed,
LDAPTLS_REQCERT=never ldapsearch -H ldaps://dc1.1plus1.corp/ -b
dc=1plus1,dc=corp -D s.korsak@1plus1.corp -y .ldappass -f sids0toff.text.txt
'(objectSid=%s)' objectSid | grep ^objectSid > objectSid.text.txt
LDAPTLS_REQCERT=never ldapsearch -H ldaps://dc1.1plus1.corp/ -b
dc=1plus1,dc=corp -D s.korsak@1plus1.corp -y .ldappass -f sids0toff.bin.txt
'(objectSid=%s)' objectSid | grep ^objectSid > objectSid.bin.txt

The results are competely equal, see two last attaches (objectSid.text.txt &
objectSid.bin.txt).
dc1.1plus1.corp runs Windows Server 2008 R2 Standard 7601 Service Pack 1.

Do you know any official reference that describes the exact format of objectSid
filter? Must it be S-txt-txt... or backslash escaped-sequence? In the last
case, what bytes Must be esacaped, what bytes Must Not be escaped, and what
bytes format has no matter?

My case only shows that in AD v.2008 environment we can safely use textual or
fully-backslash-escaped SIDs in LDAP '(objectSid=%s)' filter.
Comment 26 Jeremy Allison 2012-09-11 20:23:12 UTC
Created attachment 7883 [details]
git-am fix for 3.6.next

Ok, as requested by Michael, abstracted the decision into a fn call.
Comment 27 Jeremy Allison 2012-09-11 20:26:36 UTC
Created attachment 7884 [details]
(correct) git-am fix for 3.6.next.

Uses functions as requested by obnox.
Comment 28 Jeremy Allison 2012-09-11 20:32:40 UTC
Created attachment 7885 [details]
git-am fix for 3.5.next.

Uses functions as requested.
Comment 29 Sergey Korsak 2012-09-12 17:38:56 UTC
For the completeness I've tested what characters must be converted to \XX sequence
to be accepted by AD LDAP in '(objectSid=%s)' filter. The result surprised me
very much - 250 of 256 are accepted as-is.
Only
\00 (nul),
\28 (left parenthesis),
\29 (right parenthesis),
\2A (asterisk)
\5C (backslash)
caused "Bad search filter" error or "Nothig found" result.
But the latest four, being backslashed - \(, \), \*, \\
were accepted by LDAP and returned correct object.

Only nul must be definitely written as \00, but it may quite be caused
by openldap's internal coding (C-style End of String, etc...).
This conclusion is independent of LC_xxx locale setting.

So It's a puzzle why the original (unpatched) winbindd could not fetch info
from LDAP sending not-escaped objectSid filter.

Cheers!
Comment 30 Jeremy Allison 2012-09-12 17:50:07 UTC
I agree your tests show what is expected, but I've always abided by the "be conservative in what you send" code, so I think we're safer with this patch (it's already in master).
Jeremy.
Comment 31 Sergey Korsak 2012-09-12 18:24:59 UTC
> So It's a puzzle why the original (unpatched) winbindd could not fetch info
> from LDAP sending not-escaped objectSid filter.
> 
A possible key is that openldap converts raw characters to \XX sequences
itself.
The behavior of the "net" tool indirectly confirms such guess because it
doesn't find user so:
net ads search
'(objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\9D\05\A5\0E\EF\82\09\91\48\4D\29\0Bя\22\00\00)'
while
ldapsearch
'(objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\9D\05\A5\0E\EF\82\09\91\48\4D\29\0Bя\22\00\00)'
finds.

> I agree your tests show what is expected, but I've always abided by the "be
> conservative in what you send" code, so I think we're safer with this patch
> (it's already in master).
> Jeremy.
I have no objection against the patch. It does what it should. I just think about wouldn't it be safer to remap all 256 characters into \XX sequence.
The byte overtraffic will be negligible, the packet overtraffic will be missing.
Comment 32 Sergey Korsak 2012-09-13 15:11:37 UTC
I've tcpdump-ed what ldapsearch sends to LDAP server.

1. ldapsearch '(objectSid=S-1-5-21-245695901-2433319663-187256136-8959)' :
000001b0  01 00 02 01 00 02 01 00  01 01 00 a3 39 04 09 6f  |...........Ј9..o|
000001c0  62 6a 65 63 74 53 69 64  04 2c 53 2d 31 2d 35 2d  |bjectSid.,S-1-5-|
000001d0  32 31 2d 32 34 35 36 39  35 39 30 31 2d 32 34 33  |21-245695901-243|
000001e0  33 33 31 39 36 36 33 2d  31 38 37 32 35 36 31 33  |3319663-18725613|
000001f0  36 2d 38 39 35 39 30 00  43 af 51 50 5a 59 04 00  |6-89590.CЇQPZY..|
Just cleartext, isn't it ?

2. ldapsearch '(objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\9D\05\A5\0E\EF\82\09\91\48\4D\29\0B\FF\22\00\00)'
000001b0  01 00 02 01 00 02 01 00  01 01 00 a3 29 04 09 6f  |...........Ј)..o|
000001c0  62 6a 65 63 74 53 69 64  04 1c 01 05 00 00 00 00  |bjectSid........|
000001d0  00 05 15 00 00 00 9d 05  a5 0e ef 82 09 91 48 4d  |......ќ.Ґ.п‚.‘HM|
000001e0  29 0b ff 22 00 00 30 00  41 ad 51 50 b4 a4 0b 00  |).я"..0.A­QPґ¤..|
Note bytes 01ca-01e5.
There are no backslashes - just sequence of bytes.

3. ldapsearch '(objectSid=\01\05\00\00\00\00\00\05\15\00\00\00\9D\05\A5\0E\EF\82\09\91\48\4D\29\0Bя\22\00\00)' (one byte is unescaped)
Dump is the same, as in paragraph 2.

What sends "net ads search" is a mystery, because it unconditionally uses StartTLS.

So openldap client or library deals with escaping-unescaping, LDAP server just gets raw byte sequence.

According to http://tools.ietf.org/html/rfc4515#section-3
we must escape octets "*" (\2a), "(" (\28), ")" (\29), "\" (\\5c), NUL (\00) and all octets greater than 0x7F (\80 to \ff). Other octets MAY be escaped.
Comment 33 Michael Adam 2012-09-13 15:48:32 UTC
Comment on attachment 7884 [details]
(correct) git-am fix for 3.6.next.

This reduces the code duplication from 4 to 2.

;-)

Thanks Jeremy
Comment 34 Michael Adam 2012-09-13 15:48:42 UTC
Comment on attachment 7885 [details]
git-am fix for 3.5.next.

This reduces the code duplication from 4 to 2.

;-)

Thanks Jeremy
Comment 35 Michael Adam 2012-09-13 15:49:43 UTC
==> Karolin for inclusion into the releases...
Comment 36 Karolin Seeger 2012-09-17 09:34:51 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!