DCERPC Security Context Multiplexing (ready for review)

[Stefan Metzmacher]

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).

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),

> 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 :-(.

I'd keep the changes minimal and don't change them to return NTSTATUS.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181223/1c850bb2/signature.sig>