All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ksmbd: fix transform header validation
@ 2021-09-29 13:36 Namjae Jeon
  2021-09-29 15:18 ` Tom Talpey
  0 siblings, 1 reply; 3+ messages in thread
From: Namjae Jeon @ 2021-09-29 13:36 UTC (permalink / raw)
  To: linux-cifs
  Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
	Steve French, Sergey Senozhatsky, Hyunchul Lee

OriginalMessageSize and SessionId should be used after validating
transform header in request buffer.

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>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index ec05d9dc6436..b06361313889 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -8455,16 +8455,8 @@ int smb3_decrypt_req(struct ksmbd_work *work)
 	unsigned int buf_data_size = pdu_length + 4 -
 		sizeof(struct smb2_transform_hdr);
 	struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf;
-	unsigned int orig_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
 	int rc = 0;
 
-	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
-	if (!sess) {
-		pr_err("invalid session id(%llx) in transform header\n",
-		       le64_to_cpu(tr_hdr->SessionId));
-		return -ECONNABORTED;
-	}
-
 	if (pdu_length + 4 <
 	    sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) {
 		pr_err("Transform message is too small (%u)\n",
@@ -8472,11 +8464,19 @@ int smb3_decrypt_req(struct ksmbd_work *work)
 		return -ECONNABORTED;
 	}
 
-	if (pdu_length + 4 < orig_len + sizeof(struct smb2_transform_hdr)) {
+	if (pdu_length + 4 <
+	    le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) {
 		pr_err("Transform message is broken\n");
 		return -ECONNABORTED;
 	}
 
+	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
+	if (!sess) {
+		pr_err("invalid session id(%llx) in transform header\n",
+		       le64_to_cpu(tr_hdr->SessionId));
+		return -ECONNABORTED;
+	}
+
 	iov[0].iov_base = buf;
 	iov[0].iov_len = sizeof(struct smb2_transform_hdr);
 	iov[1].iov_base = buf + sizeof(struct smb2_transform_hdr);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ksmbd: fix transform header validation
  2021-09-29 13:36 [PATCH] ksmbd: fix transform header validation Namjae Jeon
@ 2021-09-29 15:18 ` Tom Talpey
  2021-09-30  0:00   ` Namjae Jeon
  0 siblings, 1 reply; 3+ messages in thread
From: Tom Talpey @ 2021-09-29 15:18 UTC (permalink / raw)
  To: Namjae Jeon, linux-cifs
  Cc: Ronnie Sahlberg, Ralph Böhme, Steve French,
	Sergey Senozhatsky, Hyunchul Lee

On 9/29/2021 9:36 AM, Namjae Jeon wrote:
> OriginalMessageSize and SessionId should be used after validating
> transform header in request buffer.


I suggest rewording the log for clarity:

++ Validate that the transform and smb request headers are present
++ before checking OriginalMessageSize and SessionId fields.

Is there some reason you aren't using the buf_data_size that is
already calculated, to verify these offsets? It seems like a lot
of redundant, and therefore fragile, coding.

Reviewed-By: Tom Talpey <tom@talpey.com>


> 
> 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>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index ec05d9dc6436..b06361313889 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -8455,16 +8455,8 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>   	unsigned int buf_data_size = pdu_length + 4 -
>   		sizeof(struct smb2_transform_hdr);
>   	struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf;
> -	unsigned int orig_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
>   	int rc = 0;
>   
> -	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
> -	if (!sess) {
> -		pr_err("invalid session id(%llx) in transform header\n",
> -		       le64_to_cpu(tr_hdr->SessionId));
> -		return -ECONNABORTED;
> -	}
> -
>   	if (pdu_length + 4 <
>   	    sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) {
>   		pr_err("Transform message is too small (%u)\n",
> @@ -8472,11 +8464,19 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>   		return -ECONNABORTED;
>   	}
>   
> -	if (pdu_length + 4 < orig_len + sizeof(struct smb2_transform_hdr)) {
> +	if (pdu_length + 4 <
> +	    le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct smb2_transform_hdr)) {
>   		pr_err("Transform message is broken\n");
>   		return -ECONNABORTED;
>   	}
>   
> +	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
> +	if (!sess) {
> +		pr_err("invalid session id(%llx) in transform header\n",
> +		       le64_to_cpu(tr_hdr->SessionId));
> +		return -ECONNABORTED;
> +	}
> +
>   	iov[0].iov_base = buf;
>   	iov[0].iov_len = sizeof(struct smb2_transform_hdr);
>   	iov[1].iov_base = buf + sizeof(struct smb2_transform_hdr);
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ksmbd: fix transform header validation
  2021-09-29 15:18 ` Tom Talpey
