Bug 4888 - ADUC: Missing computer informations
Summary: ADUC: Missing computer informations
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: unspecified
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Matthias Dieter Wallnöfer
URL: http://repo.or.cz/w/Samba/mdw.git?a=l...
Depends on:
Reported: 2007-08-15 16:33 UTC by Matthias Dieter Wallnöfer
Modified: 2009-08-03 02:50 UTC (History)
1 user (show)

See Also:

First version of a patch (161.98 KB, patch)
2009-06-26 09:26 UTC, Matthias Dieter Wallnöfer
no flags Details
Updated patch (161.63 KB, patch)
2009-06-30 07:18 UTC, Matthias Dieter Wallnöfer
no flags Details
Updated patch (161.61 KB, patch)
2009-06-30 09:07 UTC, Matthias Dieter Wallnöfer
no flags Details
Updated patch (159.84 KB, patch)
2009-07-01 09:19 UTC, Matthias Dieter Wallnöfer
no flags Details
Updated patch (159.84 KB, patch)
2009-07-07 03:23 UTC, Matthias Dieter Wallnöfer
no flags Details
Updated patch (162.24 KB, patch)
2009-07-10 15:26 UTC, Matthias Dieter Wallnöfer
no flags Details
Allow parsing of GetDomainInfo blob (2.17 KB, patch)
2009-07-22 02:07 UTC, Andrew Bartlett
no flags Details
The packet for use with ndrdump to verify this IDL (680 bytes, application/octet-stream)
2009-07-22 02:09 UTC, Andrew Bartlett
no flags Details
(not finished) example of how we might test the call (2.30 KB, patch)
2009-07-22 02:30 UTC, Andrew Bartlett
no flags Details
Patch to make RPC-NETLOGON even try a GetDomainInfo call (2.60 KB, patch)
2009-07-23 19:29 UTC, Andrew Bartlett
no flags Details
Fix for IDL (1.41 KB, patch)
2009-07-24 04:09 UTC, Andrew Bartlett
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Dieter Wallnöfer 2007-08-15 16:33:03 UTC
On my test environment I encountered the following: When I open a object of a joined workstation in ADUC, the informations about the OS type and version are missing. But when I look at the domain controller object (SAMBA 4), the information is displayed in the right way. Can you reproduce this behaviour?
Comment 1 Andrew Bartlett 2007-08-15 18:48:35 UTC
This information is supplied by the client in one of the NETLOGON calls.  A compromised client could indeed call itself 'Microsoft Mangler for Users' should it so desire...

We should however handle this correctly (perhaps even the NT4 fallback).
Comment 2 Andrew Bartlett 2008-01-08 21:19:13 UTC
The call involved is netr_LogonGetDomainInfo - which does far more than it you might imagine.  I've started to look at what would be involved in filling this in right...
Comment 3 Matthias Dieter Wallnöfer 2009-06-26 09:26:40 UTC
Created attachment 4357 [details]
First version of a patch

