* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).