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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
Hi,

I got curious after failing to get any useful response from the windows
quota dialog tab (win8.1 client) when connected to a samba share. Seems
smbd doesn't support some of the quota related messages in smb2. The
attached patches hope to fix that, additionally I tried to clean up some
of the code by removing some of the manual marshall/unmarshall code.

The last patch in the series (PATCH 9) modifies build_user_quota_buffer,
the resulting function is quite similar to the fill_quota_buffer from
PATCH 4, the implementation could be made common for the client & smbd I
think (and probably live in libsmb somewhere) I'd be interested to hear
if that would be the correct place to put it (or even any other
suggestions). If it is ok I would like to do that as a followup patch

Noel


quota.patch (62K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
On 02/03/17 12:09, Noel Power wrote:

> Hi,
>
> I got curious after failing to get any useful response from the windows
> quota dialog tab (win8.1 client) when connected to a samba share. Seems
> smbd doesn't support some of the quota related messages in smb2. The
> attached patches hope to fix that, additionally I tried to clean up some
> of the code by removing some of the manual marshall/unmarshall code.
>
> The last patch in the series (PATCH 9) modifies build_user_quota_buffer,
> the resulting function is quite similar to the fill_quota_buffer from
> PATCH 4, the implementation could be made common for the client & smbd I
> think (and probably live in libsmb somewhere) I'd be interested to hear
> if that would be the correct place to put it (or even any other
> suggestions). If it is ok I would like to do that as a followup patch
>
> Noel
>
<sigh> please hold off looking at these for the moment, it seems I have
made some mistake when I was porting these from 4.x, I will post a new
set of patches when I fix whatever extra mess I have made


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
On 02/03/17 13:59, Noel Power wrote:

> On 02/03/17 12:09, Noel Power wrote:
>> Hi,
>>
>> I got curious after failing to get any useful response from the windows
>> quota dialog tab (win8.1 client) when connected to a samba share. Seems
>> smbd doesn't support some of the quota related messages in smb2. The
>> attached patches hope to fix that, additionally I tried to clean up some
>> of the code by removing some of the manual marshall/unmarshall code.
>>
>> The last patch in the series (PATCH 9) modifies build_user_quota_buffer,
>> the resulting function is quite similar to the fill_quota_buffer from
>> PATCH 4, the implementation could be made common for the client & smbd I
>> think (and probably live in libsmb somewhere) I'd be interested to hear
>> if that would be the correct place to put it (or even any other
>> suggestions). If it is ok I would like to do that as a followup patch
>>
>> Noel
>>
> <sigh> please hold off looking at these for the moment, it seems I have
> made some mistake when I was porting these from 4.x, I will post a new
> set of patches when I fix whatever extra mess I have made
>
please see (hopefully better) version 2 :/

Noel


quota-v2.patch (63K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jeremy Allison
On Thu, Mar 02, 2017 at 04:26:39PM +0000, Noel Power wrote:

> On 02/03/17 13:59, Noel Power wrote:
> > On 02/03/17 12:09, Noel Power wrote:
> >> Hi,
> >>
> >> I got curious after failing to get any useful response from the windows
> >> quota dialog tab (win8.1 client) when connected to a samba share. Seems
> >> smbd doesn't support some of the quota related messages in smb2. The
> >> attached patches hope to fix that, additionally I tried to clean up some
> >> of the code by removing some of the manual marshall/unmarshall code.
> >>
> >> The last patch in the series (PATCH 9) modifies build_user_quota_buffer,
> >> the resulting function is quite similar to the fill_quota_buffer from
> >> PATCH 4, the implementation could be made common for the client & smbd I
> >> think (and probably live in libsmb somewhere) I'd be interested to hear
> >> if that would be the correct place to put it (or even any other
> >> suggestions). If it is ok I would like to do that as a followup patch
> >>
> >> Noel
> >>
> > <sigh> please hold off looking at these for the moment, it seems I have
> > made some mistake when I was porting these from 4.x, I will post a new
> > set of patches when I fix whatever extra mess I have made
> >
> please see (hopefully better) version 2 :/

In [PATCH 4/9] s3/smbd: Add support for GetInfo/SMB2_0_INFO_QUOTA [MS-SMB2] 2.2.37.1 requests

You have:

+static enum ndr_err_code extract_sids_from_buf(TALLOC_CTX *mem_ctx,
+                                 uint32_t sidlistlength,
+                                 uint32_t startsidlength,
+                                 uint32_t startsidoffset,
+                                 uint8_t *sid_buf,
+                                 struct dom_sid **sids,
+                                 uint32_t *num)
+{
+       DATA_BLOB blob;
+       int i = 0;
+       enum ndr_err_code err;
+
.....
+               while (can_pull) {
+                       struct file_get_quota_info info;
+                       struct sid_list_elem *item = NULL;
+                       NDR_PULL_NEED_BYTES(ndr_pull, offset);
+                       err = ndr_pull_file_get_quota_info(ndr_pull,
+                                          NDR_SCALARS | NDR_BUFFERS, &info);
+                       if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
+                               DBG_ERR("Failed to pull file_get_quota_info "
+                                       "from sidlist buffer\n");
+                               goto done;
+                       }
+                       item = talloc_zero(list_ctx, struct sid_list_elem);
+                       if (!item) {
+                               DBG_ERR("OOM\n");
+                               err = NDR_ERR_ALLOC;
+                               goto done;
+                       }
+                       item->sid = info.sid;
+                       DLIST_ADD(sid_list, item);
+                       i++;
+                       offset = info.next_entry_offset;
+                       can_pull = (offset > 0);
+               }

Don't use 'int i' here. You're parsing stuff
off the wire and MUST be paranoid. I suggest
uint32_t i, and add a range/wrap check after
the i++.

I know wrap here is essentially impossible,
but I'm trying to teach everyone to be scared
writing C code that parses stuff from the wire
(actually I'd rather we didn't use C, but
that's another matter :-).

Cheers,

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
Hi Jeremy,

On 02/03/17 22:05, Jeremy Allison wrote:

> On Thu, Mar 02, 2017 at 04:26:39PM +0000, Noel Power wrote:
>> On 02/03/17 13:59, Noel Power wrote:
>>> On 02/03/17 12:09, Noel Power wrote:
>>>> Hi,
>>>>
>>>> I got curious after failing to get any useful response from the windows
>>>> quota dialog tab (win8.1 client) when connected to a samba share. Seems
>>>> smbd doesn't support some of the quota related messages in smb2. The
>>>> attached patches hope to fix that, additionally I tried to clean up some
>>>> of the code by removing some of the manual marshall/unmarshall code.
>>>>
>>>> The last patch in the series (PATCH 9) modifies build_user_quota_buffer,
>>>> the resulting function is quite similar to the fill_quota_buffer from
>>>> PATCH 4, the implementation could be made common for the client & smbd I
>>>> think (and probably live in libsmb somewhere) I'd be interested to hear
>>>> if that would be the correct place to put it (or even any other
>>>> suggestions). If it is ok I would like to do that as a followup patch
>>>>
>>>> Noel
[...]
> In [PATCH 4/9] s3/smbd: Add support for GetInfo/SMB2_0_INFO_QUOTA
> [MS-SMB2] 2.2.37.1 requests
> You have:
[...]

> +                       item->sid = info.sid;
> +                       DLIST_ADD(sid_list, item);
> +                       i++;
> +                       offset = info.next_entry_offset;
> +                       can_pull = (offset > 0);
> +               }
>
> Don't use 'int i' here. You're parsing stuff
> off the wire and MUST be paranoid. I suggest
> uint32_t i, and add a range/wrap check after
> the i++.
>
> I know wrap here is essentially impossible,
> but I'm trying to teach everyone to be scared
'i' should in anycase be unsigned thanks for catching that. 'i' isn't
really used in the parsing though (I depend completely on the ndr
routines) it's just a counter. Agreed you can never be paranoid enough
(I am already paranoid about my ability to screw things up :-)) so
hopefully the new patch does the needed :-)

