Bug 6136 - LDAP integer search filters don't handle signed/unsigned 32-bit rollover
LDAP integer search filters don't handle signed/unsigned 32-bit rollover
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
unspecified
x64 Linux
: P3 normal
: ---
Assigned To: Andrew Bartlett
Matthias Dieter Wallnöfer
:
Depends on:
Blocks: 6600
  Show dependency treegraph
 
Reported: 2009-02-23 21:19 UTC by Andrew Kroeger
Modified: 2009-08-17 06:55 UTC (History)
1 user (show)

See Also:


Attachments
Packet capture of W2K3 client querying W2K3 DC (69.07 KB, application/octet-stream)
2009-02-23 21:21 UTC, Andrew Kroeger
no flags Details
Packet capture of W2K3 client querying S4 DC (74.37 KB, application/octet-stream)
2009-02-23 21:21 UTC, Andrew Kroeger
no flags Details
Patch to enable correct behaviour on "groupType", "userAccountControl" and "sAMAccountType" (10.61 KB, patch)
2009-06-13 12:22 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to handle integer 32bit attributes correctly (6.57 KB, patch)
2009-06-15 14:58 UTC, Matthias Dieter Wallnöfer
no flags Details
New patch to handle integer 32bit attributes correctly (11.90 KB, patch)
2009-06-16 04:58 UTC, Matthias Dieter Wallnöfer
no flags Details
New patch to handle 32bit integer attributes correctly (10.77 KB, patch)
2009-06-17 14:44 UTC, Matthias Dieter Wallnöfer
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Kroeger 2009-02-23 21:19:14 UTC
When Windows populates its list of available users/groups for assigning file security, it performs a wholeSubtree LDAP search on e.g DC=smb4,DC=internal,DC=id10ts,DC=net with a filter of (groupType=2147483653).  This query from a W2K3 client against an S4 DC does not return any results, while a W2K3 client querying against a W2K3 DC returns 17 results.  This causes users/groups such as "Administrators" and "Server Operators" to not appear in the results.

When we provision a new S4 server, the groupType attribute for "Administrators" and "Server Operators" is set to "-2147483643".  Note that both 2147483653 and -2147483643 result in the same 32-bit field (0x80000005).

The Windows LDAP server returns the same 17 results using either number (2147483653 or -2147483643) as the groupType search filter.  The following ldapsearch commands were used to test this:

1) ldapsearch -H ldap://192.168.5.179 -D cn=administrator,cn=users,dc=ads,dc=internal,dc=id10ts,dc=net -x -w password -b dc=ads,dc=internal,dc=id10ts,dc=net "(groupType=2147483653)"

2) ldapsearch -H ldap://192.168.5.179 -D cn=administrator,cn=users,dc=ads,dc=internal,dc=id10ts,dc=net -x -w password -b dc=ads,dc=internal,dc=id10ts,dc=net "(groupType=-2147483643)"

Testing with ldapsearch against S4 gives 17 results when using -2147483643 as the filter, but no results when using 2147483653 as the filter.  The following ldapsearch commands were used to test this:

1) ldapsearch -H ldap://amd64 -D cn=administrator,cn=users,dc=smb4,dc=internal,dc=id10ts,dc=net -x -w password -b dc=smb4,dc=internal,dc=id10ts,dc=net "(groupType=2147483653)"

2) ldapsearch -H ldap://amd64 -D cn=administrator,cn=users,dc=smb4,dc=internal,dc=id10ts,dc=net -x -w password -b dc=smb4,dc=internal,dc=id10ts,dc=net "(groupType=-2147483643)"

Note:  I may be seeing this because I am running S4 on a 64-bit Linux system.  I am setting up a 32-bit VM to test in & will add another comment once I am done with that testing.
Comment 1 Andrew Kroeger 2009-02-23 21:21:22 UTC
Created attachment 3951 [details]
Packet capture of W2K3 client querying W2K3 DC
Comment 2 Andrew Kroeger 2009-02-23 21:21:50 UTC
Created attachment 3952 [details]
Packet capture of W2K3 client querying S4 DC
Comment 3 Andrew Kroeger 2009-02-24 01:30:46 UTC
I just finished testing in a 32-bit VM, and the same behaviour occurs as was originally seen when running S4 under 64-bit Linux.
Comment 4 Andrew Bartlett 2009-03-09 20:51:18 UTC
We need to add a new '32 bit integer' syntax, handling this rollover to:

lib/ldb-samba/ldif_handlers.c

And we need to use the OID allocated for that in:

