The branch, master has been updated
via d2d5fb1 libcli/smb: verify decrypted SMB2 pdus correctly
via 7a7e9b1 libcli/smb: fix parsing of compounded messages within a
SMB2_TRANSFORM pdu
via 84f6b0f libcli/smb: fix smb2cli_req_compound_submit for multiple
encrypted messages
via b596a11 s3:smb2_server: do calculations based on
SMBD_SMB2_NUM_IOV_PER_REQ in smbd_smb2_request_validate()
via 7ffee47 libcli/smb: all flags except SMB2_HDR_FLAG_ASYNC should be
cleared in a cancel request.
from 24b1143 s3-sysacls: Remove sys_acl_free_qualifier() as it is a no-op
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit d2d5fb1abfcb9d21fe2742d53de00c7638fad14d
Author: Stefan Metzmacher <me...@samba.org>
Date: Tue Aug 14 09:35:59 2012 +0200
libcli/smb: verify decrypted SMB2 pdus correctly
We need to make sure we got a encrypted response if we asked
for it.
If we don't get a encrypted response, we use a similar logic
as with signing to propagated wellknown errors to the higher
layer and set state->smb2.signing_skipped = true.
metze
Autobuild-User(master): Stefan Metzmacher <me...@samba.org>
Autobuild-Date(master): Wed Aug 15 16:26:26 CEST 2012 on sn-devel-104
commit 7a7e9b1c76f3967cc8cdae34e5d64759305e592a
Author: Stefan Metzmacher <me...@samba.org>
Date: Tue Aug 14 09:33:01 2012 +0200
libcli/smb: fix parsing of compounded messages within a SMB2_TRANSFORM pdu
One SMB2_TRANSFORM pdu wraps multiple SMB2 pdus.
We inject the SMB2_TRANSFORM header to each response which was wrapped
inside. This allows the next layer to verify if the SMB2 pdu was encrypted.
metze
commit 84f6b0f962a9106e0c108cdcd5eb5a1599cd8097
Author: Stefan Metzmacher <me...@samba.org>
Date: Tue Aug 14 09:30:43 2012 +0200
libcli/smb: fix smb2cli_req_compound_submit for multiple encrypted messages
There should be only one SMB2_TRANSFORM header for all compound requests.
metze
commit b596a116fd006bdc78bccef4dc5b9c9ad2807365
Author: Stefan Metzmacher <me...@samba.org>
Date: Wed Aug 15 14:43:40 2012 +0200
s3:smb2_server: do calculations based on SMBD_SMB2_NUM_IOV_PER_REQ in
smbd_smb2_request_validate()
metze
commit 7ffee47bc6cc2039a32a527e19e4a76c257fc6b0
Author: Stefan Metzmacher <me...@samba.org>
Date: Wed Aug 15 14:17:25 2012 +0200
libcli/smb: all flags except SMB2_HDR_FLAG_ASYNC should be cleared in a
cancel request.
metze
-----------------------------------------------------------------------
Summary of changes:
libcli/smb/smbXcli_base.c | 230 ++++++++++++++++++++++++++++++-------------
source3/smbd/smb2_server.c | 6 +-
2 files changed, 163 insertions(+), 73 deletions(-)
Changeset truncated at 500 lines:
diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index dad869c..45da5fd 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -243,6 +243,7 @@ struct smbXcli_req_state {
bool should_sign;
bool should_encrypt;
+ uint64_t encryption_session_id;
bool signing_skipped;
bool notify_async;
@@ -2422,6 +2423,12 @@ static bool smb2cli_req_cancel(struct tevent_req *req)
}
substate = tevent_req_data(subreq, struct smbXcli_req_state);
+ /*
+ * clear everything but the SMB2_HDR_FLAG_ASYNC flag
+ * e.g. if SMB2_HDR_FLAG_CHAINED is set we get INVALID_PARAMETER back
+ */
+ flags &= SMB2_HDR_FLAG_ASYNC;
+
if (flags & SMB2_HDR_FLAG_ASYNC) {
mid = 0;
}
@@ -2595,14 +2602,17 @@ NTSTATUS smb2cli_req_compound_submit(struct tevent_req
**reqs,
struct tevent_req *subreq;
struct iovec *iov;
int i, num_iov, nbt_len;
+ int tf_iov = -1;
+ const DATA_BLOB *encryption_key = NULL;
+ uint64_t encryption_session_id = 0;
/*
- * 1 for the nbt length
- * per request: TRANSFORM, HDR, fixed, dyn, padding
+ * 1 for the nbt length, optional TRANSFORM
+ * per request: HDR, fixed, dyn, padding
* -1 because the last one does not need padding
*/
- iov = talloc_array(reqs[0], struct iovec, 1 + 5*num_reqs - 1);
+ iov = talloc_array(reqs[0], struct iovec, 1 + 1 + 4*num_reqs - 1);
if (iov == NULL) {
return NT_STATUS_NO_MEMORY;
}
@@ -2610,8 +2620,65 @@ NTSTATUS smb2cli_req_compound_submit(struct tevent_req
**reqs,
num_iov = 1;
nbt_len = 0;
+ /*
+ * the session of the first request that requires encryption
+ * specifies the encryption key.
+ */
+ for (i=0; i<num_reqs; i++) {
+ if (!tevent_req_is_in_progress(reqs[i])) {
+ return NT_STATUS_INTERNAL_ERROR;
+ }
+
+ state = tevent_req_data(reqs[i], struct smbXcli_req_state);
+
+ if (!smbXcli_conn_is_connected(state->conn)) {
+ return NT_STATUS_CONNECTION_DISCONNECTED;
+ }
+
+ if ((state->conn->protocol != PROTOCOL_NONE) &&
+ (state->conn->protocol < PROTOCOL_SMB2_02)) {
+ return NT_STATUS_REVISION_MISMATCH;
+ }
+
+ if (state->session == NULL) {
+ continue;
+ }
+
+ if (!state->smb2.should_encrypt) {
+ continue;
+ }
+
+ encryption_key = &state->session->smb2->encryption_key;
+ if (encryption_key->length == 0) {
+ return NT_STATUS_INVALID_PARAMETER_MIX;
+ }
+
+ encryption_session_id = state->session->smb2->session_id;
+
+ tf_iov = num_iov;
+ iov[num_iov].iov_base = state->smb2.transform;
+ iov[num_iov].iov_len = sizeof(state->smb2.transform);
+ num_iov += 1;
+
+ SBVAL(state->smb2.transform, SMB2_TF_PROTOCOL_ID,
SMB2_TF_MAGIC);
+ SBVAL(state->smb2.transform, SMB2_TF_NONCE,
+ state->session->smb2->nonce_low);
+ SBVAL(state->smb2.transform, SMB2_TF_NONCE+8,
+ state->session->smb2->nonce_high);
+ SBVAL(state->smb2.transform, SMB2_TF_SESSION_ID,
+ encryption_session_id);
+
+ state->session->smb2->nonce_low += 1;
+ if (state->session->smb2->nonce_low == 0) {
+ state->session->smb2->nonce_high += 1;
+ state->session->smb2->nonce_low += 1;
+ }
+
+ nbt_len += SMB2_TF_HDR_SIZE;
+ break;
+ }
+
for (i=0; i<num_reqs; i++) {
- int tf_iov;
int hdr_iov;
size_t reqlen;
bool ret;
@@ -2621,7 +2688,6 @@ NTSTATUS smb2cli_req_compound_submit(struct tevent_req
**reqs,
uint16_t credits;
uint64_t mid;
const DATA_BLOB *signing_key = NULL;
- const DATA_BLOB *encryption_key = NULL;
if (!tevent_req_is_in_progress(reqs[i])) {
return NT_STATUS_INTERNAL_ERROR;
@@ -2681,7 +2747,7 @@ NTSTATUS smb2cli_req_compound_submit(struct tevent_req
**reqs,
SBVAL(state->smb2.hdr, SMB2_HDR_MESSAGE_ID, mid);
skip_credits:
- if (state->session) {
+ if (state->session && encryption_key == NULL) {
/*
* We prefer the channel signing key if it is
* already there.
@@ -2705,17 +2771,6 @@ skip_credits:
if (signing_key && signing_key->length == 0) {
signing_key = NULL;
}
-
- if (state->smb2.should_encrypt) {
- encryption_key =
&state->session->smb2->encryption_key;
- }
- }
-
- if (encryption_key) {
- tf_iov = num_iov;
- iov[num_iov].iov_base = state->smb2.transform;
- iov[num_iov].iov_len = sizeof(state->smb2.transform);
- num_iov += 1;
}
hdr_iov = num_iov;
@@ -2748,53 +2803,9 @@ skip_credits:
SIVAL(state->smb2.hdr, SMB2_HDR_NEXT_COMMAND, reqlen);
}
- if (encryption_key) {
- NTSTATUS status;
- uint8_t *buf;
- int vi;
-
- SBVAL(state->smb2.transform, SMB2_TF_NONCE,
- state->session->smb2->nonce_low);
- SBVAL(state->smb2.transform, SMB2_TF_NONCE+8,
- state->session->smb2->nonce_high);
-
- state->session->smb2->nonce_low += 1;
- if (state->session->smb2->nonce_low == 0) {
- state->session->smb2->nonce_high += 1;
- state->session->smb2->nonce_low += 1;
- }
-
- buf = talloc_array(iov, uint8_t, reqlen);
- if (buf == NULL) {
- return NT_STATUS_NO_MEMORY;
- }
-
- reqlen += SMB2_TF_HDR_SIZE;
-
- /*
- * We copy the buffers before encrypting them,
- * this is at least currently needed for the
- * to keep state->smb2.hdr.
- *
- * Also the callers may expect there buffers
- * to be const.
- */
- for (vi = hdr_iov; vi < num_iov; vi++) {
- struct iovec *v = &iov[vi];
- const uint8_t *o = (const uint8_t *)v->iov_base;
-
- memcpy(buf, o, v->iov_len);
- v->iov_base = (void *)buf;
- buf += v->iov_len;
- }
+ state->smb2.encryption_session_id = encryption_session_id;
- status = smb2_signing_encrypt_pdu(*encryption_key,
- state->session->conn->protocol,
- &iov[tf_iov], num_iov - tf_iov);
- if (!NT_STATUS_IS_OK(status)) {
- return status;
- }
- } else if (signing_key) {
+ if (signing_key != NULL) {
NTSTATUS status;
status = smb2_signing_sign_pdu(*signing_key,
@@ -2818,6 +2829,42 @@ skip_credits:
iov[0].iov_base = state->length_hdr;
iov[0].iov_len = sizeof(state->length_hdr);
+ if (encryption_key != NULL) {
+ NTSTATUS status;
+ size_t buflen = nbt_len - SMB2_TF_HDR_SIZE;
+ uint8_t *buf;
+ int vi;
+
+ buf = talloc_array(iov, uint8_t, buflen);
+ if (buf == NULL) {
+ return NT_STATUS_NO_MEMORY;
+ }
+
+ /*
+ * We copy the buffers before encrypting them,
+ * this is at least currently needed for the
+ * to keep state->smb2.hdr.
+ *
+ * Also the callers may expect there buffers
+ * to be const.
+ */
+ for (vi = tf_iov + 1; vi < num_iov; vi++) {
+ struct iovec *v = &iov[vi];
+ const uint8_t *o = (const uint8_t *)v->iov_base;
+
+ memcpy(buf, o, v->iov_len);
+ v->iov_base = (void *)buf;
+ buf += v->iov_len;
+ }
+
+ status = smb2_signing_encrypt_pdu(*encryption_key,
+ state->conn->protocol,
+ &iov[tf_iov], num_iov - tf_iov);
+ if (!NT_STATUS_IS_OK(status)) {
+ return status;
+ }
+ }
+
if (state->conn->dispatch_incoming == NULL) {
state->conn->dispatch_incoming = smb2cli_conn_dispatch_incoming;
}
@@ -2906,6 +2953,9 @@ static NTSTATUS smb2cli_inbuf_parse_compound(struct
smbXcli_conn *conn,
int num_iov = 0;
size_t taken = 0;
uint8_t *first_hdr = buf;
+ size_t verified_buflen = 0;
+ uint8_t *tf = NULL;
+ size_t tf_len = 0;
iov = talloc_array(mem_ctx, struct iovec, num_iov);
if (iov == NULL) {
@@ -2913,8 +2963,6 @@ static NTSTATUS smb2cli_inbuf_parse_compound(struct
smbXcli_conn *conn,
}
while (taken < buflen) {
- uint8_t *tf = NULL;
- size_t tf_len = 0;
size_t len = buflen - taken;
uint8_t *hdr = first_hdr + taken;
struct iovec *cur;
@@ -2923,6 +2971,13 @@ static NTSTATUS smb2cli_inbuf_parse_compound(struct
smbXcli_conn *conn,
uint16_t body_size;
struct iovec *iov_tmp;
+ if (verified_buflen > taken) {
+ len = verified_buflen - taken;
+ } else {
+ tf = NULL;
+ tf_len = 0;
+ }
+
if (len < 4) {
DEBUG(10, ("%d bytes left, expected at least %d\n",
(int)len, 4));
@@ -2973,6 +3028,8 @@ static NTSTATUS smb2cli_inbuf_parse_compound(struct
smbXcli_conn *conn,
TALLOC_FREE(iov);
return status;
}
+
+ verified_buflen = taken + len;
}
/*
@@ -3006,9 +3063,6 @@ static NTSTATUS smb2cli_inbuf_parse_compound(struct
smbXcli_conn *conn,
if (next_command_ofs > full_size) {
goto inval;
}
- if (tf && next_command_ofs < len) {
- goto inval;
- }
full_size = next_command_ofs;
}
if (body_size < 2) {
@@ -3105,6 +3159,7 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct
smbXcli_conn *conn,
uint32_t new_credits;
struct smbXcli_session *session = NULL;
const DATA_BLOB *signing_key = NULL;
+ bool was_encrypted = false;
new_credits = conn->smb2.cur_credits;
new_credits += credits;
@@ -3222,6 +3277,26 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct
smbXcli_conn *conn,
}
}
+ if (cur[0].iov_len == SMB2_TF_HDR_SIZE) {
+ const uint8_t *tf = (const uint8_t *)cur[0].iov_base;
+ uint64_t uid = BVAL(tf, SMB2_TF_SESSION_ID);
+
+ /*
+ * If the response was encrypted in a SMB2_TRANSFORM
+ * pdu, which belongs to the correct session,
+ * we do not need to do signing checks
+ *
+ * It could be the session the response belongs to
+ * or the session that was used to encrypt the
+ * SMB2_TRANSFORM request.
+ */
+ if ((session && session->smb2->session_id == uid) ||
+ (state->smb2.encryption_session_id == uid)) {
+ signing_key = NULL;
+ was_encrypted = true;
+ }
+ }
+
if (NT_STATUS_EQUAL(status, NT_STATUS_USER_SESSION_DELETED)) {
/*
* if the server returns NT_STATUS_USER_SESSION_DELETED
@@ -3229,9 +3304,24 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct
smbXcli_conn *conn,
* propagate the NT_STATUS_USER_SESSION_DELETED
* status to the caller.
*/
+ state->smb2.signing_skipped = true;
signing_key = NULL;
- } else if (state->smb2.should_encrypt) {
- if (cur[0].iov_len != SMB2_TF_HDR_SIZE) {
+ }
+
+ if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) {
+ /*
+ * if the server returns
+ * NT_STATUS_INVALID_PARAMETER
+ * the response might not be encrypted.
+ */
+ if (state->smb2.should_encrypt && !was_encrypted) {
+ state->smb2.signing_skipped = true;
+ signing_key = NULL;
+ }
+ }
+
+ if (state->smb2.should_encrypt && !was_encrypted) {
+ if (!state->smb2.signing_skipped) {
return NT_STATUS_ACCESS_DENIED;
}
}
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index a84776a..97739e5 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -648,7 +648,7 @@ static NTSTATUS smbd_smb2_request_validate(struct
smbd_smb2_request *req)
}
flags = IVAL(inhdr, SMB2_HDR_FLAGS);
- if (idx == 1) {
+ if (idx < SMBD_SMB2_NUM_IOV_PER_REQ) {
/*
* the 1st request should never have the
* SMB2_HDR_FLAG_CHAINED flag set
@@ -657,7 +657,7 @@ static NTSTATUS smbd_smb2_request_validate(struct
smbd_smb2_request *req)
req->next_status = NT_STATUS_INVALID_PARAMETER;
return NT_STATUS_OK;
}
- } else if (idx == 4) {
+ } else if (idx < 2*SMBD_SMB2_NUM_IOV_PER_REQ) {
/*
* the 2nd request triggers related vs. unrelated
* compounded requests
@@ -665,7 +665,7 @@ static NTSTATUS smbd_smb2_request_validate(struct
smbd_smb2_request *req)
if (flags & SMB2_HDR_FLAG_CHAINED) {
req->compound_related = true;
}
- } else if (idx > 4) {
+ } else {
#if 0
/*
* It seems the this tests are wrong
--
Samba Shared Repository
