[PATCH] Re: netlogon_creds_cli_validate() in master4-schannel

Andrew Bartlett abartlet at samba.org
Thu Dec 19 17:17:58 MST 2013


On Fri, 2013-12-20 at 00:46 +0100, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> >>>>>>>>> Thanks! Are you able to do a wintest with this?
> >>>>>>>>>
> >>>>>>>>> I also want to do some tests with windows dcs.
> >>>>>>>>>
> >>>>>>>>> I important thing I want to verify is the behavior of
> >>>>>>>>>
> >>>>>>>>>          invalidate_cm_connection(&domain->conn);
> >>>>>>>>> +       domain->conn.netlogon_force_reauth = true;
> >>>>>>>>>
> >>>>>>>>> in _wbint_CheckMachineAccount() and related code.
> >>>>>>>>>
> >>>>>>>>> Testing against a s4 dc showed that we are doing
> >>>>>>>>> netr_ServerReqChallenge/netr_ServerAuthenticate3 over a connection
> >>>>>>>>> with DCERPC_AUTH_TYPE_SCHANNEL/DCERPC_AUTH_LEVEL_PRIVACY and I'm not
> >>>>>>>>> sure Windows also likes that.
> >>>>>>>>>
> >>>>>>>>> I think some combination of 'wbinfo -t' and 'wbinfo -c' triggered that.
> >>>>>>>>>
> >>>>>>>>> Günther can you also do some tests with your VMs?
> >>>>>>>> I'll get Garming to give this a test against some real Windows VMs, and
> >>>>>>>> yes, this is a very good excuse to get wintest running again.
> >>>>>>>>
> >>>>>>>> Andrew Bartlett
> >>>>>>>>
> >>>>>>>
> >>>>>>> It appears to work just fine on my end.
> >>>>>>
> >>>>>> Against what windows versions did you test?
> >>>>>
> >>>>> Garming tested with 2008R2.
> >>>>>
> >>>>>> I've tested today against a w2012 dc and found that it works.
> >>>>>>
> >>>>>> I just found one bug when using net rpc testjoin, which triggered
> >>>>>> a DCERPC_FAULT_SEC_PKG_ERROR.
> >>>>>> This commit should fix the problem for now:
> >>>>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=88d3b57a7f744c4be39668031717df146eba7e6d
> >>>>>> it's part of
> >>>>>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-ok
> >>>>>> now.
> >>>>>>
> >>>>>> I've done some captures see
> >>>>>> https://www.samba.org/~metze/ads/caps/netlogon/v4-0-schannel/20131213/
> >>>>>>
> >>>>>> I'll try to do some more testing on monday.
> >>>>
> >>>> I've also tested with Windows 2008 and will do with nt4 and windows 2000
> >>>> and some samba versions.
> >>>>
> >>>> I have some updates in my
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-ok
> >>>> branch.
> >>>>
> >>>> While testing with winbind sealed pipes = no, I noticed that we send the
> >>>> same Authenticator again and again to a dc that returns NOT_IMPLEMENTED
> >>>> to LogonGetCapabilities(). As this is the first request on each schannel
> >>>> connection,
> >>>> I think it's better to avoid this, as the session key is much more long
> >>>> living now.
> >>>>
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=fa68a5814d7ad3fb48b22eaaad1bdb0ed2fc495c
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=5df6c619f5670b71e04ab047a2d6f12073d376dc
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=485ed1950affa3b9da0d78dc927c4185b2111e8c
> >>>>
> >>>> are the cleanup ups for this.
> >>>>
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=23896aefe5f50ba977167a85b1b6189dd65d03f0
> >>>> got netlogon_creds_cli_open_global_db()
> >>>> which is used in
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=82e902bad329a0734ab2b4c1436f53c440cca4ef
> >>>> which is used in
> >>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=46949116273667b65d7ac59f1d1a11ec9284f963
> >>>>
> >>>> This makes sure that the winbind parent opens the netlogon_creds_cli.tdb
> >>>> and it doesn't get cleared
> >>>> if a child was killed and a new one was started. This way we only do a
> >>>> ServerChallenge/ServerAuthenticate
> >>>> pair when winbindd is restarted or the dc gets restarted.
> >>>
> >>> I've looked over the changes individually, and the diff between what I
> >>> last reviewed and your current tree.  On that basis:
> >>>
> >>> Reviewed-by: Andrew Bartlett <abartlet at samba.org>
> >>>
> >>> Thanks for all your great work here!
> >>
> >> After testing with NT4 4.0 SP6a I have some little changes.
> >>
> >> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=e20b593ce69298f1dc7ec0ac126f865173d4f3c9
> >>
> >> Here I changed the comment, when getting NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE
> >> in netlogon_creds_cli_check_caps(), NT 4.0 also returns that.
> > 
> > I'm a bit worried about accepting that.  This is the one error that can
> > be trivially faked by an attacker.  As you noticed we can do an auth2
> > over SCHANNEL, could we use that to verify the flags instead in this
> > case?
> 
> If we get NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE, it means we don't
> increment the schannel_state->seq_num. Which means the server would detect
> an attacker with the next request. And we as client would also detect it
> in the next response.

