[PATCH] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[PATCH] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Namjae Jeon]

This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
length exceeds maximum value in ksmbd_pdu_size_has_room().

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>
---
 fs/ksmbd/smb_common.c | 3 ++-
 fs/ksmbd/smb_common.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 5901b2884c60..ebc835ab414c 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -274,7 +274,8 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
 
 bool ksmbd_pdu_size_has_room(unsigned int pdu)
 {
-	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
+	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4 &&
+		pdu <= MAX_STREAM_PROT_LEN);
 }
 
 int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 994abede27e9..10b8d7224dfa 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,6 +48,8 @@
 #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
 #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
 
+#define MAX_STREAM_PROT_LEN	0x00FFFFFF
+
 /* Responses when opening a file. */
 #define F_SUPERSEDED	0
 #define F_OPENED	1
-- 
2.25.1

[PATCH v4] ksmbd: fix invalid request buffer access in compound

[Namjae Jeon]

Ronnie reported invalid request buffer access in chained command when
inserting garbage value to NextCommand of compound request.
This patch add validation check to avoid this issue.

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>
Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
  v2:
   - fix integer overflow from work->next_smb2_rcv_hdr_off.
  v3:
   - check next command offset and at least header size of next pdu at
     the same time.
  v4:
   - add next_cmd variable not to avoid repeat conversion.

 fs/ksmbd/smb2pdu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 90f867b9d560..301558a04298 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work)
 bool is_chained_smb2_message(struct ksmbd_work *work)
 {
 	struct smb2_hdr *hdr = work->request_buf;
-	unsigned int len;
+	unsigned int len, next_cmd;
 
 	if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
 		return false;
 
 	hdr = ksmbd_req_buf_next(work);
-	if (le32_to_cpu(hdr->NextCommand) > 0) {
+	next_cmd = le32_to_cpu(hdr->NextCommand);
+	if (next_cmd > 0) {
+		if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 >
+		    get_rfc1002_len(work->request_buf)) {
+			pr_err("next command(%u) offset exceeds smb msg size\n",
+			       next_cmd);
+			return false;
+		}
+
 		ksmbd_debug(SMB, "got SMB2 chained command\n");
 		init_chained_smb2_rsp(work);
 		return true;
-- 
2.25.1

[PATCH v3] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

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);
+	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;
 }
 
-- 
2.25.1

Re: [PATCH] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Tom Talpey]

On 9/22/2021 11:48 PM, Namjae Jeon wrote:
> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
> length exceeds maximum value in ksmbd_pdu_size_has_room().
> 
> 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>
> ---
>   fs/ksmbd/smb_common.c | 3 ++-
>   fs/ksmbd/smb_common.h | 2 ++
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index 5901b2884c60..ebc835ab414c 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -274,7 +274,8 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
>   
>   bool ksmbd_pdu_size_has_room(unsigned int pdu)
>   {
> -	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
> +	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4 &&
> +		pdu <= MAX_STREAM_PROT_LEN);

Incorrect. If the pdu is already 2^24-1 bytes long, there is no "room".

I don't think  a "<" fixes this either. One byte isn't sufficient to
allow any significant addition, in all likelihood.

What, exactly, is this check protecting?

Tom.

>   }
>   
>   int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
> index 994abede27e9..10b8d7224dfa 100644
> --- a/fs/ksmbd/smb_common.h
> +++ b/fs/ksmbd/smb_common.h
> @@ -48,6 +48,8 @@
>   #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
>   #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>   
> +#define MAX_STREAM_PROT_LEN	0x00FFFFFF
> +
>   /* Responses when opening a file. */
>   #define F_SUPERSEDED	0
>   #define F_OPENED	1
> 

Re: [PATCH v4] ksmbd: fix invalid request buffer access in compound

[Tom Talpey]

