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

Jeremy Allison jra at samba.org
Thu Mar 9 21:46:40 UTC 2017


On Wed, Mar 08, 2017 at 09:30:51AM +0000, Noel Power wrote:
> On 08/03/17 07:59, Noel Power wrote:
> > On 07/03/17 23:41, Jeremy Allison wrote:
> [...]
> >> offset is pulled out of the ndr encoding using
> >> ndr_pull_file_get_quota_info() as info.next_entry_offset.
> >>
> >> Consider what can happen if someone hand crafts a packet
> >> containng a info.next_entry_offset that loops back to
> >> the previous entry.
> >>
> >> I think this should be something like:
> >>
> >> 		new_offset = info.next_entry_offset;
> >> 		if (new_offset <= offset) {
> >> 			DBG_ERR("bad offset (old=0x%x, new=0x%x)\n",
> >> 				(unsigned int)offset,
> >> 				(unsigned int)new_offset);
> >> 			err = NDR_ERR_ARRAY_SIZE;
> >> 			goto done;
> >> 		}
> >> 		offset = new_offset;
> >> 		can_pull = (offset > 0);
> >>
> >> to prevent that. Do you agree ?
> > sure :-) I'll spin up another version
> >
> > thanks alot
> >
> > Noel
> 
> here's  v4,

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

which is a pushover for an attacker.
Hint: what if start_sid_offset = start_sid_offset = 0xFFFFFFFF ?

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

Jeremy.



More information about the samba-technical mailing list