Bug 9626 - Samba master and 3.6.x does not handle invalid RELATED flag correctly
Samba master and 3.6.x does not handle invalid RELATED flag correctly
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-02 00:06 UTC by Richard Sharpe
Modified: 2014-07-29 23:21 UTC (History)
1 user (show)

See Also:


Attachments
The capture (75.20 KB, application/octet-stream)
2013-02-02 01:19 UTC, Richard Sharpe
no flags Details
experimental patchset for 3.6.x (8.64 KB, patch)
2013-02-16 04:45 UTC, Jeremy Allison
no flags Details
Wireshark trace against w2k8 (5.38 KB, application/vnd.tcpdump.pcap)
2013-02-16 04:46 UTC, Jeremy Allison
no flags Details
Wireshark trace against w2k12 (4.97 KB, application/vnd.tcpdump.pcap)
2013-02-16 04:46 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2013-02-02 00:06:00 UTC
The smb2.compound test reveals that we do not handle the case where the first request in a compound request has the RELATED (CHAINED) flag set.

The SMB2 spec says:

----------------------
SMB2_FLAGS_RELATED_OPERATIONS MUST be set in the Flags field of SMB2 headers on all requests except the first one.
----------------------

However, the foot notes say:

----------------------
<193> Section 3.3.5.2.7: Windows-based SMB2 servers allow a mix of related and unrelated compound requests in the same transport send. Upon encountering a request with SMB2_FLAGS_RELATED_OPERATIONS not set, a Windows-based SMB2 server treats it as the start of a chain.
<194> Section 3.3.5.2.7.2: If SMB2_FLAGS_RELATED_OPERATIONS is present in the first request and the request does not have valid SessionId, TreeId or FileId, Windows servers fail all requests in compounded chain with error STATUS_INVALID_PARAMETER. Otherwise, the operation will succeed.
----------------------

This accords with what I see in the captures from Windows. That is, Windows seems to regard itself as still being in a non-compound-request state when it starts on the second request in the compound if it has returned INVALID_PARAMETER on the first.

However, it tries to process the third request.
Comment 1 Richard Sharpe 2013-02-02 00:10:15 UTC
At first I thought that the following would do the trick, however, a capture I have suggests that we have to keep some state if we want to duplicate what Windows does. That state is whether or not we are in a related compound request. If a request is judged invalid, I suspect that it does not change the state for Windows.

--- build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c.orig     2013-02-01 23:40:41.444063192 -0800
+++ build/cloudcc/build_x86_64/samba-3.6.6/source3/smbd/smb2_server.c   2013-02-01 23:42:28.223040408 -0800
@@ -1241,9 +1241,14 @@ NTSTATUS smbd_smb2_request_dispatch(stru
                }
        }

-       allowed_flags = SMB2_HDR_FLAG_CHAINED |
-                       SMB2_HDR_FLAG_SIGNED |
+       /*
+        * SMB2_HDR_FLAG_CHAINED not allowed on the first request in a
+        * compound, so add it later
+        */
+       allowed_flags = SMB2_HDR_FLAG_SIGNED |
                        SMB2_HDR_FLAG_DFS;
+       if (i > 1)
+               allowed_flags |= SMB2_HDR_FLAG_CHAINED;
        if (opcode == SMB2_OP_CANCEL) {
                allowed_flags |= SMB2_HDR_FLAG_ASYNC;
        }
Comment 2 Richard Sharpe 2013-02-02 01:19:54 UTC
Created attachment 8526 [details]
The capture

Frames 295 and 303 show the problem.
Comment 3 Richard Sharpe 2013-02-05 20:01:55 UTC
OK, this comment in source4/torture/smb2/compound.c at around like 393 makes sense now:

        status = smb2_create_recv(req[0], tree, &cr);
        /* TODO: check why this fails with --signing=required */
        CHECK_STATUS(status, NT_STATUS_INVALID_PARAMETER);
        status = smb2_close_recv(req[1], &cl);
        CHECK_STATUS(status, NT_STATUS_INVALID_PARAMETER);
        status = smb2_close_recv(req[2], &cl);
        CHECK_STATUS(status, NT_STATUS_FILE_CLOSED);


The problem here is that when signing is on, we go down a slightly different path that causes the error.