(Doesn't work completely right yet)
Comment 4 Andrew Bartlett 2009-06-29 22:31:46 UTC
This looks really good.  

The big thing you need to be careful about is not to allow any workstation to change the details of any other workstation.  Instead, you must access the authenticated workstation account name or SID.

You get this by supplying the final parameter to 

In struct netlogon_creds_CredentialState you will see account_name and sid.

You can even use the SID as the DN:

dn = ldb_dn_new_fmt(mem_ctx, ldb, "<SID=%s>", dom_sid_string(creds->sid))

This avoids you having to handle an additional search to find the DN.

I hope this helps!

Andrew Bartlett

(Also, in future please omit the autogenerated files form the patch).
Comment 5 Matthias Dieter Wallnöfer 2009-06-30 07:18:07 UTC
Created attachment 4375 [details]
Updated patch

Contains the improvements advised by Andrew.
I have to wait until I can test it (since my ADUC doesn't work against SAMBA 4 at the moment) and eventually write a testcase.
I let the autogenerated files in since the SAMBA 3 people prefer them for certain reasons (no need for PIDL on build machine...). But on the final patch I can remove them.
Comment 6 Matthias Dieter Wallnöfer 2009-06-30 09:07:44 UTC
Created attachment 4376 [details]
Updated patch

Another patch.
Slightly improved since it uses the samdb wrapper functions when they exist as alternatives to the default LDB calls.
Comment 7 Matthias Dieter Wallnöfer 2009-07-01 09:19:35 UTC
Created attachment 4383 [details]
Updated patch

A much better version of the patch. Now we have the DNS domainname and the operating system name handled correctly in the directory.
What's missing are the operating system version and service pack level. I'm not able to read them correctly out from the netr_LogonGetDomainInformations call and checked also the IDL file from the WSPP documentation.
I realised that the NETLOGON WSPP documentation is a bit inaccurate at this points ("netr_LogonGetDomainInformations" call and IDL types for call).
Comment 8 Matthias Dieter Wallnöfer 2009-07-07 03:23:37 UTC
Created attachment 4399 [details]
Updated patch

I deactivated for now the "version number" feature since it generates conversion errors.
Comment 9 Matthias Dieter Wallnöfer 2009-07-10 15:26:05 UTC
Created attachment 4411 [details]
Updated patch

This should be the latest patch. All is working like a charm on my test environment!
Comment 10 Andrew Bartlett 2009-07-10 18:53:59 UTC
Sorry Matthias, but you can't cast a network structure like that.

Instead, you have to use the NDR functions to pull and push it (otherwise, you might not get a full structure from the wire, and nothing will convert it from little-endian).  You should declare it in the IDL, then place it into the structure with [subcontext].  

Andrew Bartlett
Comment 11 Matthias Dieter Wallnöfer 2009-07-12 15:15:33 UTC
Andrew, but the "OsVersionInfoEx" is the internal structure of the content of a "Lsa_BinaryString". So the concerns "might not get the full content" shouldn't be true: either we get the full (content of) "Lsa_BinaryString" or we get nothing, am I right?
Okay, maybe you don't see the most elegant solution taken in my approach (since I simply cast). So to do it properly in your point of view I had to add a new wrapper type insted of "Lsa_BinaryString" in the middle of both ("Lsa_CustomBinaryString"?) which contains the reference to the "OsVersionInfoEx" structure and itself is a member of the "WorkstationInfo" structure. Then the cast would be superfluous.
Comment 12 Andrew Bartlett 2009-07-12 17:18:20 UTC
Yes, you get the full LsaBinaryString, or nothing.  But the LsaBinaryString is variable length, so you could get half an OsVersionInfoEx, or complete garbage.

By using [subcontext] you can tell the IDL compiler that 'this looks like a blob, but really there is another structure in here, you need to parse'.  

Sadly I've not found an excellent example of how to use [subcontext], so you will have to talk to jelmer or metze for details. 
Comment 13 Matthias Dieter Wallnöfer 2009-07-13 01:01:17 UTC
So I should simply put [subcontext] in front of "Lsa_BinaryString osVersion" in the workstation structure?
Please enlighten me a bit more! (Or give a concrete example - I'm definitely lost since I'm not a specialist in IDL yet).
Comment 14 Matthias Dieter Wallnöfer 2009-07-17 09:23:30 UTC
Comment on attachment 4411 [details]
Updated patch

Also here, I provide a link to my patch in my repo (as URL)
Comment 15 Andrew Bartlett 2009-07-20 07:07:57 UTC

We need to change the cast:
+               os_version = (struct netr_OsVersionInfoEx *)
+                       r->in.query->workstation_info->os_version.array;
to ndr_pull_struct_blob. eg:

os_version_blob.data = r->in.query->workstation_info->os_version.array
os_version_blob.length = r->in.query->workstation_info->os_version.len

ndr_err = ndr_pull_struct_blob(&os_version_blob, mem_ctx, lp_iconv_convenience(dce_call->conn->dce_ctx->lp_ctx), &os_version,
if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {

See the example in dcesrv_lsa_CreateTrustedDomain_base() in source4/rpc_server/lsa/dcesrv_lsa.c
Comment 16 Matthias Dieter Wallnöfer 2009-07-20 16:07:36 UTC
I prepared a new patch which should work. However I was not able to suppress a warning on the "ndr_pull_struct_blob" call. I tried with casts but had no luck.
Comment 17 Andrew Bartlett 2009-07-20 21:26:08 UTC
The problem is with the assignment of .array in the 'string' to .data in the DATA_BLOB.  

But you can't just add a cast - we need to somehow tell pidl to always marshal this in little endian (even on big endian hosts).  Then ndr_pull_struct_blob() can safely know what byte order to read the rest in (it assumes little-endian by default).

This worked for you because your workstation (like most desktops) is little endian.

I'll seen answers from a PIDL guru.

Andrew Bartlett
Comment 18 Andrew Bartlett 2009-07-22 02:07:46 UTC
Created attachment 4452 [details]
Allow parsing of GetDomainInfo blob

I've started looking into this seriously, and I have a few comments:

Firstly, please remove everything from this patch other than that changes you need to make.  Reformatting, changing of [ref] and anything else can go in a different patch, to be considered and discussed.

Similarly, do *not* include the changes to the generated output.  I'll happily re-run 'make idl_full' when the code is actually committed.

Once this is done, start looking at how you would test this.  Firstly make Samba4 build with the changed IDL (it does not build with your changes).

Then modify torture/rpc/netlogon.c to send a version, and then use ldb_search() against the same server to see if the version changed correctly.  Make sure it's all still correct with run in bigendian mode (ie, ncacn_np:target[bigendian]

To help you along the way, the attached IDL patch parses the blob we send to Microsoft.
Comment 19 Andrew Bartlett 2009-07-22 02:09:39 UTC
Created attachment 4453 [details]
The packet for use with ndrdump to verify this IDL

Run this command to verify that the IDL in your source tree is correct:
make bin/ndrdump && bin/ndrdump netlogon 29 in /tmp/netlogon-29.0.in 

If if fails, see how far it gets to in the structure (valgrind or gdb may help), and start debugging there.
Comment 20 Andrew Bartlett 2009-07-22 02:30:21 UTC
Created attachment 4454 [details]
(not finished) example of how we might test the call

This patch does not even compile, but should give you a few hints about how to modify smbtorture to test this call.  You would also need to check the changes over LDAP using LDB.
Comment 21 Matthias Dieter Wallnöfer 2009-07-23 15:17:04 UTC
Now the patch set should be complete. Please merge and I'll close the bug!
Comment 22 Andrew Bartlett 2009-07-23 19:29:02 UTC
Created attachment 4469 [details]
Patch to make RPC-NETLOGON even try a GetDomainInfo call


Did you actually test this against Windows?  I tried to run this - in particular I wanted to verify your claims about it updating the servicePrincipalNames.  

The attached patch makes the test actually start (using the invocation that Matthieu used in his sorting test), but shows that we send a packet not accepted by Windows 2008.  (That is the 'write fault' error). 

Naturally, I'll need you to fix this before the patch and test can be accepted.


Andrew Bartlett
Comment 23 Andrew Bartlett 2009-07-24 04:09:42 UTC
Created attachment 4470 [details]
Fix for IDL

Once this patch is applied, on top of the others, it is clear that the  dnsHostName is not updated by this call.  I suspect that neither is the servicePrincipalName attribute.

I'm sorry I supplied the incorrect IDL before, I certainly did not and should not expect you to be able to fix this level of 'magic'.

I hope this makes the rest of your work easier - we should remove the changes to dnsHostName and servicePrincipalName from the server, but rewrite the torture test to assert that they are *not* changed by this call.
Comment 24 Matthias Dieter Wallnöfer 2009-07-24 09:02:04 UTC
Now I corrected the test to work against Windows and SAMBA and also our implementation should be proper now.
Comment 25 Andrew Bartlett 2009-07-26 17:55:51 UTC

I'm sorry, but you haven't actually finished the testing, so it's not possible to say that you have finished the tests, or the patch.

A good smbtorture test should prove that all things that are expected to change do change, and that all things that must not change stay the same.

For example, you suggest that this call updates dnsHostName.  I suggest it does not.  The way to resolve this is to have the test check dnsHostName before we run GetDomainInfo, set it to a new value, and read it again after.  

Then do the same for the servicePrincipalName attribute.

Make sure that you have a test for every action you propose to take in the NetLogon server, as well as any other action that the docs suggest, or appear likely.  Once you figure out what windows does, hardcode that into the test (ie, make the test for dnsHostName negative - ie that this call does not change it). 

Only once you do that, and you personally verify that the test passes against your Win2k3 or Win2k8 DC, will you have a finished test.  (Note that other aspects of the test seem to fail against Win2k8 at the moment, so do a 'diff' of the output to ensure that only the previous failures still exist).

I hope this all makes sense,

Andrew Bartlett

Comment 26 Matthias Dieter Wallnöfer 2009-08-03 02:50:01 UTC
Should be fixed in "master".