From a5b0b2e19070644bd42fb726c242387b3d66eda6 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 15 Feb 2013 20:20:48 -0800 Subject: [PATCH 1/5] Remove compound_related field in struct smbd_smb2_request. This only ever mirrored the state of sconn->smb2.compound_related_in_progress and so serves no purpose. Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 1 - source3/smbd/smb2_server.c | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index e227295..68f1668 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -365,7 +365,6 @@ struct smbd_smb2_request { bool do_signing; bool async; bool cancelled; - bool compound_related; /* fake smb1 request. */ struct smb_request *smb1req; diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 4764b83..866cf32 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -984,7 +984,6 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req, * state, and this is no longer a compound * request. */ - req->compound_related = false; req->sconn->smb2.compound_related_in_progress = false; } @@ -1364,7 +1363,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) } if (flags & SMB2_HDR_FLAG_CHAINED) { - req->compound_related = true; req->sconn->smb2.compound_related_in_progress = true; } @@ -1833,10 +1831,7 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req) return NT_STATUS_OK; } - if (req->compound_related) { - req->compound_related = false; - req->sconn->smb2.compound_related_in_progress = false; - } + req->sconn->smb2.compound_related_in_progress = false; smb2_setup_nbt_length(req->out.vector, req->out.vector_count); -- 1.7.10.4 From d64cccfa8998287e14e640760ab25b0332e81c3e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 15 Feb 2013 20:26:09 -0800 Subject: [PATCH 2/5] Move compound_related_in_progress setting to the SMB2_HDR_FLAG_CHAINED check. Simplifies the code. Signed-off-by: Jeremy Allison --- source3/smbd/smb2_server.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 866cf32..0837500 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -1340,6 +1340,7 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) if (!NT_STATUS_IS_OK(session_status)) { return smbd_smb2_request_error(req, NT_STATUS_INVALID_PARAMETER); } + req->sconn->smb2.compound_related_in_progress = true; } else { req->compat_chain_fsp = NULL; } @@ -1362,10 +1363,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) return smbd_smb2_request_error(req, NT_STATUS_ACCESS_DENIED); } - if (flags & SMB2_HDR_FLAG_CHAINED) { - req->sconn->smb2.compound_related_in_progress = true; - } - switch (opcode) { case SMB2_OP_NEGPROT: /* This call needs to be run as root */ -- 1.7.10.4 From b826d59703811ed45ee825c9a417ba6cf8010cfb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 15 Feb 2013 20:31:32 -0800 Subject: [PATCH 3/5] Fix checks around SMB2_HDR_FLAG_CHAINED to match Windows. --------------------------------------------------------- 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. Signed-off-by: Jeremy Allison --- source3/smbd/smb2_server.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 0837500..4e168b7 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -1333,6 +1333,14 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) session_status = smbd_smb2_request_check_session(req); if (flags & SMB2_HDR_FLAG_CHAINED) { + /* First request in a compound can't have chain flag set. */ + if (i == 1) { + return smbd_smb2_request_error(req, NT_STATUS_INVALID_PARAMETER); + } + /* Can't flip from non-chained to chained in the same compound. */ + if (i > 4 && !req->sconn->smb2.compound_related_in_progress) { + return smbd_smb2_request_error(req, NT_STATUS_INVALID_PARAMETER); + } /* * This check is mostly for giving the correct error code * for compounded requests. @@ -1342,6 +1350,13 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) } req->sconn->smb2.compound_related_in_progress = true; } else { + /* + * Once we've seen SMB2_HDR_FLAG_CHAINED it + * must be set on all requests in the compound. + */ + if (req->sconn->smb2.compound_related_in_progress) { + return smbd_smb2_request_error(req, NT_STATUS_INVALID_PARAMETER); + } req->compat_chain_fsp = NULL; } -- 1.7.10.4 From 80f40786c968525c06fd29e87ea2aad4f8dfa542 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 15 Feb 2013 20:34:01 -0800 Subject: [PATCH 4/5] Now we have the chained checks set correctly we no longer need to map the session error. Signed-off-by: Jeremy Allison --- source3/smbd/smb2_server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 4e168b7..9465e51 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -1346,7 +1346,7 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req) * for compounded requests. */ if (!NT_STATUS_IS_OK(session_status)) { - return smbd_smb2_request_error(req, NT_STATUS_INVALID_PARAMETER); + return smbd_smb2_request_error(req, session_status); } req->sconn->smb2.compound_related_in_progress = true; } else { -- 1.7.10.4 From 0fe2e9424752fbc7acd02c2e0c96ad23376bb628 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 15 Feb 2013 20:41:57 -0800 Subject: [PATCH 5/5] ***EXPERIMENTAL**** I think Windows goes out of a related operations state after a lookup of session/tcon or fileid fails. This would explain why subsequent operations even in the same chain return INVALID_PARAMETER. Signed-off-by: Jeremy Allison --- source3/smbd/files.c | 1 + source3/smbd/smb2_sesssetup.c | 1 + source3/smbd/smb2_tcon.c | 1 + 3 files changed, 3 insertions(+) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 58c24a8..ebad1d4 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -602,6 +602,7 @@ struct files_struct *file_fsp_smb2(struct smbd_smb2_request *smb2req, } if (volatile_id > UINT16_MAX) { + smb2req->sconn->smb2.compound_related_in_progress = false; return NULL; } diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c index 1f48e33..39283fc 100644 --- a/source3/smbd/smb2_sesssetup.c +++ b/source3/smbd/smb2_sesssetup.c @@ -840,6 +840,7 @@ NTSTATUS smbd_smb2_request_check_session(struct smbd_smb2_request *req) /* lookup an existing session */ p = idr_find(req->sconn->smb2.sessions.idtree, in_session_id); if (p == NULL) { + req->sconn->smb2.compound_related_in_progress = false; return NT_STATUS_USER_SESSION_DELETED; } session = talloc_get_type_abort(p, struct smbd_smb2_session); diff --git a/source3/smbd/smb2_tcon.c b/source3/smbd/smb2_tcon.c index 5f0e3a9..31f97f6 100644 --- a/source3/smbd/smb2_tcon.c +++ b/source3/smbd/smb2_tcon.c @@ -303,6 +303,7 @@ NTSTATUS smbd_smb2_request_check_tcon(struct smbd_smb2_request *req) /* lookup an existing session */ p = idr_find(req->session->tcons.idtree, in_tid); if (p == NULL) { + req->sconn->smb2.compound_related_in_progress = false; return NT_STATUS_NETWORK_NAME_DELETED; } tcon = talloc_get_type_abort(p, struct smbd_smb2_tcon); -- 1.7.10.4