DCERPC Security Context Multiplexing (ready for review)

[Jeremy Allison]

On Fri, Dec 21, 2018 at 02:35:14PM -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:-)
> 
> First comment - in [PATCH 033/134] librpc: add dcerpc_get_auth_length() helper function
> you have:
> 
> +uint16_t dcerpc_get_auth_length(const DATA_BLOB *blob)
> +{
> +       if (CVAL(blob->data,DCERPC_DREP_OFFSET) & DCERPC_DREP_LE) {
> +               return SVAL(blob->data, DCERPC_AUTH_LEN_OFFSET);
> +       } else {
> +               return RSVAL(blob->data, DCERPC_AUTH_LEN_OFFSET);
> +       }
> +}
> 
> As DATA_BLOB has a length already included, for safety's sake
> can you check it's larger than DCERPC_AUTH_LEN_OFFSET (which
> is 10, DCERPC_DREP_OFFSET is 4) plus the reading size before indirecting.
> 
> I'm sure you already check this elsewhere :-), but adding it
> inside this function makes it self-sufficient and obviously safe.
> 
> Something like:
> 
> NTSTATUS dcerpc_get_auth_length(const DATA_BLOB *blob, uint16_t *plen)
> {
> 	if (blob->length < DCERPC_DREP_OFFSET+1 ||
> 			blob->length < DCERPC_AUTH_LEN_OFFSET+2) {
> 		return NT_STATUS_INVALID_PARAMETER;
> 	}
> 	if (CVAL(blob->data,DCERPC_DREP_OFFSET) & DCERPC_DREP_LE) {
> 		*plen = SVAL(blob->data, DCERPC_AUTH_LEN_OFFSET);
> 	} else {
> 		*plen = RSVAL(blob->data, DCERPC_AUTH_LEN_OFFSET);
> 	}
> 	return NT_STATUS_OK;
> }
> 
> Yeah I know it makes it more of a pain to use elsewhere, sorry :-(.

One more point, dcerpc_get_auth_length() isn't static, so fixing
it to be DATA_BLOB offset safe will protect it from accidently
getting called from elsewhere later on.

If you want, just make it return 0 on error rather than rippling
the NTSTATUS up through all the callers, which is what you're
doing in the later patches.

Although changing the following:

[PATCH 034/134] librpc: add dcerpc_get_auth_{type,level,context_id}() helper functions

patch to return NTSTATUS back up the stack rather than 0 on
length errors might be better ?