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.