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.
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; }
Created attachment 8526 [details] The capture Frames 295 and 303 show the problem.
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.
We had that check, but it seems it got lost in commit 52aa2612b73b43589f01d87a4852df28e44d0cfb
(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...
Ok, starting work on this one. I'll make sure we add a regression test for this. Jeremy.
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.
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.
Created attachment 8560 [details] Wireshark trace against w2k8
Created attachment 8561 [details] Wireshark trace against w2k12
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.
(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...
Yeah, this one is dead... Fixed in 4.x.