Bug 10424 - incorrect zero length server-side copy request handling
incorrect zero length server-side copy request handling
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-06 16:12 UTC by David Disseldorp
Modified: 2014-02-16 16:14 UTC (History)
0 users

See Also:


Attachments
Patch for the v4-1-test branch. Cherry-pick of master commits with small change in smbtorture patch context (7.35 KB, patch)
2014-02-07 10:57 UTC, David Disseldorp
jra: review+
ddiss: review? (asn)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2014-02-06 16:12:22 UTC
I was playing with the reproducer for the btrfs kernel.org bug 60666, and to my surprise found that a BTRFS_IOC_CLONE_RANGE with @src_length=0 resulted in a clone of all data from @src_offset->EOF!

Given that Samba's vfs_btrfs module maps server-side copy requests to BTRFS_IOC_CLONE_RANGE requests, this could potentially lead to file corruption if an SMB2 client were to issue an FSCTL_SRV_COPYCHUNK request with a zero byte
chunk length.

The MS-SMB2 spec states:
    Length (4 bytes): The number of bytes of data to copy.
Which would imply that a zero length request should not copy any data.

Testing this against Windows Server 2012, I found that it returns NT_STATUS_INVALID_PARAMETER to any copy-chunk requests with length=0, which luckily indicates that the len=0 request is not normally seen on the wire.

As feared, Samba with vfs_btrfs enabled returns success, and clones the entire range from copy-chunk source offset to end-of-file over to the destination offset.
With vfs_default, no IO is performed.


Aside from the smbtorture test, I plan to address this bug with patches to:
- Check for zero length server-side copy (FSCTL_SRV_COPYCHUNK)
  SMB2 requests. Return NT_STATUS_INVALID_PARAMETER in such a
  case, matching Windows Server 2012 behaviour.

- Fail zero length requests in btrfs_copy_chunk_send(). The
  server-side copy handler is currently the only caller, but
  this may change in future, so should be caught just in case.
Comment 1 David Disseldorp 2014-02-07 10:57:09 UTC
Created attachment 9653 [details]
Patch for the v4-1-test branch. Cherry-pick of master commits with small change in smbtorture patch context
Comment 2 Jeremy Allison 2014-02-07 20:15:58 UTC
Comment on attachment 9653 [details]
Patch for the v4-1-test branch. Cherry-pick of master commits with small change in smbtorture patch context

LGTM.
Comment 3 Jeremy Allison 2014-02-07 20:16:21 UTC
Re-assigning to Karolin for inclusion in 4.1.next.

Jeremy.
Comment 4 Karolin Seeger 2014-02-14 19:09:01 UTC
Pushed to autobuild-v4-1-test.
Comment 5 Karolin Seeger 2014-02-16 16:14:05 UTC
Pushed to v4-1-test.
Closing out bug report.

Thanks!