DCERPC Security Context Multiplexing (ready for review)

[Jeremy Allison]

On Sun, Dec 23, 2018 at 02:19:11PM +0100, Stefan Metzmacher wrote:
> Hi Jeremy,
> 
> thanks for starting the review!
> 
> >> 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.
> 
> This is more or less a copy from dcerpc_get_frag_length()
> and as all functions in librpc/rpc/dcerpc_util.c they rely
> on the blob having at least DCERPC_NCACN_PAYLOAD_OFFSET (16)
> bytes. We always go (directly or indirectly) through
> dcerpc_pull_ncacn_packet() or explicitly check for
> DCERPC_NCACN_PAYLOAD_OFFSET (or RPC_HEADER_LEN which is the same).

We do *now*, but cut-and-paste has a way of
separating such logic from it's original intentions :-).

> If you want we can add
> SMB_ASSERT(blob->length >= DCERPC_NCACN_PAYLOAD_OFFSET);
> at the beginning of each function (as a follow up patch),

Yep, that would work - thanks ! Can you do that or
do you want me to ?

Jeremy.