netlogon_creds_cli_validate() in master4-schannel

[Garming Sam]

On 29/11/13 13:33, Garming Sam wrote:
> 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)
>
>
>

The seg fault was because it couldn't fetch the secret. This patch 
prints an error in this case and fixes the seg fault by reinitializing 
the pointer.



Garming Sam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-rpcclient-give-errors-and-clean-up-correctly-after-f.patch
Type: text/x-patch
Size: 2023 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20131129/f6f6608e/attachment.bin>