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

[Andrew Bartlett]

On Tue, 2013-12-10 at 14:47 +0100, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> >>>>> + sudo bin/net rpc join -S 192.168.122.249 -Uroot%password12#
> >>>>> No realm has been specified! Do you really want to join an Active
> >>>>> Directory server?
> >>>>> netlogon_creds_cli_ServerPasswordSet failed:
> >>>>> NT_STATUS_INVALID_PARAMETER_MIX
> >>>>> No realm has been specified! Do you really want to join an Active
> >>>>> Directory server?
> >>>>> netlogon_creds_cli_check failed with NT_STATUS_NOT_IMPLEMENTED
> >>>>> libnet_join_ok: failed to open schannel session on netlogon pipe to
> >>>>> server 192.168.122.249 for domain S3. Error was
> >>>>> NT_STATUS_NOT_IMPLEMENTED
> >>>>> Failed to join domain: failed to verify domain membership after joining:
> >>>>> Not implemented
> >>
> >> So the above was not the 'bin/net rpc join -S 192.168.122.249
> >> -Uroot%password12#' output?
> > 
> > We couldn't find it in that output, and couldn't find it in the previous
> > output.  Can you remind us where your original question came from, so we
> > can re-investigate?
> 
> Just the above lines.

Ah, I just re-read the thread, and we saw it in the bisect of the net
rpc join code then.  We don't see that now. 

> > 
> > I take it that's the bit for schannel?  I guess that's reasonable, but
> > what situations would we not negotiate that?
> > 
> >> The reason I added this was to prevent downgrade attacks where we would call
> >> netlogon_creds_cli_ServerPasswordSet() without
> >> netlogon_creds_cli_check() before
> >> and use dcerpc_netr_ServerPasswordSet() with just des encryption.
> > 
> > Hmm.  We really should do whatever we can to avoid using DES for this.  
> > 
> > As Samba 3.5 and above implement ServerPasswordSet2, why are we calling
> > ServerPasswordSet at all?
> > 
> >> We also have this in netlogon_creds_cli_ServerPasswordSet_locked:
> >>
> >>         if (state->auth_type != DCERPC_AUTH_TYPE_SCHANNEL) {
> >>                 uint32_t tmp = state->creds->negotiate_flags;
> >>
> >>                 if (tmp & NETLOGON_NEG_AUTHENTICATED_RPC) {
> >>                         /*
> >>                          * if DCERPC_AUTH_TYPE_SCHANNEL is supported
> >>                          * it should be used, which means
> >>                          * we had a chance to verify no downgrade
> >>                          * happened.
> >>                          *
> >>                          * This relies on netlogon_creds_cli_check*
> >>                          * being called before, as first request after
> >>                          * the DCERPC bind.
> >>                          */
> >>                         tevent_req_nterror(req,
> >> NT_STATUS_INVALID_PARAMETER_MIX);
> >>                         return;
> >>                 }
> >>         }
> >>
> >> Maybe we should combine both tests. And also
> >> add NETLOGON_NEG_PASSWORD_SET2 with if (require_strong_key)
> >> as NETLOGON_NEG_STRONG_KEYS and NETLOGON_NEG_PASSWORD_SET2 were both
> >> added with Windows 2000.
> > 
> > That sounds reasonable.  I don't want to break old Samba 3.x domains,
> > but at the same time, I really think we must start asserting a
> > requirement for modern crypto, particularly when we talk to an AD DC.
> > 
> > Samba 3.5 seems a reasonable minimum, earlier versions are long out of
> > our security support anyway. 
> 
> This changes add the "require strong key" option and we set the default
> to "yes",
> which means we have this:
> 
>         if (require_strong_key) {
>                 required_flags |= NETLOGON_NEG_ARCFOUR;
>                 required_flags |= NETLOGON_NEG_STRONG_KEYS;
>                 required_flags |= NETLOGON_NEG_PASSWORD_SET2;
>                 required_flags |= NETLOGON_NEG_AUTHENTICATED_RPC;
>         }
> 
> which should avoid des as much as possible.

Great, thanks. 

> I've updated my
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-ok
> 
> I now require NETLOGON_NEG_PASSWORD_SET2 with require_strong_key.
> and we also require NETLOGON_NEG_ARCFOUR unless we don't propose
> NETLOGON_NEG_AUTHENTICATED_RPC.
> 
> I've also added "allow nt4 crypto" (default: no) and "reject md5
> clients" (default: no)
> as options for the AD netlogon server.

I'll look over the changes today, and hopefully be able to give you my
review.  Do you want me to push if it's all OK?

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