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