On 9/22/2021 11:48 PM, Namjae Jeon wrote:
This patch add validation to check request buffer check in smb2
negotiate.
Cc: Tom Talpey <tom@xxxxxxxxxx>
Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
Cc: Ralph Böhme <slow@xxxxxxxxx>
Cc: Steve French <smfrench@xxxxxxxxx>
Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
---
v3:
- fix integer(nego_ctxt_off) overflow issue.
- change data type of variables to unsigned.
fs/ksmbd/smb2pdu.c | 40 ++++++++++++++++++++++++++++++++++++++++
fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++--
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 301558a04298..2f9719a3f089 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1080,6 +1080,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
struct smb2_negotiate_req *req = work->request_buf;
struct smb2_negotiate_rsp *rsp = work->response_buf;
int rc = 0;
+ unsigned int smb2_buf_len, smb2_neg_size;
__le32 status;
ksmbd_debug(SMB, "Received negotiate request\n");
@@ -1097,6 +1098,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
goto err_out;
}
+ smb2_buf_len = get_rfc1002_len(work->request_buf);
Where is it validated that the pdu actually contains the number
of bytes in the DirectTCP header?
Honestly I don't understand why the 4 bytes are passed around at all.
Normally I would expect these to be stripped off after validation by
the lower-layer receive processing. This would simplify the gazillion
"- 4" corrections all over the code, too.
+ smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4;
+ if (conn->dialect == SMB311_PROT_ID) {
+ unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
+ unsigned int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount);
+
+ if (smb2_buf_len < (u64)nego_ctxt_off + nego_ctxt_count) {
This seems to be wrong. nego_ctxt_off is the base offset, but the
nego_ctxt_count is the number, not the length of the contexts!
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size > nego_ctxt_off) {
Isn't this completely redundant with the next test?
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ nego_ctxt_off) {
This validates that all the dialects are present, but it does not
account for the padding and negotiate contexts fields which follow
them.
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ } else {
+ if (smb2_neg_size > smb2_buf_len) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb2_buf_len) {
Same connects as the 3.1.1 validation above.
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ }
+
conn->cli_cap = le32_to_cpu(req->Capabilities);
switch (conn->dialect) {
case SMB311_PROT_ID:
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index ebc835ab414c..02fe2a06dda9 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -235,13 +235,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count)
static int ksmbd_negotiate_smb_dialect(void *buf)
{
- __le32 proto;
+ int smb_buf_length = get_rfc1002_len(buf);
Same comments as above on length field and passed buffer size.
+ __le32 proto = ((struct smb2_hdr *)buf)->ProtocolId;
- proto = ((struct smb2_hdr *)buf)->ProtocolId;
if (proto == SMB2_PROTO_NUMBER) {
struct smb2_negotiate_req *req;
+ int smb2_neg_size =
+ offsetof(struct smb2_negotiate_req, Dialects) - 4;
req = (struct smb2_negotiate_req *)buf;
+ if (smb2_neg_size > smb_buf_length)
+ goto err_out;
What is this test protecting? neg_size is the length of the pdu *before*
the Dialects field.
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb_buf_length)
+ goto err_out;
+
return ksmbd_lookup_dialect_by_id(req->Dialects,
req->DialectCount);
}
@@ -251,10 +260,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf)
struct smb_negotiate_req *req;
req = (struct smb_negotiate_req *)buf;
+ if (le16_to_cpu(req->ByteCount) < 2)
+ goto err_out;
So, this is SMB1-style negotiation, and it's very unclear if the
ByteCount is being checked for overflow. The code in mainline is
very different, and it's not clear what this patch applies to.
+
+ if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 +
+ le16_to_cpu(req->ByteCount) > smb_buf_length) {
+ goto err_out;
+ }
+
return ksmbd_lookup_dialect_by_name(req->DialectsArray,
req->ByteCount);
}
+err_out:
return BAD_PROT_ID;
}