In smbd_smb2_request_dispatch, we call smbd_smb2_request_check_session, and then if signing is required we check the status of the session check:

        /*
         * Check if the client provided a valid session id,
         * if so smbd_smb2_request_check_session() calls
         * set_current_user_info().
         *
         * As some command don't require a valid session id
         * we defer the check of the session_status
         */
        session_status = smbd_smb2_request_check_session(req);

        req->do_signing = false;
        if (flags & SMB2_HDR_FLAG_SIGNED) {
                if (!NT_STATUS_IS_OK(session_status)) {
                        return smbd_smb2_request_error(req, session_status);
                }


However, if CHAINED is on on the first request in the compound, then check_session returns NT_STATUS_USER_SESSION_DELETED.

So, we should not allow the first request in the compound to have that bit set.

Unfortunately, the error will likely shift to the next request in the test unless we keep some state as I discussed before to ensure that we do not treat the second request as if the first request had succeeded.

Based on the Windows note, we need a flag saying first chain seen or something similar.
Comment 4 Stefan Metzmacher 2013-02-06 05:50:19 UTC
We had that check, but it seems it got lost in commit
52aa2612b73b43589f01d87a4852df28e44d0cfb
Comment 5 Stefan Metzmacher 2013-02-06 05:50:59 UTC
(In reply to comment #4)
> We had that check, but it seems it got lost in commit
> 52aa2612b73b43589f01d87a4852df28e44d0cfb

I wonder why we didn't had tests which detected that change...
Comment 6 Jeremy Allison 2013-02-14 01:30:36 UTC
Ok, starting work on this one. I'll make sure we add a regression test for this.

Jeremy.
Comment 7 Jeremy Allison 2013-02-15 22:15:56 UTC
Ok, here's the relevant bit in the spec that allows us to fix this somewhat easier I think.

---------------------------------------------------------
3.2.4.1.4 Sending Compounded Requests

Compounding Related Requests

SMB2_FLAGS_RELATED_OPERATIONS MUST be set in the Flags field of SMB2 headers on all requests except the first one. The client MAY choose to send multiple requests required to perform a desired action as a compounded send containing related operations. Two examples would be to open a file and read from it, or to write to an open and close it. This form of compounding MUST NOT be used in combination with compounding unrelated requests within a single send.
---------------------------------------------------------

This means that we can only allow a request without the chained bit as the first request in a compound packet, and then all other requests must have the chained bit set, or all the requests in the compound packet must not have the chained bit set.

So if we ever see a chained bit in a compound requests, and then see a subsequent request in that compound *without* the chained bit, we can just bounce it immediately with INVALID_PARAMETER.

Jeremy.
Comment 8 Jeremy Allison 2013-02-16 04:45:01 UTC
Created attachment 8559 [details]
experimental patchset for 3.6.x

Ok, here is an experimental patchset for 3.6.x that should show you where I'm going with this.

I think we can get this to return the same error codes as W2K12, and then we should add a regression fix for master smbtorture smb2.compound tests to freeze the behaviour we expect and get these changes into master.

Jeremy.
Comment 9 Jeremy Allison 2013-02-16 04:46:23 UTC
Created attachment 8560 [details]
Wireshark trace against w2k8
Comment 10 Jeremy Allison 2013-02-16 04:46:54 UTC
Created attachment 8561 [details]
Wireshark trace against w2k12
Comment 11 Jeremy Allison 2013-02-20 00:52:32 UTC
Ok, I have an (ugly) patchset that makes up pass smb2.compound completely in 3.6.next. I'll clean up and submit to master and add to the bug here.

FYI. I finally got to test against W2K12 and the current code in master completes successfully against a W2K12 server, so I think that's the 'gold standard' we need to match.

Jeremy.
Comment 12 Stefan Metzmacher 2014-07-29 14:22:43 UTC
(In reply to comment #11)
> Ok, I have an (ugly) patchset that makes up pass smb2.compound completely in
> 3.6.next. I'll clean up and submit to master and add to the bug here.
> 
> FYI. I finally got to test against W2K12 and the current code in master
> completes successfully against a W2K12 server, so I think that's the 'gold
> standard' we need to match.

Jeremy, can we close this bug? I think this is fixed in current
Samba 4 releases...
Comment 13 Jeremy Allison 2014-07-29 23:21:24 UTC
Yeah, this one is dead... Fixed in 4.x.