Bug 9460 - Samba 3.6.x and Master respond incorrectly to FILE_STREAM_INFO requests
Samba 3.6.x and Master respond incorrectly to FILE_STREAM_INFO requests
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
unspecified
All All
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8622
  Show dependency treegraph
 
Reported: 2012-12-04 17:06 UTC by Richard Sharpe
Modified: 2012-12-07 08:25 UTC (History)
0 users

See Also:


Attachments
Win7 to W2K08 dir/r results (2.90 KB, application/octet-stream)
2012-12-04 17:07 UTC, Richard Sharpe
no flags Details
Win 7 to Samba 3.6.6 dir/r result (4.31 KB, application/octet-stream)
2012-12-04 17:09 UTC, Richard Sharpe
no flags Details
Patch that gets Samba 3.6.6 behaving as W2K08 does. (1.25 KB, application/octet-stream)
2012-12-04 17:38 UTC, Richard Sharpe
no flags Details
git-am fix for master, 4.0.0, and 3.6.x. (2.20 KB, patch)
2012-12-05 01:23 UTC, Jeremy Allison
rsharpe: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2012-12-04 17:06:17 UTC
There is a bug in source3/smbd/trans2.c:marshall_stream_info where it is overflowing the buffer provided and returning without all the data.

Attached is a capture of Windows to Windows (via SMB2) of a dir/r request. It shows W2K08 responding to STATUS_BUFFER_OVERFLOW twice until the client gives it a large enough buffer.

Attached also is a capture of Samba 3.6.6 (but it applies to master, I believe) of Samba returning incomplete data and an overflowed result but the wrong status.
Comment 1 Richard Sharpe 2012-12-04 17:07:54 UTC
Created attachment 8275 [details]
Win7 to W2K08 dir/r results
Comment 2 Richard Sharpe 2012-12-04 17:09:34 UTC
Created attachment 8276 [details]
Win 7 to Samba 3.6.6 dir/r result
Comment 3 Richard Sharpe 2012-12-04 17:13:52 UTC
Here is a potential fix for this problem. Basically, correctly detect that we will overflow the return buffer and return STATUS_BUFFER_OVERFLOW before doing so. I will attach a patch as well after I have tested it.

index 61d755c..9bd1f53 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -4159,7 +4159,7 @@ static NTSTATUS marshall_stream_info(unsigned int num_stre
        unsigned int i;
        unsigned int ofs = 0;

-       for (i = 0; i < num_streams && ofs <= max_data_bytes; i++) {
+       for (i = 0; i < num_streams; i++) {
                unsigned int next_offset;
                size_t namelen;
                smb_ucs2_t *namebuf;
@@ -4178,6 +4178,15 @@ static NTSTATUS marshall_stream_info(unsigned int num_str

                namelen -= 2;

+               /*
+                * We cannot overflow ...
+                */
+               if ((ofs + 24 + namelen) > max_data_bytes) {
+                       DEBUG(10, ("overflowed the data buffer at stream %u\n",
+                               i));
+                       return STATUS_BUFFER_OVERFLOW;
+               }
+
                SIVAL(data, ofs+4, namelen);
                SOFF_T(data, ofs+8, streams[i].size);
                SOFF_T(data, ofs+16, streams[i].alloc_size);
@@ -4801,6 +4810,7 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn,
                        if (!NT_STATUS_IS_OK(status)) {
                                DEBUG(10, ("marshall_stream_info failed: %s\n",
                                           nt_errstr(status)));
+                               TALLOC_FREE(streams);
                                return status;
                        }
Comment 4 Richard Sharpe 2012-12-04 17:38:20 UTC
Created attachment 8278 [details]
Patch that gets Samba 3.6.6 behaving as W2K08 does.

With this patch we now seem to behave as W2K08 does.
Comment 5 Jeremy Allison 2012-12-04 17:43:22 UTC
This needs to be a blocker for 3.6.x and 4.0.x.

Jeremy.
Comment 6 Jeremy Allison 2012-12-05 01:18:10 UTC
Ok, the same check needs to be done when we're adding the alignment padding just a little later on.

Updated patches (git-am format) for master, 4.0.0 and 3.6.x to follow.

Jeremy.
Comment 7 Jeremy Allison 2012-12-05 01:23:36 UTC
Created attachment 8281 [details]
git-am fix for master, 4.0.0, and 3.6.x.

Richard can you review please ? If it passes your tests I'll push to master and assign to Karolin for inclusion in 3.6.next and 4.0.0.

Same fix cherry-picks cleanly across all branches.

Jeremy.
Comment 8 Richard Sharpe 2012-12-05 03:41:38 UTC
Comment on attachment 8281 [details]
git-am fix for master, 4.0.0, and 3.6.x.

Looks good to me. The last debug could be dropped. It is not really needed.
Comment 9 Jeremy Allison 2012-12-06 00:33:49 UTC
Pushed to master (943797c232f96a5dd411a803ad90b6980b2785b0).

Re-assigning to Karolin for inclusion in 3.6.x and 4.0.0.

Jeremy.
Comment 10 Karolin Seeger 2012-12-06 08:36:20 UTC
Pushed to autobuild-v4-0-test and v3-6-test (will be included in 3.6.10).
Comment 11 Karolin Seeger 2012-12-07 08:25:16 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!