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

[Jeremy Allison]

On Fri, Mar 10, 2017 at 09:48:28AM +0000, Noel Power wrote:

> 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?

Yes, that's much better - thanks !

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.

Whereas any code that uses the standard patterns:

unsigned x = READ_FROM_WIRE;
unsigned y = another value - read from wire or not.

if (x + y < x) {
	integer wrap;
}
if (x + y > max_len) {
	range error;
}
z = x + y;

// z is now safe to use.

is obviously safe right at the site of first use.

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

The checks need to be done right next to the reads,
as well as at any areas embodying higher-level logic.

Until Samba moves to rust or go, we must continuously
be vigilent here, otherwise we'll end up in the next
Wikileaks/CIA collaboration :-) :-).

Cheers,

	Jeremy.