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).

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...

Created attachment 4357 [details]
First version of a patch

(Doesn't work completely right yet)

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 
dcesrv_netr_creds_server_step_check()

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).

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.

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.

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).

Created attachment 4399 [details]
Updated patch

I deactivated for now the "version number" feature since it generates conversion errors.

Created attachment 4411 [details]
Updated patch

This should be the latest patch. All is working like a charm on my test environment!

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

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.

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. 

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 on attachment 4411 [details]
Updated patch

Also here, I provide a link to my patch in my repo (as URL)

Matthias,

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,
					       (ndr_pull_flags_fn_t)ndr_pull_netr_osVersionEx);
if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
	return NT_STATUS_INVALID_PARAMETER;
}				

See the example in dcesrv_lsa_CreateTrustedDomain_base() in source4/rpc_server/lsa/dcesrv_lsa.c

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.

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

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.

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.

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.

Now the patch set should be complete. Please merge and I'll close the bug!

Created attachment 4469 [details]
Patch to make RPC-NETLOGON even try a GetDomainInfo call

Matthias,

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.

Thanks,

Andrew Bartlett

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.

Now I corrected the test to work against Windows and SAMBA and also our implementation should be proper now.

Matthias,

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

Should be fixed in "master".