Hmm.  I'm cautious about this given Microsoft said they specifically
needed this to avoid an unauthenticated reply.  

> So we could also remove this block:
> 
>                 if (tmp & NETLOGON_NEG_SUPPORTS_AES) {
>                         /*
>                          * NT 4.0 and Old Samba servers need
>                          * "disable aes schannel = yes"
>                          */
>                         status = NT_STATUS_DOWNGRADE_DETECTED;
>                         tevent_req_nterror(req, status);
>                         netlogon_creds_cli_check_cleanup(req, status);
>                         return;
>                 }
> 
> And maybe also the "disable aes schannel" option completely.
> 
> What do you think?

Either way, I would like "disable aes schannel" removed, if only because
it seems to me to be a very confusing option.  

I also think that by the time Samba 4.2 is released, I really think that
Samba DCs before bug #6099 would be very, very rare.  Not only are they
beyond security support, anyone who added even a trial Windows 7 desktop
would have hit the issues and found it easier to upgrade the servers
rather than face setting registry keys across the whole site.  

I understand wanting to be able to control this for testing, but for
user experience can we safely roll this in to "require strong keys = no"
somehow?

> >> I tested with this settings:
> >>
> >>         # NT 4.0 sp6a
> >>         security = domain
> >>         workgroup = NT4DOM
> >>         require strong key = no
> >>         disable aes schannel = yes
> >>         client NTLMv2 auth = no
> >>
> >> I also tested if "client schannel = no" still works.
> >> I had to change netlogon_creds_cli_auth_send() and
> >> we always start with try_auth3 and try_auth2, we also
> >> set require_auth2 if require any of NETLOGON_NEG_ARCFOUR,
> >> NETLOGON_NEG_STRONG_KEYS, NETLOGON_NEG_PASSWORD_SET2,
> >> NETLOGON_NEG_SUPPORTS_AES. I may add NETLOGON_NEG_AUTHENTICATED_RPC
> >> after testing against 3.0.37, it seems that NT 4.0 and Samba 3.0.37 both
> >> support Authenticate2.
> > 
> > Yes, NT 4.0 and 3.0.37 both support Authenticate2, it's been in the
> > protocol as long as I have been involved in Samba. 
> 
> Ok, then I think I'll require auth2 and add
> NETLOGON_NEG_AUTHENTICATED_RPC to
> the required flags unless we have "client schannel = no".

That seems reasonable. 

> Later we could think about removing this parameter completely.
> 
> >> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=9447e1d665991af78d71223ca099a9a8a49d88ce
> >> Here I explained where "disable aes schannel = yes" might be needed.
> > 
> > I don't want to get in the way of this getting into master, but this
> > explanation makes me feel like the parameter is poorly named.  
> 
> It was modeled to behave like a windows client that doesn't support aes.
> As written above we might be able to remove it.
> 
> >> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=c78e78cea8b9f25e79407c12a60410e755d05f58
> >> Here I fixed the flags we require with "reject md5 servers = yes".
> >>
> >> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=371fa99fd1ae88898839d0c4a26bd5a3ae4e0a39
> >> Here I fixed the flags we require with "require strong key = yes" (the
> >> default)
> >> and that "require strong key = no" is needed for Samba < 3.5.
> > 
> > I think you pasted the wrong link here. 
> 
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=35fa79d6873fb20fa78e75c4a6566f46cb9a3a32
> 
> > Do we really need all of "disable aes schannel", "require strong key"
> > and "reject md5 servers", and can we rename these to have some link to
> > the NETLOGON pipe that they apply to?  As they are, users will set them
> > randomly without really understanding them. 
> 
> I can add a "netlogon" prefix to them, but I think we need all of them.

The prefix would be very helpful, and was what I imagined. 

Thanks!

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba






More information about the samba-technical mailing list