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

Noel Power nopower at suse.com
Wed Mar 8 07:59:39 UTC 2017


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



More information about the samba-technical mailing list