[PATCH] smbd: Fix snapshot query on shares with DFS enabled

[Jeremy Allison]

On Mon, Aug 22, 2016 at 12:50:45PM +0300, Uri Simchoni wrote:
> On 08/20/2016 06:37 AM, Jeremy Allison wrote:
> > On Fri, Aug 19, 2016 at 11:22:54AM +0300, Uri Simchoni wrote:

> I didn't understand why the following check is made in
> cli_smb2_shadow_copy_data_fnum_recv():
> 
> > +       if (state->out_output_buffer.length +
> > +                       (2 * sizeof(SHADOW_COPY_LABEL)) <
> > +                               state->out_output_buffer.length) {
> > +               return NT_STATUS_INVALID_NETWORK_RESPONSE;
> > +       }
> > +
> 
> (besides not understanding its purpose, we should also keep in mind that
> smb2cli_ioctl_recv() has made certain that the output buffer is not
> larger than what we've asked for. The move to pointer arithmetic does
> not prevent overflow in 32-bit platforms, but the check that the
> response is < 64K does)

Yes, smb2cli_ioctl_recv() does make that guarantee, but as a matter
of security coding I prefer that each layer of code that directly
reads and manipulates remote returned offsets and pointers has it's own overflow
and wrap protection, on top of what called or calling functions do for it.

That way if the code gets cut-and-pasted elsewhere (as does happen :-)
the offset and wrap checks usually get pasted with it, retaining the
protection if the data comes from a source that *isn't* making such
a guarentee.

So the above makes sense as integer wrap protection if state->out_output_buffer.length
comes from a 'raw' remote source.

> Also if you happen to touch the code, perhaps it's worth commenting that
> dlength + 12 > state->out_output_buffer.length *is* a valid response (it
> means there are more snapshots than our buffer allows), but we currently
> don't support that.

OK, I'll add a comment here.

> Otherwise RB+ me.

Thanks !