From 715f7c7f9e561ce4063f8165207404f7accd55cc Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 11 Jan 2021 10:01:39 +0100 Subject: [PATCH 1/4] libcli/smb: Change some checks to SMB_ASSERTS If we end up here, it's definitely a programming error in the basic parsing layer of the SMB2 packet. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14607 Signed-off-by: Stefan Metzmacher Reviewed-by: Volker Lendecke (cherry picked from commit fdcdfceefdd3186ef0b70bb6e83dddc8f4c073db) --- libcli/smb/smb2_signing.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/libcli/smb/smb2_signing.c b/libcli/smb/smb2_signing.c index cc03607d789..230475480c2 100644 --- a/libcli/smb/smb2_signing.c +++ b/libcli/smb/smb2_signing.c @@ -189,13 +189,8 @@ NTSTATUS smb2_signing_check_pdu(struct smb2_signing_key *signing_key, static const uint8_t zero_sig[16] = { 0, }; int i; - if (count < 2) { - return NT_STATUS_INVALID_PARAMETER; - } - - if (vector[0].iov_len != SMB2_HDR_BODY) { - return NT_STATUS_INVALID_PARAMETER; - } + SMB_ASSERT(count >= 2); + SMB_ASSERT(vector[0].iov_len == SMB2_HDR_BODY); hdr = (const uint8_t *)vector[0].iov_base; -- 2.27.0 From 49bfa47e4c10c4b52168b1400db59409be55ec45 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 6 Jan 2021 09:03:05 -0800 Subject: [PATCH 2/4] libcli/smb: Allow smb2cli_validate_negotiate_info_done() to ignore NT_STATUS_INVALID_PARAMETER. This can be returned from NetApp Ontap 7.3.7 SMB server implementations. Now we have ensured smb2_signing_check_pdu() cannot return NT_STATUS_INVALID_PARAMETER on a signing error it's safe to check this error code here. Windows 10 clients ignore this error from the NetApp. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14607 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke Reviewed-by: Stefan Metzmacher (cherry picked from commit 0abb5ca6b96c843909dea56d5594e334547ae90f) --- libcli/smb/smbXcli_base.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 0115cbbec18..e71f82456b2 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -5424,6 +5424,18 @@ static void smb2cli_validate_negotiate_info_done(struct tevent_req *subreq) &state->out_input_buffer, &state->out_output_buffer); TALLOC_FREE(subreq); + + /* + * This response must be signed correctly for + * these "normal" error codes to be processed. + * If the packet wasn't signed correctly we will get + * NT_STATUS_ACCESS_DENIED or NT_STATUS_HMAC_NOT_SUPPORTED, + * or NT_STATUS_INVALID_NETWORK_RESPONSE + * from smb2_signing_check_pdu(). + * + * We must never ignore the above errors here. + */ + if (NT_STATUS_EQUAL(status, NT_STATUS_FILE_CLOSED)) { /* * The response was signed, but not supported @@ -5469,6 +5481,19 @@ static void smb2cli_validate_negotiate_info_done(struct tevent_req *subreq) tevent_req_done(req); return; } + if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) { + /* + * The response was signed, but not supported + * + * This might be returned by NetApp Ontap 7.3.7 SMB server + * implementations. + * + * BUG: https://bugzilla.samba.org/show_bug.cgi?id=14607 + * + */ + tevent_req_done(req); + return; + } if (tevent_req_nterror(req, status)) { return; } -- 2.27.0 From 7b20ed60a4e19c33bcfafb1fcdc5bd3fe970b6f8 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 14 Jan 2021 17:27:21 +0100 Subject: [PATCH 3/4] libcli/smb: split out smb2cli_ioctl_parse_buffer() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14607 Pair-Programmed-With: Volker Lendecke Signed-off-by: Stefan Metzmacher Signed-off-by: Volker Lendecke (cherry picked from commit 508ed5b42c23f8b3d9730d838bd921cb73c61358) --- libcli/smb/smb2cli_ioctl.c | 190 +++++++++++++++++++++---------------- 1 file changed, 110 insertions(+), 80 deletions(-) diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c index 2b572baeb23..6368bd31bc2 100644 --- a/libcli/smb/smb2cli_ioctl.c +++ b/libcli/smb/smb2cli_ioctl.c @@ -160,6 +160,80 @@ struct tevent_req *smb2cli_ioctl_send(TALLOC_CTX *mem_ctx, return req; } +static NTSTATUS smb2cli_ioctl_parse_buffer(uint32_t dyn_offset, + const DATA_BLOB dyn_buffer, + uint32_t min_offset, + uint32_t buffer_offset, + uint32_t buffer_length, + uint32_t max_length, + uint32_t *next_offset, + DATA_BLOB *buffer) +{ + uint32_t offset; + bool oob; + + *buffer = data_blob_null; + *next_offset = dyn_offset; + + if (buffer_offset == 0) { + /* + * If the offset is 0, we better ignore + * the buffer_length field. + */ + return NT_STATUS_OK; + } + + if (buffer_length == 0) { + /* + * If the length is 0, we better ignore + * the buffer_offset field. + */ + return NT_STATUS_OK; + } + + SMB_ASSERT(min_offset >= dyn_offset); + if (buffer_offset != min_offset) { + return NT_STATUS_INVALID_NETWORK_RESPONSE; + } + + /* + * Make [input|output]_buffer_offset relative to "dyn_buffer" + */ + offset = buffer_offset - dyn_offset; + oob = smb_buffer_oob(dyn_buffer.length, offset, buffer_length); + if (oob) { + return NT_STATUS_INVALID_NETWORK_RESPONSE; + } + + /* + * Give the caller a hint what we consumed, + * the caller may need to add possible padding. + */ + *next_offset = buffer_offset + buffer_length; + + if (max_length == 0) { + /* + * If max_input_length is 0 we ignore the + * input_buffer_length, because Windows 2008 echos the + * DCERPC request from the requested input_buffer to + * the response input_buffer. + * + * We just use the same logic also for max_output_length... + */ + buffer_length = 0; + } + + if (buffer_length > max_length) { + return NT_STATUS_INVALID_NETWORK_RESPONSE; + } + + *buffer = (DATA_BLOB) { + .data = dyn_buffer.data + offset, + .length = buffer_length, + }; + return NT_STATUS_OK; +} + static void smb2cli_ioctl_done(struct tevent_req *subreq) { struct tevent_req *req = @@ -169,15 +243,19 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) tevent_req_data(req, struct smb2cli_ioctl_state); NTSTATUS status; + NTSTATUS error; struct iovec *iov; uint8_t *fixed; - uint8_t *dyn; - size_t dyn_len; + DATA_BLOB dyn_buffer = data_blob_null; uint32_t dyn_ofs = SMB2_HDR_BODY + 0x30; + uint32_t input_min_offset; uint32_t input_buffer_offset; uint32_t input_buffer_length; + uint32_t input_next_offset; + uint32_t output_min_offset; uint32_t output_buffer_offset; uint32_t output_buffer_length; + uint32_t output_next_offset; static const struct smb2cli_req_expected_response expected[] = { { .status = NT_STATUS_OK, @@ -247,92 +325,44 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) state->recv_iov = iov; fixed = (uint8_t *)iov[1].iov_base; - dyn = (uint8_t *)iov[2].iov_base; - dyn_len = iov[2].iov_len; + dyn_buffer = data_blob_const((uint8_t *)iov[2].iov_base, + iov[2].iov_len); input_buffer_offset = IVAL(fixed, 0x18); input_buffer_length = IVAL(fixed, 0x1C); output_buffer_offset = IVAL(fixed, 0x20); output_buffer_length = IVAL(fixed, 0x24); - if ((input_buffer_offset > 0) && (input_buffer_length > 0)) { - uint32_t ofs; - - if (input_buffer_offset != dyn_ofs) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - ofs = input_buffer_length; - ofs = NDR_ROUND(ofs, 8); - - if (state->max_input_length == 0) { - /* - * If max_input_length is 0 we ignore - * the input_buffer_length, because - * Windows 2008 echos the DCERPC request - * from the requested input_buffer - * to the response input_buffer. - */ - input_buffer_length = 0; - } - - if (input_buffer_length > dyn_len) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - if (input_buffer_length > state->max_input_length) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - state->out_input_buffer.data = dyn; - state->out_input_buffer.length = input_buffer_length; - - if (ofs > dyn_len) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - dyn_ofs += ofs; - dyn += ofs; - dyn_len -= ofs; + input_min_offset = dyn_ofs; + input_next_offset = dyn_ofs; + error = smb2cli_ioctl_parse_buffer(dyn_ofs, + dyn_buffer, + input_min_offset, + input_buffer_offset, + input_buffer_length, + state->max_input_length, + &input_next_offset, + &state->out_input_buffer); + if (tevent_req_nterror(req, error)) { + return; } - if ((output_buffer_offset > 0) && (output_buffer_length > 0)) { - if (output_buffer_offset != dyn_ofs) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - if (state->max_output_length == 0) { - /* - * We do the same logic as for - * max_input_length. - */ - output_buffer_length = 0; - } - - if (output_buffer_length > dyn_len) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - if (output_buffer_length > state->max_output_length) { - tevent_req_nterror( - req, NT_STATUS_INVALID_NETWORK_RESPONSE); - return; - } - - state->out_output_buffer.data = dyn; - state->out_output_buffer.length = output_buffer_length; + /* + * If output data is returned, the output offset MUST be set to + * InputOffset + InputCount rounded up to a multiple of 8. + */ + output_min_offset = NDR_ROUND(input_next_offset, 8); + output_next_offset = 0; /* this variable is completely ignored */ + error = smb2cli_ioctl_parse_buffer(dyn_ofs, + dyn_buffer, + output_min_offset, + output_buffer_offset, + output_buffer_length, + state->max_output_length, + &output_next_offset, + &state->out_output_buffer); + if (tevent_req_nterror(req, error)) { + return; } state->out_valid = true; -- 2.27.0 From 4c4c68be82189267e1dd343c3ee5e37fb52dc926 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 14 Jan 2021 17:32:15 +0100 Subject: [PATCH 4/4] libcli/smb: allow unexpected padding in SMB2 IOCTL responses A NetApp Ontap 7.3.7 SMB server add 8 padding bytes to an offset that's already 8 byte aligned. RN: Work around special SMB2 IOCTL response behavior of NetApp Ontap 7.3.7 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14607 Pair-Programmed-With: Volker Lendecke Signed-off-by: Stefan Metzmacher Signed-off-by: Volker Lendecke Autobuild-User(master): Volker Lendecke Autobuild-Date(master): Fri Jan 15 08:36:34 UTC 2021 on sn-devel-184 (cherry picked from commit 4c6c71e1378401d66bf2ed230544a75f7b04376f) --- libcli/smb/smb2cli_ioctl.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c index 6368bd31bc2..f9abcc57bab 100644 --- a/libcli/smb/smb2cli_ioctl.c +++ b/libcli/smb/smb2cli_ioctl.c @@ -191,8 +191,25 @@ static NTSTATUS smb2cli_ioctl_parse_buffer(uint32_t dyn_offset, return NT_STATUS_OK; } + if ((buffer_offset % 8) != 0) { + /* + * The offset needs to be 8 byte aligned. + */ + return NT_STATUS_INVALID_NETWORK_RESPONSE; + } + + /* + * We used to enforce buffer_offset to be + * an exact match of the expected minimum, + * but the NetApp Ontap 7.3.7 SMB server + * gets the padding wrong and aligns the + * input_buffer_offset by a value of 8. + * + * So we just enforce that the offset is + * not lower than the expected value. + */ SMB_ASSERT(min_offset >= dyn_offset); - if (buffer_offset != min_offset) { + if (buffer_offset < min_offset) { return NT_STATUS_INVALID_NETWORK_RESPONSE; } -- 2.27.0