Noel

quota-v3.txt (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jeremy Allison
On Fri, Mar 03, 2017 at 09:56:48AM +0000, Noel Power wrote:

> 'i' should in anycase be unsigned thanks for catching that. 'i' isn't
> really used in the parsing though (I depend completely on the ndr
> routines) it's just a counter. Agreed you can never be paranoid enough
> (I am already paranoid about my ability to screw things up :-)) so
> hopefully the new patch does the needed :-)

Better, thanks. However I'm going to ask for a v4,
to prevent a potential CPU spinning attack.

In s3/smbd: Add support for GetInfo/SMB2_0_INFO_QUOTA [MS-SMB2] 2.2.37.1 requests

You have:

+               while (can_pull) {
+                       struct file_get_quota_info info;
+                       struct sid_list_elem *item = NULL;
+                       NDR_PULL_NEED_BYTES(ndr_pull, offset);
+                       err = ndr_pull_file_get_quota_info(ndr_pull,
+                                          NDR_SCALARS | NDR_BUFFERS, &info);
+                       if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
+                               DBG_ERR("Failed to pull file_get_quota_info "
+                                       "from sidlist buffer\n");
+                               goto done;
+                       }
+                       item = talloc_zero(list_ctx, struct sid_list_elem);
+                       if (!item) {
+                               DBG_ERR("OOM\n");
+                               err = NDR_ERR_ALLOC;
+                               goto done;
+                       }
+                       item->sid = info.sid;
+                       DLIST_ADD(sid_list, item);
+                       i++;
+                       if (i == UINT32_MAX) {
+                               DBG_ERR("Integer overflow\n");
+                               err = NDR_ERR_ARRAY_SIZE;
+                               goto done;
+                       }
+                       offset = info.next_entry_offset;
+                       can_pull = (offset > 0);
+               }

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 ?

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
On 07/03/17 23:41, Jeremy Allison wrote:

