DCERPC Security Context Multiplexing (ready for review)

[Jeremy Allison]

On Thu, Jan 10, 2019 at 03:45:10PM -0800, Jeremy Allison via samba-technical wrote:
> On Wed, Jan 09, 2019 at 12:46:31PM -0800, Jeremy Allison via samba-technical wrote:
> > On Thu, Dec 20, 2018 at 12:53:52AM +0100, Stefan Metzmacher wrote:
> > > I've fixed the missing settimeout in the pylibsmb bindings and fixed the
> > > commit messages.
> > > 
> > > This is ready for review now.
> > > 
> > > I'm not attaching the patches here as they're over 500kb in size, which
> > > is not allowed on the list.
> > > 
> > > They can be downloaded here:
> > > https://gitlab.com/samba-team/samba/merge_requests/173.patch
> > > 
> > > >> Please review and comment:-)
> > > 
> > > Please review and push:-)
> > 
> > I'm reviewing the remaining this week, sorry for the delay
> > over the holidays !
> 
> One quick comment. In [PATCH 044/103] s4:rpc_server/lsa: make use of dcesrv_call_auth_info()
> 
> you're removing the check for auth->auth_level < DCERPC_AUTH_LEVEL_INTEGRITY,
> so this patch also needs the commit message to have the text:
> 
> "It's enough to check the auth_type for DCERPC_AUTH_TYPE_SCHANNEL,
> there's no need to also check the auth_level for integrity or privacy.
> 
> The gensec layer already required at least DCERPC_AUTH_LEVEL_INTEGRITY,
> see schannel_update_internal()."
> 
> added to it, as [PATCH 045/103] already does.
> 
> Just wanted to let you know I'm paying attention :-). FYI, I've
> added that text into the commit message of my local copy and
> when finished review will push/resend to the list.

One more. In [PATCH 064/103] s4:rpc_server: add dcesrv_iface_state_{store,find}_{assoc,conn}() helpers

you have:

+static NTSTATUS dcesrv_iface_state_store(struct dcesrv_assoc_group *assoc,
+                               const struct dcesrv_interface *iface,
+                               const struct dom_sid *owner,
+                               const struct dcesrv_connection *conn,
+                               const struct dcesrv_auth *auth,
+                               const struct dcesrv_connection_context *pres,
+                               uint64_t magic,
+                               TALLOC_CTX *mem_ctx,
+                               void *ptr,
+                               const char *location)
+{
+       struct dcesrv_iface_state *istate = NULL;
+       void *optr = NULL;
+
+       optr = dcesrv_iface_state_find(assoc,
+                                      iface,
+                                      owner,
+                                      conn,
+                                      auth,
+                                      pres,
+                                      magic,
+                                      ptr);
+       if (optr != NULL) {
+               return NT_STATUS_OBJECTID_EXISTS;
+       }
+
+       istate = talloc(ptr, struct dcesrv_iface_state);

                 ^^^^^^
                 I would prefer talloc_zero() here.

Yes I know you fill in all the elements (including
linked list pointers) below, but using talloc_zero
when allocating such things seems safer defensive
programming to me.

I've changed my local copy.

Jeremy

+       if (istate == NULL) {
+               return NT_STATUS_NO_MEMORY;
+       }
+
+       *istate = (struct dcesrv_iface_state) {
+               .assoc = assoc,
+               .iface = iface,
+               .owner = *owner,
+               .conn  = conn,
+               .auth  = auth,
+               .pres  = pres,
+               .magic = magic,
+               .location = location,
+       };
+
+       istate->ptr = talloc_steal(mem_ctx, ptr);
+
+       talloc_set_destructor(istate, dcesrv_iface_state_destructor);
+
+       DLIST_ADD_END(assoc->iface_states, istate);
+
+       return NT_STATUS_OK;
+}