Bug 8989 - Samba 3.5.x (and probably all other versions of Samba) does not send correct responses to NT Transact Secondary when no data and no params
Samba 3.5.x (and probably all other versions of Samba) does not send correct ...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.3
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-08 04:36 UTC by Richard Sharpe
Modified: 2012-06-30 10:40 UTC (History)
1 user (show)

See Also:
rsharpe: review+
rsharpe: review+
rsharpe: review? (rsharpe)


Attachments
What Windows does. (172.00 KB, application/octet-stream)
2012-06-08 16:09 UTC, Richard Sharpe
no flags Details
What Samba does (107.49 KB, application/octet-stream)
2012-06-08 16:11 UTC, Richard Sharpe
no flags Details
A fix for Wireshark to desegment NT TRANSACT requests/responses (6.98 KB, patch)
2012-06-10 03:22 UTC, Richard Sharpe
no flags Details
git am fix for 3.6.next and 3.5.next. (1.12 KB, patch)
2012-06-14 21:55 UTC, Jeremy Allison
no flags Details
git am fix for 3.6.next and 3.5.next. (2.20 KB, patch)
2012-06-14 23:53 UTC, Jeremy Allison
no flags Details
git am fix for 3.6.next and 3.5.next. (2.93 KB, patch)
2012-06-18 23:27 UTC, Jeremy Allison
jra: review? (rsharpe)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2012-06-08 04:36:28 UTC
This problem came to light when one of our QA people tried to create an SD with large DACLs (larger than 32kiB). 

Anyway, I checked what W2K03 and W2K08 do, and W2K08 generates
responses like that, and W2K03 accepts them, and when I added the
small change below, W2K03 stopped complaining about us.

If you look in the normal path where the number of parameters in the
response is not zero or the amount of data in the response is not zero
you will see that we set the response to an NT TRANSACT response, not
an NT TRANSACT SECONDARY response.

The following patch fixes things. (Although I suspect that this could be better done in the main body.)