> On Fri, Mar 03, 2017 at 09:56:48AM +0000, Noel Power wrote:
>
>> 'i' should in anycase be unsigned thanks for catching that. 'i' isn't
>> really used in the parsing though (I depend completely on the ndr
>> routines) it's just a counter. Agreed you can never be paranoid enough
>> (I am already paranoid about my ability to screw things up :-)) so
>> hopefully the new patch does the needed :-)
> Better, thanks. However I'm going to ask for a v4,
> to prevent a potential CPU spinning attack.
>
> In s3/smbd: Add support for GetInfo/SMB2_0_INFO_QUOTA [MS-SMB2] 2.2.37.1 requests
>
> You have:
>
> +               while (can_pull) {
> +                       struct file_get_quota_info info;
> +                       struct sid_list_elem *item = NULL;
> +                       NDR_PULL_NEED_BYTES(ndr_pull, offset);
> +                       err = ndr_pull_file_get_quota_info(ndr_pull,
> +                                          NDR_SCALARS | NDR_BUFFERS, &info);
> +                       if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
> +                               DBG_ERR("Failed to pull file_get_quota_info "
> +                                       "from sidlist buffer\n");
> +                               goto done;
> +                       }
> +                       item = talloc_zero(list_ctx, struct sid_list_elem);
> +                       if (!item) {
> +                               DBG_ERR("OOM\n");
> +                               err = NDR_ERR_ALLOC;
> +                               goto done;
> +                       }
> +                       item->sid = info.sid;
> +                       DLIST_ADD(sid_list, item);
> +                       i++;
> +                       if (i == UINT32_MAX) {
> +                               DBG_ERR("Integer overflow\n");
> +                               err = NDR_ERR_ARRAY_SIZE;
> +                               goto done;
> +                       }
> +                       offset = info.next_entry_offset;
> +                       can_pull = (offset > 0);
> +               }
>
> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Noel Power
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,

thanks,

Noel


