RFC smbd smb2 quota support (+ patches to get rid of manual marshall/unmarshall by ndr-ising the existing code)

[Noel Power]

Hi Jeremy
On 09/03/17 21:46, Jeremy Allison wrote:
[...]
> OK, sorry - didn't notice this from:
>
> [PATCH 3/9] librpc/idl Add some query [getset]info quota related structures
> :
>
> +interface smb2_query_quoata
> +{
> +       /* MS-SMB2 2.2.37.1 */
> +       typedef [public] struct {
> +               uint8 return_single;
> +               uint8 restart_scan;
> +               uint16 reserved;
> +               uint32 sid_list_length;
> +               uint32 start_sid_length;
> +               uint32 start_sid_offset;
> +               uint8 sid_buf[sid_list_length ? sid_list_length : start_sid_length + start_sid_offset];
> +       } smb2_query_quota_info;
> +}
>
> You can't do arithmetic like 'start_sid_length + start_sid_offset'
> on structures read from the wire. That ends up with auto-generated
> code:
>
>                 size_sid_buf_0 = r->sid_list_length?r->sid_list_length:r->start_sid_length + r->start_sid_offset;
>                 NDR_PULL_ALLOC_N(ndr, r->sid_buf, size_sid_buf_0);
not sure I understand why this is a problem,  if we get bogus values for
sid_list_length, start_sid_length or start_sid_offset, there are iiuc 2
outcomes
   a) we try to read an array of bytes into buf that is less than the
remaining bytes on the wire
   b) we try to read a too large array of bytes into buf that is greater
than the remaining bytes on the wire

a) should be handled by ndr errors when we later try to pull structures
from the short buffer.
b) should directly result in an error (at the ndr level) when pulling
this struct

Now I am not saying this is good but I don't really see how it is
different from

struct foo {
    uint32 length;
   uint8 buf[length]
}

if you craft a bogus length.  Or is it you are more concerned about
trying to detect the possible wrap with the 'start_sid_length +
start_sid_offset' (we could also check that after the fact in the code
that uses the structure)

I'm not trying to be awkward or saying I'm not going to change anything
(actually the opposite, I'm happy to make any required changes) but I do
want to understand why and I'd like to know what type of changes best
chance of 'making it' :-)

Now in the case of 'smb2_query_quota_info' attempting to pull 'buf' is
really only a convenience, you have to do it here or directly on the
remainder of the buffer later in the code. One thing I don't like is
that the length of 'buf' is calculated twice, once here in the idl and
then later in the the code that tries to extract more stuff from 'buf'
So, actually having buf doesn't give you much other than having all the
need pieces in one struct.

How about if I did something like

modify the existing structure

       typedef [public] struct {
               uint8 return_single;
               uint8 restart_scan;
               uint16 reserved;
               uint32 sid_list_length;
               uint32 start_sid_length;
               uint32 start_sid_offset;
              /*
                * remaining bytes represent the sid or sid_list
                * buffer.
                */
    }

and in code limit/range check (as much as that is possible) the
calculation of the remaining bytes in the buffer to construct the
DATA_BLOB we need to read the next structures from.

Is that approach acceptable? or is there some other way to force the
idl/generated code to to what is required that I don't know about?

> which is a pushover for an attacker.
> Hint: what if start_sid_offset = start_sid_offset = 0xFFFFFFFF ?
I don't get it :-( afaics whatever numbers (fake or otherwise) you pump
into sid_list_length, start_sid_length or start_sid_offset you are going
to end up with an effective array len of 0 -> 4294967295, if it is too
big the ndr code will detect that, if it is too small then we will find
out about that later.
>
> *Everything* going to/from the wire must be null, range
> and wrap checked if any arithmetic is done on it.
>
> Sorry for being a hardass on this stuff, but what you're
> writing here is the most security-sensitive code that
> can possibly be in Samba, and I know from *bitter* experience
> (i.e. I've personally screwed this up many, many, times)
> that this stuff must be perfect.
>
> Sorry,
np :-) thanks in advance for your patience with my questions above

Noel