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

[Andrew Bartlett]

On Mon, 2013-12-09 at 08:41 +0100, Stefan (metze) Metzmacher wrote:
> Hi Garming,
> 
> >>> + 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?

> >> I've fixed the NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE and
> >> NT_STATUS_NOT_IMPLEMENTED
> >> code pathes in netlogon_creds_cli_check_caps(). I also added some
> >> comments.
> >>
> >> I also check result in netlogon_creds_cli_auth_srvauth_done() before the
> >> downgrade check.
> >>
> >> I've updated my
> >> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-schannel-ok
> >>
> >> branch.
> >>
> >> Can you reset with the new code. It would be good to know if
> >> netlogon_creds_cli_ServerPasswordSet
> >> still reports NT_STATUS_INVALID_PARAMETER_MIX.
> >>
> >> metze
> >>
> > 
> > Hi,
> > 
> > I tried out your new code and it joins just fine to the 3.5 domain with
> > the net rpc join command that I used previously.
> 
> Thanks for testing!
> 
> > As for the NT_STATUS_INVALID_PARAMETER_MIX error, I connected with
> > rpcclient to the domain.
> > 
> > I ran this command and the invalid parameter mix error appears:
> > 
> >     sudo bin/rpcclient ncacn_np:ubuntumachine -Uroot%password12# -c
> > change_trust_pw
> > 
> 
> Ok, with rpcclient this could be expected...
> 
> > It seems to come from this piece of code:
> > 
> > function: netlogon_creds_cli_ServerPasswordSet_send in
> > libcli/auth/netlogon_creds_cli.c
> > line number: 1674
> > 
> >   dcerpc_binding_handle_auth_info(state->binding_handle,
> >                     &state->auth_type,
> >                     &state->auth_level);
> > 
> >     switch (state->auth_level) {
> >     case DCERPC_AUTH_LEVEL_INTEGRITY:
> >     case DCERPC_AUTH_LEVEL_PRIVACY:
> >         break;
> >     default:
> >         tevent_req_nterror(req, NT_STATUS_INVALID_PARAMETER_MIX);
> >         return tevent_req_post(req, ev);
> >     }
> 
> Andrew, what do you think should we just remove this check?
> It's not strictly needed, as the password fields are encrypted on their own.
> 
> Maybe we should only do this check if NETLOGON_NEG_AUTHENTICATED_RPC was
> negotiated.

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. 

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