quota-v4.txt (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Hi Jeremy
On 13/03/17 09:15, Noel Power wrote:
> Hi Jeremy
> On 10/03/17 16:50, Jeremy Allison wrote:
[...]

>>> 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
please see latest patches, hopefully they cover the issues above,
additionally I added 2 more patches, one to enable smb2 for the existing
quota check and the other the follow-up patch to combine
build_user_quota_buffer & fill_quota_buffer into a single common func.

thanks,

Noel


quota-v6.txt (65K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 13/03/17 22:08, Noel Power wrote:

> Hi Jeremy
> On 13/03/17 09:15, Noel Power wrote:
>> Hi Jeremy
>> On 10/03/17 16:50, Jeremy Allison wrote:
> [...]
>>>> 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
> please see latest patches, hopefully they cover the issues above,
> additionally I added 2 more patches, one to enable smb2 for the existing
> quota check and the other the follow-up patch to combine
> build_user_quota_buffer & fill_quota_buffer into a single common func.
bah, I have to say please ignore these for  a moment, I spotted 2
problems (1 the patches all don't build individually anymore and a small
bug for smbcquota -C) I will post new patches later

Noel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 15/03/17 18:52, Noel Power wrote:

> On 13/03/17 22:08, Noel Power wrote:
>> Hi Jeremy
>> On 13/03/17 09:15, Noel Power wrote:
>>> Hi Jeremy
>>> On 10/03/17 16:50, Jeremy Allison wrote:
>> [...]
>>>>> 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
>> please see latest patches, hopefully they cover the issues above,
>> additionally I added 2 more patches, one to enable smb2 for the existing
>> quota check and the other the follow-up patch to combine
>> build_user_quota_buffer & fill_quota_buffer into a single common func.
> bah, I have to say please ignore these for  a moment, I spotted 2
> problems (1 the patches all don't build individually anymore and a small
> bug for smbcquota -C) I will post new patches later
>
> Noel
hopefully better now, please see attached, basically a couple of hunks
needed to be moved from Patch 11 to ensure Patch 8 would build.
Additionally Patch 11 was missing a test to see if max_data was
specified before checking if its value was exceeded

thanks

Noel


quota-v6.1.txt (85K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Hi Jeremy
On 15/03/17 19:15, Noel Power wrote:

> On 15/03/17 18:52, Noel Power wrote:
>> On 13/03/17 22:08, Noel Power wrote:
>>> Hi Jeremy
>>> On 13/03/17 09:15, Noel Power wrote:
>>>> Hi Jeremy
>>>> On 10/03/17 16:50, Jeremy Allison wrote:
>>> [...]
>>>>>> 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
>>> please see latest patches, hopefully they cover the issues above,
>>> additionally I added 2 more patches, one to enable smb2 for the existing
>>> quota check and the other the follow-up patch to combine
>>> build_user_quota_buffer & fill_quota_buffer into a single common func.
>> bah, I have to say please ignore these for  a moment, I spotted 2
>> problems (1 the patches all don't build individually anymore and a small
>> bug for smbcquota -C) I will post new patches later
>>
>> Noel
> hopefully better now, please see attached, basically a couple of hunks
> needed to be moved from Patch 11 to ensure Patch 8 would build.
> Additionally Patch 11 was missing a test to see if max_data was
> specified before checking if its value was exceeded
>
> thanks
>
> Noel
>
well one thing that I missed was an easy way to run list/get/set each
time I changed some detail so I've updated the patch set to include a
basic torture test for smbcquotas exercising get/set/list of quotas
using smb1 & smb2.

Also patch 14 makes a change to smbcquota to all admin users rather that
root actually set quoatas (currently only enabled if --enable-selftest)
I have to wonder though about the requirement for the user to actually
be root in order to set quotas, is that a mistake or intentional ?

thanks,

Noel


quotas-v7.txt (112K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Gentle reminder :-) please let me know if there anything else I need to
do here

Noel
On 23/03/17 15:56, Noel Power wrote:

> Hi Jeremy
> On 15/03/17 19:15, Noel Power wrote:
>> On 15/03/17 18:52, Noel Power wrote:
>>> On 13/03/17 22:08, Noel Power wrote:
>>>> Hi Jeremy
>>>> On 13/03/17 09:15, Noel Power wrote:
>>>>> Hi Jeremy
>>>>> On 10/03/17 16:50, Jeremy Allison wrote:
>>>> [...]
>>>>>>> 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
>>>> please see latest patches, hopefully they cover the issues above,
>>>> additionally I added 2 more patches, one to enable smb2 for the existing
>>>> quota check and the other the follow-up patch to combine
>>>> build_user_quota_buffer & fill_quota_buffer into a single common func.
>>> bah, I have to say please ignore these for  a moment, I spotted 2
>>> problems (1 the patches all don't build individually anymore and a small
>>> bug for smbcquota -C) I will post new patches later
>>>
>>> Noel
>> hopefully better now, please see attached, basically a couple of hunks
>> needed to be moved from Patch 11 to ensure Patch 8 would build.
>> Additionally Patch 11 was missing a test to see if max_data was
>> specified before checking if its value was exceeded
>>
>> thanks
>>
>> Noel
>>
> well one thing that I missed was an easy way to run list/get/set each
> time I changed some detail so I've updated the patch set to include a
> basic torture test for smbcquotas exercising get/set/list of quotas
> using smb1 & smb2.
>
> Also patch 14 makes a change to smbcquota to all admin users rather that
> root actually set quoatas (currently only enabled if --enable-selftest)
> I have to wonder though about the requirement for the user to actually
> be root in order to set quotas, is that a mistake or intentional ?
>
> thanks,
>
> Noel
>


Loading...