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

[Noel Power]

Hi Jeremy
On 10/03/17 16:50, Jeremy Allison wrote:
> On Fri, Mar 10, 2017 at 09:48:28AM +0000, Noel Power wrote:
[...]
> 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?
> Yes, that's much better - thanks !
ok
>
> It's not so much about the specific attack that can or
> can't be done. It's about the massive danger of doing
> *any* unchecked arithmetic on values read from the wire.
>
> From talking to Microsoft engineers in the past I believe
> that internal to Microsoft code can be flagged as dealing
> with wire values, and then *any* attempt to do "raw" C
> arithmetic on it is marked as a compiler error.
>
> So when I saw what the ndr-generated code was doing a
> big red *WARNING* light started flashing. Yes, we may
> or may not check it later. But anyone quickly looking
> at the code CAN'T TELL if this is safe or not. And that's
> an invitation for attackers to start probing there (I
> would). That means you need to spend time and effort
> proving that this is safe.
understood :-) thanks for the explanation(s)
>
> Whereas any code that uses the standard patterns:
[...]
>> 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.
> I'm sure you're right, and I'll believe you. I still
> don't want to see *ANY* unchecked arithmetic in values
> read from the wire.
that's fine, now I get where you are coming from, I will post new
patches as soon as I rework the series

Noel