On 9/22/2021 11:48 PM, Namjae Jeon wrote:
> Ronnie reported invalid request buffer access in chained command when
> inserting garbage value to NextCommand of compound request.
> This patch add validation check to avoid this issue.
> 
> 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>
> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>    v2:
>     - fix integer overflow from work->next_smb2_rcv_hdr_off.
>    v3:
>     - check next command offset and at least header size of next pdu at
>       the same time.
>    v4:
>     - add next_cmd variable not to avoid repeat conversion.
> 
>   fs/ksmbd/smb2pdu.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 90f867b9d560..301558a04298 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work)
>   bool is_chained_smb2_message(struct ksmbd_work *work)
>   {
>   	struct smb2_hdr *hdr = work->request_buf;
> -	unsigned int len;
> +	unsigned int len, next_cmd;
>   
>   	if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
>   		return false;
>   
>   	hdr = ksmbd_req_buf_next(work);
> -	if (le32_to_cpu(hdr->NextCommand) > 0) {
> +	next_cmd = le32_to_cpu(hdr->NextCommand);
> +	if (next_cmd > 0) {
> +		if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 >

The "64" is somewhat arbitrary and mysterious. Is this the size
of the next command smb2_hdr? Why not code that, at least with
a #define?

> +		    get_rfc1002_len(work->request_buf)) {
> +			pr_err("next command(%u) offset exceeds smb msg size\n",
> +			       next_cmd);
> +			return false;
> +		}

Hmm, well the header fits in the reminder of the message. What
about the rest of the nextcommand smb2 request? It seems very
odd, and more than a little risky, to be validating only one
aspect of the nextcommand here.

> +
>   		ksmbd_debug(SMB, "got SMB2 chained command\n");

This, to me, is entirely needless debug splat.

Tom.

>   		init_chained_smb2_rsp(work);
>   		return true;
> 

Re: [PATCH v3] ksmbd: add validation in smb2 negotiate

[Tom Talpey]

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;
>   }
>   
> 

Re: [PATCH] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

[Namjae Jeon]

2021-09-24 0:05 GMT+09:00, Tom Talpey <tom@talpey.com>:
>
>
> On 9/22/2021 11:48 PM, Namjae Jeon wrote:
>> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
>> length exceeds maximum value in ksmbd_pdu_size_has_room().
>>
>> 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>
>> ---
>>   fs/ksmbd/smb_common.c | 3 ++-
>>   fs/ksmbd/smb_common.h | 2 ++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
>> index 5901b2884c60..ebc835ab414c 100644
>> --- a/fs/ksmbd/smb_common.c
>> +++ b/fs/ksmbd/smb_common.c
>> @@ -274,7 +274,8 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
>>
>>   bool ksmbd_pdu_size_has_room(unsigned int pdu)
>>   {
>> -	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
>> +	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4 &&
>> +		pdu <= MAX_STREAM_PROT_LEN);
>
> Incorrect. If the pdu is already 2^24-1 bytes long, there is no "room".
>
> I don't think  a "<" fixes this either. One byte isn't sufficient to
> allow any significant addition, in all likelihood.
>
> What, exactly, is this check protecting?
Ah, Sorry, The function name and comment seems to create a
misunderstood. It is to check the min/max size of pdu. If pdu size is
bigger than maximum stream protocol length (0x00FFFFFF), ksmbd don't
handle such invalid requests.

>
> Tom.
>
>>   }
>>
>>   int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int
>> info_level,
>> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
>> index 994abede27e9..10b8d7224dfa 100644
>> --- a/fs/ksmbd/smb_common.h
>> +++ b/fs/ksmbd/smb_common.h
>> @@ -48,6 +48,8 @@
>>   #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
>>   #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>>
>> +#define MAX_STREAM_PROT_LEN	0x00FFFFFF
>> +
>>   /* Responses when opening a file. */
>>   #define F_SUPERSEDED	0
>>   #define F_OPENED	1
>>
>

Re: [PATCH v4] ksmbd: fix invalid request buffer access in compound

[Namjae Jeon]

2021-09-24 0:13 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/22/2021 11:48 PM, Namjae Jeon wrote:
>> Ronnie reported invalid request buffer access in chained command when
>> inserting garbage value to NextCommand of compound request.
>> This patch add validation check to avoid this issue.
>>
>> 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>
>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>    v2:
>>     - fix integer overflow from work->next_smb2_rcv_hdr_off.
>>    v3:
>>     - check next command offset and at least header size of next pdu at
>>       the same time.
>>    v4:
>>     - add next_cmd variable not to avoid repeat conversion.
>>
>>   fs/ksmbd/smb2pdu.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 90f867b9d560..301558a04298 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -459,13 +459,21 @@ static void init_chained_smb2_rsp(struct ksmbd_work
>> *work)
>>   bool is_chained_smb2_message(struct ksmbd_work *work)
>>   {
>>   	struct smb2_hdr *hdr = work->request_buf;
>> -	unsigned int len;
>> +	unsigned int len, next_cmd;
>>
>>   	if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
>>   		return false;
>>
>>   	hdr = ksmbd_req_buf_next(work);
>> -	if (le32_to_cpu(hdr->NextCommand) > 0) {
>> +	next_cmd = le32_to_cpu(hdr->NextCommand);
>> +	if (next_cmd > 0) {
>> +		if ((u64)work->next_smb2_rcv_hdr_off + next_cmd + 64 >
>
> The "64" is somewhat arbitrary and mysterious. Is this the size
> of the next command smb2_hdr? Why not code that, at least with
> a #define?
Okay, Will use macro.
>
>> +		    get_rfc1002_len(work->request_buf)) {
>> +			pr_err("next command(%u) offset exceeds smb msg size\n",
>> +			       next_cmd);
>> +			return false;
>> +		}
>
> Hmm, well the header fits in the reminder of the message. What
> about the rest of the nextcommand smb2 request? It seems very
> odd, and more than a little risky, to be validating only one
> aspect of the nextcommand here.
There is a loop to check the rest of the nextcommand.
Please see do { } while (is_chained_smb2_message(work)); in server.

>
>> +
>>   		ksmbd_debug(SMB, "got SMB2 chained command\n");
>
> This, to me, is entirely needless debug splat.
The reason is ?
>
> Tom.
>
>>   		init_chained_smb2_rsp(work);
>>   		return true;
>>
>

Re: [PATCH v3] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

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;
>>   }
>>
>>
>