netlogon_creds_cli_validate() in master4-schannel

[Garming Sam]

On 29/11/13 11:12, Andrew Bartlett wrote:
> On Thu, 2013-11-28 at 16:18 +0100, Stefan (metze) Metzmacher wrote:
>> Hi Andrew,
>>
>>> I was looking at this in particular, because it seems to be blocking a
>>> lot of other work.
>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=b96ee68714389abee2f0cc0743246a335818376a
>>>
>>> What I don't understand is why we have this complex
>>> netlogon_creds_cli_validate() routine, rather than something at the NDR
>>> layer using memcmp()?  If we ever add something to this structure, we
>>> are going to have to keep this in sync, and that seems unfortunate.
>> I've changed that.
>>
>>> Other than that, it looks good.  I was supprised by the downgrade
>>> handling in netlogon_creds_cli_check_caps but as long as we are careful
>>> as to when we choose not to propose AES, I guess it's OK.
>>>
>>> How can I help you get the rest of this set merged?
>> Please review/test/push my
>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-base
>> branch. This contains the netlogon secure channel changes which should
>> be ready for master.
>>
>> The rest needs more work. I need to do some more research regarding the
>> password change code and
>> fallback to the old password.
> I've reviewed them:
> http://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/master4-schannel-base
>
> Some non-critical comments:
>   In 's3:libnet_join: make use of rpccli_{create,setup}_netlogon_creds()'
> you have
> + /*
> + * We now have an anonymous connection to IPC$ on the domain password
> server.
> + */
>
> However the connection appears to be authenticated.
>
> I was worried about netlogon_creds_cli_context_copy() but I see you
> remove it in the last patch.  Nice work!
>
> Finally, I'm a little worries that the rpcclient changes don't have a
> testsuite.
>
> We just need to extend the rpcclient blackbox test script.  I'll hack at
> this later today.
>
> It would also be a good idea to run some of our make test with the
> different options etc, and to have a test showing the defence against a
> downgrade attack, given how important that is.
>
> Andrew Bartlett
>
>
>
I did some testing with Andrew and it looks like there is a seg fault in 
rpcclient. Note that it only happens when we call samlogon twice in the 
same session. We'll have a look into it, but you probably don't want to 
push the changes right away.

SELFTEST_TESTENV=s3dc make testenv


garming at garming-pc:~/samba$ valgrind bin/rpcclient ncacn_np:locals3dc2 
-U$USERNAME%$PASSWORD -c "samlogon administrator locals3dc2pass;samlogon 
administrator locals3dc2pass"
==5159== Memcheck, a memory error detector
==5159== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==5159== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==5159== Command: bin/rpcclient ncacn_np:locals3dc2 
-Ugarming%locals3dc2pass -c samlogon\ administrator\ 
locals3dc2pass;samlogon\ administrator\ locals3dc2pass
==5159==
==5159== Invalid read of size 8
==5159==    at 0x6728401: netlogon_creds_cli_LogonSamLogon_send 
(netlogon_creds_cli.c:1971)
==5159==    by 0x67293F8: netlogon_creds_cli_LogonSamLogon 
(netlogon_creds_cli.c:2378)
==5159==    by 0x5590168: rpccli_netlogon_password_logon 
(cli_netlogon.c:258)
==5159==    by 0x1375C6: cmd_netlogon_sam_logon (cmd_netlogon.c:804)
==5159==    by 0x11E339: do_cmd (rpcclient.c:826)
==5159==    by 0x11E55D: process_cmd (rpcclient.c:881)
==5159==    by 0x11F066: main (rpcclient.c:1172)
==5159==  Address 0x20 is not stack'd, malloc'd or (recently) free'd
==5159==
==5159==
==5159== Process terminating with default action of signal 11 (SIGSEGV)
==5159==  Access not within mapped region at address 0x20
==5159==    at 0x6728401: netlogon_creds_cli_LogonSamLogon_send 
(netlogon_creds_cli.c:1971)
==5159==    by 0x67293F8: netlogon_creds_cli_LogonSamLogon 
(netlogon_creds_cli.c:2378)
==5159==    by 0x5590168: rpccli_netlogon_password_logon 
(cli_netlogon.c:258)
==5159==    by 0x1375C6: cmd_netlogon_sam_logon (cmd_netlogon.c:804)
==5159==    by 0x11E339: do_cmd (rpcclient.c:826)
==5159==    by 0x11E55D: process_cmd (rpcclient.c:881)
==5159==    by 0x11F066: main (rpcclient.c:1172)
==5159==  If you believe this happened as a result of a stack
==5159==  overflow in your program's main thread (unlikely but
==5159==  possible), you can try to increase the size of the
==5159==  main thread stack using the --main-stacksize= flag.
==5159==  The main thread stack size used in this run was 16777216.
==5159==
==5159== HEAP SUMMARY:
==5159==     in use at exit: 104,648 bytes in 306 blocks
==5159==   total heap usage: 1,240 allocs, 934 frees, 414,566 bytes 
allocated
==5159==
==5159== LEAK SUMMARY:
==5159==    definitely lost: 180 bytes in 3 blocks
==5159==    indirectly lost: 0 bytes in 0 blocks
==5159==      possibly lost: 95,320 bytes in 171 blocks
==5159==    still reachable: 9,148 bytes in 132 blocks
==5159==         suppressed: 0 bytes in 0 blocks
==5159== Rerun with --leak-check=full to see details of leaked memory
==5159==
==5159== For counts of detected and suppressed errors, rerun with: -v
==5159== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)
Segmentation fault (core dumped)