@ 2021-09-30  0:00   ` Namjae Jeon
  0 siblings, 0 replies; 3+ messages in thread
From: Namjae Jeon @ 2021-09-30  0:00 UTC (permalink / raw)
  To: Tom Talpey
  Cc: linux-cifs, Ronnie Sahlberg, Ralph Böhme, Steve French,
	Sergey Senozhatsky, Hyunchul Lee

2021-09-30 0:18 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/29/2021 9:36 AM, Namjae Jeon wrote:
>> OriginalMessageSize and SessionId should be used after validating
>> transform header in request buffer.
>
>
> I suggest rewording the log for clarity:
>
> ++ Validate that the transform and smb request headers are present
> ++ before checking OriginalMessageSize and SessionId fields.
Looks more clear:) Thank you! I will update it.
>
> Is there some reason you aren't using the buf_data_size that is
> already calculated, to verify these offsets? It seems like a lot
> of redundant, and therefore fragile, coding.
I can't remember why I did this 3 years ago... I will change it on
another patch after checking it.
>
> Reviewed-By: Tom Talpey <tom@talpey.com>
Thanks for your review!
>
>
>>
>> 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>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index ec05d9dc6436..b06361313889 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -8455,16 +8455,8 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>>   	unsigned int buf_data_size = pdu_length + 4 -
>>   		sizeof(struct smb2_transform_hdr);
>>   	struct smb2_transform_hdr *tr_hdr = (struct smb2_transform_hdr *)buf;
>> -	unsigned int orig_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
>>   	int rc = 0;
>>
>> -	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
>> -	if (!sess) {
>> -		pr_err("invalid session id(%llx) in transform header\n",
>> -		       le64_to_cpu(tr_hdr->SessionId));
>> -		return -ECONNABORTED;
>> -	}
>> -
>>   	if (pdu_length + 4 <
>>   	    sizeof(struct smb2_transform_hdr) + sizeof(struct smb2_hdr)) {
>>   		pr_err("Transform message is too small (%u)\n",
>> @@ -8472,11 +8464,19 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>>   		return -ECONNABORTED;
>>   	}
>>
>> -	if (pdu_length + 4 < orig_len + sizeof(struct smb2_transform_hdr)) {
>> +	if (pdu_length + 4 <
>> +	    le32_to_cpu(tr_hdr->OriginalMessageSize) + sizeof(struct
>> smb2_transform_hdr)) {
>>   		pr_err("Transform message is broken\n");
>>   		return -ECONNABORTED;
>>   	}
>>
>> +	sess = ksmbd_session_lookup_all(conn, le64_to_cpu(tr_hdr->SessionId));
>> +	if (!sess) {
>> +		pr_err("invalid session id(%llx) in transform header\n",
>> +		       le64_to_cpu(tr_hdr->SessionId));
>> +		return -ECONNABORTED;
>> +	}
>> +
>>   	iov[0].iov_base = buf;
>>   	iov[0].iov_len = sizeof(struct smb2_transform_hdr);
>>   	iov[1].iov_base = buf + sizeof(struct smb2_transform_hdr);
>>
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-30  0:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29 13:36 [PATCH] ksmbd: fix transform header validation Namjae Jeon
2021-09-29 15:18 ` Tom Talpey
2021-09-30  0:00   ` Namjae Jeon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.