Quota patches

[Jeremy Allison]

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;
	}

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

+                       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.

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.

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

Cheers,

	Jeremy.