diff mbox series

[v3] ksmbd: add validation in smb2 negotiate

Message ID 20210923034855.612832-3-linkinjeon@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v3] ksmbd: add validation in smb2 negotiate | expand

Commit Message

Namjae Jeon Sept. 23, 2021, 3:48 a.m. UTC
This patch add validation to check request buffer check in smb2
negotiate.

Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 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(-)

Comments

Tom Talpey Sept. 23, 2021, 3:54 p.m. UTC | #1
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@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   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;
>   }
>   
>
Namjae Jeon Sept. 23, 2021, 8:14 p.m. UTC | #2
2021-09-24 0:54 GMT+09:00, Tom Talpey <tom@talpey.com>:
> 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@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   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?
ksmbd_smb2_check_message() validate it.
>
> 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.
Okay, I have a patch for this. There was a small amount of code
modification, so I thought to apply it after the buffer check issues
are fixed.
>
>> +	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!
Ah, Right. This can be validated in deassemble_neg_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?
How do we know if the DialectCount of the next check is valid?
>
>> +			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.
negotiate contexts will be validated in deassemble_neg_contexts().
>
>> +			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.
< 3.1.1 doesn't have negotiate contexts. So It should be checked with
smb2_buf_len(rfc1002 len)
>
>> +			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.
This will be improved on another patch.
>
>> +	__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.
We need to validate DialectCount is valid first ?
>
>> +
>> +		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.
ByteCount should be checked in ksmbd_lookup_dialect_by_name().
Could you please give a idea how to validate ByteCount ?

Thanks for your review!
>
>> +
>> +		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;
>>   }
>>
>>
>
diff mbox series

Patch

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);
+	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) {
+			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+			rc = -EINVAL;
+			goto err_out;
+		}
+
+		if (smb2_neg_size > nego_ctxt_off) {
+			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) {
+			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) {
+			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);
+	__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;
+
+		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;
+
+		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;
 }