--- smbd/nttrans.c.orig 2012-06-07 15:59:33.170255874 -0700
+++ smbd/nttrans.c      2012-06-07 15:53:05.719790218 -0700
@@ -67,6 +67,9 @@ void send_nt_replies(connection_struct *

       if(params_to_send == 0 && data_to_send == 0) {
               reply_outbuf(req, 18, 0);
+               /* [MS_CIFS].pdf 2.2.4.62.2 says NT TRANS used for response
+                  even for secondaries */
+               SCVAL(req->outbuf, smb_com, SMBnttrans);
               if (NT_STATUS_V(nt_error)) {
                       error_packet_set((char *)req->outbuf,
                                        0, 0, nt_error,
Comment 1 Volker Lendecke 2012-06-08 05:27:39 UTC
Can we get at least a network trace of this, even better would be a torture test.

Thanks,

Volker
Comment 2 Jeremy Allison 2012-06-08 15:47:33 UTC
Richard, if you can upload the network trace I'll help in writing the torture test.

Cheers,

Jeremy.
Comment 3 Richard Sharpe 2012-06-08 16:09:57 UTC
Created attachment 7634 [details]
What Windows does.

This attachment shows what Windows 2008R2 does. Note frames 312 and 315.

Wireshark gets things wrong here, but displays the basic SMB packets.
Comment 4 Richard Sharpe 2012-06-08 16:11:17 UTC
Created attachment 7635 [details]
What Samba does

This shows what Samba does in the same circumstances. Note frames 68 and 71.

Windows does not like the response.
Comment 5 Richard Sharpe 2012-06-08 16:39:13 UTC
Edited title to avoid confusion.
Comment 6 Christophuzzy R. Hertel 2012-06-08 17:33:54 UTC
In the Windows trace, we see the following sequence of events:
* Client sends an NT_TRANSACT request with function NT_SET_SECURITY_DESC (298)
* Server responds with an Interim NT_TRANSACT response (301)
* Client sends a single NT_TRANSACT_SECONDARY request (312)
* Server responds with a single NT_TRANSACT response (315)

This exchange matches the specification; the docs appear to be correct.

Note that the client could have sent multiple NT_TRANSACT_SECONDARY requests but the server would still have sent only the final response.  NT_TRANSACT_SECONDARY requests are not individually acknowledged.  The transaction as a whole is acknowledged once it has been processed.

Note also that the response from the server MAY be multiple NT_TRANSACT response message.  This will happen if the response requires multiple messages in order to transfer all of the data.

The old X/Open (Open Group) c209 specification gives a good explanation of this, starting on page 210.  The description is for SMBTrans2 exchanges, but the same patterns apply.  There is also a quick and dirty explanation here:  http://ubiqx.org/cifs/Browsing.html#BRO.4.3.4

Summary:

* [MS-CIFS], [Implementing CIFS], and [XOPEN-SMB] provide correct (based upon Windows behaviour) information on fragmented transaction exchanges.

* Samba (in some cases...not all cases have been tested) is sending individual responses to NT_TRANSACT_SECONDARY requests, which is contrary to the specification.
Comment 7 Richard Sharpe 2012-06-08 17:37:02 UTC
(In reply to comment #6)
> In the Windows trace, we see the following sequence of events:
> * Client sends an NT_TRANSACT request with function NT_SET_SECURITY_DESC (298)
> * Server responds with an Interim NT_TRANSACT response (301)
> * Client sends a single NT_TRANSACT_SECONDARY request (312)
> * Server responds with a single NT_TRANSACT response (315)
> 
> This exchange matches the specification; the docs appear to be correct.
> 
> Note that the client could have sent multiple NT_TRANSACT_SECONDARY requests
> but the server would still have sent only the final response. 
> NT_TRANSACT_SECONDARY requests are not individually acknowledged.  The
> transaction as a whole is acknowledged once it has been processed.
> 
> Note also that the response from the server MAY be multiple NT_TRANSACT
> response message.  This will happen if the response requires multiple messages
> in order to transfer all of the data.
> 
> The old X/Open (Open Group) c209 specification gives a good explanation of
> this, starting on page 210.  The description is for SMBTrans2 exchanges, but
> the same patterns apply.  There is also a quick and dirty explanation here: 
> http://ubiqx.org/cifs/Browsing.html#BRO.4.3.4
> 
> Summary:
> 
> * [MS-CIFS], [Implementing CIFS], and [XOPEN-SMB] provide correct (based upon
> Windows behaviour) information on fragmented transaction exchanges.
> 
> * Samba (in some cases...not all cases have been tested) is sending individual
> responses to NT_TRANSACT_SECONDARY requests, which is contrary to the
> specification.

Hmmm, if I get some time I will look at that to see if we are doing the correct things always, although a torture test will help determine that.
Comment 8 Richard Sharpe 2012-06-09 04:39:13 UTC
(In reply to comment #6)

> * Samba (in some cases...not all cases have been tested) is sending individual
> responses to NT_TRANSACT_SECONDARY requests, which is contrary to the
> specification.

Hmmm, in reply_nttranss (reply to an NT TRANS SECONDARY) we see the following:

        if ((state->received_param < state->total_param) ||
            (state->received_data < state->total_data)) {
                END_PROFILE(SMBnttranss);
                return;
        }

        handle_nttrans(conn, state, req);

        DLIST_REMOVE(conn->pending_trans, state);
        SAFE_FREE(state->data);
        SAFE_FREE(state->param);
        TALLOC_FREE(state);
        END_PROFILE(SMBnttranss);
        return;

bad_param:

I believe that this means that Samba does not send individual responses to NT TRANS SECONDARIES. It only sends a response (in handle_nttrans) when all the data is in. It sends an interim response for the NT TRANS if there is more data expected.
Comment 9 Richard Sharpe 2012-06-10 03:22:38 UTC
Created attachment 7636 [details]
A fix for Wireshark to desegment NT TRANSACT requests/responses

The attached patch is a fix for the current Wireshark packet-smb.c (from the repos) that now re-assembles all portions of NT TRANSACT requests/responses across multiple NT TRANSACT and NT TRANSACT SECONDARIES etc ...
Comment 10 Jeremy Allison 2012-06-14 21:55:23 UTC
Created attachment 7647 [details]
git am fix for 3.6.next and 3.5.next.

Richard, I think this is the correct fix - it modifies the command code so it will be returned on all error conditions as well as on success.

Can you test this fix (which should apply to 3.6.next and 3.5.next) and if you're happy we can get it into the next releases.

Jeremy.
Comment 11 Stefan Metzmacher 2012-06-14 22:04:30 UTC
Comment on attachment 7647 [details]
git am fix for 3.6.next and 3.5.next.

Do we need something like this for trans2.c?
Comment 12 Richard Sharpe 2012-06-14 22:11:10 UTC
(In reply to comment #10)
> Created attachment 7647 [details]
> git am fix for 3.6.next and 3.5.next.
> 
> Richard, I think this is the correct fix - it modifies the command code so it
> will be returned on all error conditions as well as on success.
> 
> Can you test this fix (which should apply to 3.6.next and 3.5.next) and if
> you're happy we can get it into the next releases.

D'oh. Of course that is the correct fix.

I will try to test this tomorrow.
Comment 13 Richard Sharpe 2012-06-14 22:11:50 UTC
(In reply to comment #11)
> Comment on attachment 7647 [details]
> git am fix for 3.6.next and 3.5.next.
> 
> Do we need something like this for trans2.c?

We probably do.
Comment 14 Jeremy Allison 2012-06-14 23:53:07 UTC
Created attachment 7648 [details]
git am fix for 3.6.next and 3.5.next.

git am fix for 3.5.next and 3.6.next that adds the same fix to the trans2 code.

See MS-CIFS 2.2.4.47.2 for details.
Comment 15 Richard Sharpe 2012-06-15 17:04:51 UTC
(In reply to comment #14)
> Created attachment 7648 [details]
> git am fix for 3.6.next and 3.5.next.
> 
> git am fix for 3.5.next and 3.6.next that adds the same fix to the trans2 code.
> 
> See MS-CIFS 2.2.4.47.2 for details.

OK, this patch, and a similar one for trans2 would seem appropriate, then, as well:

--- smbd/nttrans.c.orig 2012-06-15 10:03:46.633581980 -0700
+++ smbd/nttrans.c      2012-06-15 10:04:12.005507657 -0700
@@ -134,11 +134,6 @@ void send_nt_replies(connection_struct *
                             + data_alignment_offset);
 
                /*
-                * We might have had SMBnttranss in req->inbuf, fix that.
-                */
-               SCVAL(req->outbuf, smb_com, SMBnttrans);
-
-               /*
                 * Set total params and data to be sent.
                 */
Comment 16 Richard Sharpe 2012-06-15 17:19:30 UTC
(In reply to comment #14)
> Created attachment 7648 [details]
> git am fix for 3.6.next and 3.5.next.
> 
> git am fix for 3.5.next and 3.6.next that adds the same fix to the trans2 code.
> 
> See MS-CIFS 2.2.4.47.2 for details.

This patch fixes the problem. At least, it fixes the NT TRANS problem.

I don't currently have a test for the same issue with TRANS2.
Comment 17 Jeremy Allison 2012-06-15 17:31:14 UTC
MS-CIFS 2.2.4.47.2 is pretty clear on Trans2. It states "there is no reply for trans2s".

Jeremy.
Comment 18 Jeremy Allison 2012-06-18 22:54:58 UTC
Ok Richard, are you happy for this to go into 3.6.next and 3.5.next ?

Jeremy.
Comment 19 Richard Sharpe 2012-06-18 23:08:59 UTC
(In reply to comment #18)
> Ok Richard, are you happy for this to go into 3.6.next and 3.5.next ?
> 
> Jeremy.

Yes, I am. However, as I pointed out in comment 15, there is now some redundant code that could be removed in both functions.
Comment 20 Jeremy Allison 2012-06-18 23:27:48 UTC
Created attachment 7660 [details]
git am fix for 3.6.next and 3.5.next.

Updated fix containing the removal of the dead code. Richard, please review.

Jeremy.
Comment 21 Richard Sharpe 2012-06-18 23:53:30 UTC
Comment on attachment 7660 [details]
git am fix for 3.6.next and 3.5.next.

Yes. This is great.
Comment 22 Jeremy Allison 2012-06-19 00:03:55 UTC
Richard, you are setting the +1 flag on the bug itself. I'll take that as a +1, but what you need to do is click on the "Details" link on the attachment that contains the patch you are reviewing, and then change the flag from '?' to '+' on the ATTACHMENT details, not on the main bug page.

In the meantime, I'll re-assign to Karolin for inclusion in 3.5.next and 3.6.next.

Karolin, please merge for 3.5.next and 3.6.next.

Jeremy.
Comment 23 Richard Sharpe 2012-06-19 00:12:34 UTC
(In reply to comment #22)
> Richard, you are setting the +1 flag on the bug itself. I'll take that as a +1,
> but what you need to do is click on the "Details" link on the attachment that
> contains the patch you are reviewing, and then change the flag from '?' to '+'
> on the ATTACHMENT details, not on the main bug page.
> 
> In the meantime, I'll re-assign to Karolin for inclusion in 3.5.next and
> 3.6.next.
> 
> Karolin, please merge for 3.5.next and 3.6.next.
> 
> Jeremy.

It never shows me that box even in the Details link. I think it is because my login name has my email address in it, and I cannot figure out how to change it.
Comment 24 Karolin Seeger 2012-06-30 10:40:36 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!