Quota patches

[Noel Power]

Hi Jeremy,

Please see attached v2 of the patch and also inline comments


On 25/07/18 20:02, Jeremy Allison wrote:
> On Tue, Jul 24, 2018 at 10:29:23AM +0100, Noel Power via samba-technical wrote:
>> Hi Jeremy
>>
>> I promised at sambaxp to bug you again with the quota patches for smb2
>> (which also include some rewrite of the existing code to use idl). Aside
>> from the fact we need to step away from SMB1 we have some customer
>> interest in this. I hope you can find some time to have a look. I've
>> rebased the patches (attached) against current master
> Getting there.
>
> In fill_quota_buffer(), uint32_t max_data is always non-zero,
> so I'd prefer this code:
>
>                if (max_data && ((header_ndr->offset - next_align)
> +                  + sizeof(info.next_entry_offset)
> +                  + sizeof(info.sid_length)
> +                  + sizeof(info.change_time)
> +                  + sizeof(info.quota_used)
> +                  + sizeof(info.quota_threshold)
> +                  + sizeof(info.quota_limit)
> +                  + info.sid_length) > max_data) {
> +                       max_data_exceeded = true;
> +                       break;
> +               }
>
> to look like this:
>
> 	uint32_t dsize = sizeof(info.next_entry_offset)
> 			+ sizeof(info.sid_length)
> 			+ sizeof(info.change_time)
> 			+ sizeof(info.quota_used)
> 			+ sizeof(info.quota_threshold)
> 			+ sizeof(info.quota_limit)
>
> 	if (dsize > max_data) {
> 		max_data_exceeded = true;
> 		break;
> 	}
>
> 	if (dsize + info.sid_length < dsize) {
> 		/* Integer wrap */
> 		max_data_exceeded = true;
> 		break;
> 	}
>
> 	if (dsize + info.sid_length > max_data) {
> 		max_data_exceeded = true;
> 		break;
> 	}
the  statement "uint32_t max_data is always non-zero" is not true, in
cliquota.c function 'cli_set_user_quota' calls 'build_user_quota_buffer'
with max_data set to '0'  So I added the tests above but they have been
adjusted with that in mind
>
> Also, in:
>
> extract_sids_from_buf()
>
> +                       blob.data = sid_buf->data + offset;
>
> Above needs protection that sid_buf->data + offset doesn't
> go off the end of sid_buf->length
That actually is already in place, this is in a while loop so the test
is actually at the end of the loop (after offset is pulled off the
wire). Additionally the test is against sidlistlength because we are
sure that sidlistlength is at least less than the size of the sid_buf.
Note: the caller of 'extract_sids_from_buf' has checked that, but... you
are correct, this function should check it itself so I have added the
test to make sure  sidlistlength doesn't exceed the size of the buffer
passed in, all subsequent tests are against sidlistlength (which in
could be less than size the sid_buf passed in)
>
> +                       blob.length = sidlistlength - offset;
>
> Likewise doesn't this also need a test that sidlistlength
> fits within sid_buf ?
>
> *Anything* that does arithmetic in a ranged buffer blob
> needs really careful examination.
yep, see above, the test is at the end of the while loop, I tried to
make the logic there a little simpler, I got rid of the 'can_pull' test
in the while loop and changed the existing boundary checks to hopefully
make them clearer
>
> This:
>
> +                       if (new_offset && (sidlistlength - offset - new_offset) <= 0) {
>
> is too complicated to easily evaluate. Please
> break it down to separate statements so I can
> understand exactly what you're checking for here.
>
> Use helper variables to make things clearer.
I'm hoping the reorganisation in 'extract_sids_from_buf' covers that
>
> Having said that I *LOVE* the conversion to
> IDL-based code. This is a significant improvement
> if we can just get the range checking past my
> creaking old brain :-).
>
> I know I'm paranoid about this stuff, but it pays
> off in the long run :-).
again +1 for paranoia about this, I feel safer because of it

In addition to the above changes I also
  changed the text of one of the existing debug messages

         if (!NDR_ERR_CODE_IS_SUCCESS(err)) {
-            DBG_DEBUG("Failed to pull the quota sid\n");
+            DBG_DEBUG("Failed to push the quota sid\n");

and got rid of 2 c warnings I missed

Noel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: quota-v2.patch
Type: text/x-patch
Size: 79095 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180726/3811d8b1/quota-v2.bin>