static const struct dsdb_syntax dsdb_syntaxes[] 

in dsdb/schema/schema_syntax.c

Comment 5 Matthias Dieter Wallnöfer 2009-04-17 12:35:23 UTC
Okay, I try to find a solution for this issue. It seems that it solely happens without an LDAP backend (plain LDB).
Comment 6 Matthias Dieter Wallnöfer 2009-06-13 12:22:33 UTC
Created attachment 4282 [details]
Patch to enable correct behaviour on "groupType", "userAccountControl" and "sAMAccountType"

- Change some "int" and "unsigned int" variables to int32_t or uint32_t to make sure that the behaviour is the same on all platforms!
- Introduce the right conversion mechanisms for lookups on "groupType", "userAccountControl" and "sAMAccountType" in the "samldb" module. Therefore, all possible backends can now benefit from it.
- Remove the conversion code from the "simple_ldb_map" module which was only used for LDB LDAP backends.
Comment 7 Matthias Dieter Wallnöfer 2009-06-15 09:08:52 UTC
Comment on attachment 4282 [details]
Patch to enable correct behaviour on "groupType", "userAccountControl" and "sAMAccountType"

This should we fix in another way.
Comment 8 Matthias Dieter Wallnöfer 2009-06-15 14:58:14 UTC
Created attachment 4290 [details]
Patch to handle integer 32bit attributes correctly

Now I wrote a much better patch which should solve the problem for "groupType" but also for all integer attributes in general. I followed Andrew's guidelines and so this patch should comply.
Comment 9 Andrew Bartlett 2009-06-15 18:15:50 UTC
That looks much better. 

Comments:

Please don't rename the fucntions to ldb_ as that namespace should be reserved for the 'real' ldb.  (we need a better prefix than ldif_, but it will do for now).

To actually hook it in, see the table in schema_syntax.c and add a new .ldb_syntax = "LDB_SYNTAX_SAMBA_INT32" for it. 

Finally, we just need tests.

This is really good work.  Thank you very much!
Comment 10 Matthias Dieter Wallnöfer 2009-06-16 04:58:19 UTC
Created attachment 4292 [details]
New patch to handle integer 32bit attributes correctly

- The first patch wasn't complete
- I changed now all methods to "ldif_" prefix in the ldif handlers
- Made changes in "schema_syntax.c" were appropriate
- Removed code from "simple_ldap_map.c" - shouldn't be necessary anymore.
Question: Since this module seems very old - Does it still have the task to convert certain attributes? - Haven't we now the ldif handlers who do this for all backends (and not only LDB_LDAP)?
Comment 11 Andrew Bartlett 2009-06-16 06:19:58 UTC
The simple_ldap_map code is indeed old in this respect, it certainly could handle the conversions by looking at the schema.

However, at the moment it isn't written that way, and only ldb_tdb will use the schema code to normalise integers.  Therefore, please remove the simple_ldap_map.c code from the patch.

In syntax_map.c, don't change the .ldap_oid (as it must still be the LDB_SYNTAX_INTEGER), but instead add a .ldb_syntax = LDB_SYNTAX_INT32 element.

It seems you are getting very close.  We should soon be able to apply the patch.

Comment 12 Matthias Dieter Wallnöfer 2009-06-17 14:44:00 UTC
Created attachment 4300 [details]
New patch to handle 32bit integer attributes correctly

- Undo the changes on the parameter ".ldap_oid" and added parameters ".ldb_syntax" in "schema_syntax.c"
- Refactored the conversion function in "simple_ldap_map.c" (it looked a bit ugly in my eyes)
Comment 13 Andrew Bartlett 2009-06-17 23:35:54 UTC
Matthias, can you put that in your git tree, or re-attach as a git-format-patch?  

This version is good!

(otherwise I'll make up a commit message for you)
Comment 14 Andrew Bartlett 2009-06-18 07:54:31 UTC
Also, any chance of an extension to ldap.py to test this behaviour?

Andrew Bartlett
Comment 15 Matthias Dieter Wallnöfer 2009-06-19 01:33:41 UTC
Fixed
Comment 16 Matthias Dieter Wallnöfer 2009-08-12 05:12:02 UTC
I've to reopen the bug since the problem shows up again.
Comment 17 Matthias Dieter Wallnöfer 2009-08-17 06:55:18 UTC
This problem should now be finally fixed. I substituted the "strtol" with "strtoll" since with the you got always LONG_MAX on conversions (due to the intended overflows). This didn't happen earlier when I wrote the first patch - I think my glibc had a bug.