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.