Can we get at least a network trace of this, even better would be a torture test.

Thanks,

Volker

Richard, if you can upload the network trace I'll help in writing the torture test.

Cheers,

Jeremy.

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.

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.

Edited title to avoid confusion.

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.

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

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

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

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 on attachment 7647 [details]
git am fix for 3.6.next and 3.5.next.

Do we need something like this for trans2.c?

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

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

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.

(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.
                 */

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

MS-CIFS 2.2.4.47.2 is pretty clear on Trans2. It states "there is no reply for trans2s".

Jeremy.

Ok Richard, are you happy for this to go into 3.6.next and 3.5.next ?

Jeremy.

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

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 on attachment 7660 [details]
git am fix for 3.6.next and 3.5.next.

Yes. This is great.

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.

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

Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!