All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namjae Jeon <linkinjeon@kernel.org>
To: Ralph Boehme <slow@samba.org>
Cc: linux-cifs@vger.kernel.org,
	Ronnie Sahlberg <ronniesahlberg@gmail.com>,
	Steve French <smfrench@gmail.com>
Subject: Re: [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate
Date: Thu, 23 Sep 2021 08:13:05 +0900	[thread overview]
Message-ID: <CAKYAXd-+jfKqonB8=At-znCUBpF-J8Z3NfK=vvUJOfcUMFemoQ@mail.gmail.com> (raw)
In-Reply-To: <433e36b5-f07c-0105-fd30-c7bb4cb8957d@samba.org>

2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae
>
> patch looks great! Few nitpicks below.
>
> Am 22.09.21 um 00:51 schrieb Namjae Jeon:
>> This patch add validation to check request buffer check in smb2
>> negotiate.
>>
>> 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/smb2pdu.c    | 41 ++++++++++++++++++++++++++++++++++++++++-
>>   fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++--
>>   2 files changed, 60 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index baf7ce31d557..1fe37ad4e5bc 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
>>   	struct ksmbd_conn *conn = work->conn;
>>   	struct smb2_negotiate_req *req = work->request_buf;
>>   	struct smb2_negotiate_rsp *rsp = work->response_buf;
>> -	int rc = 0;
>> +	int rc = 0, smb2_buf_len, smb2_neg_size;
>
> I guess all len variables should use unsigned types to facilitate well
> defined overflow checks.
As Ronnie pointed out, if checking max stream size, will be no problem.
I'll fix it though.
>
>>   	__le32 status;
>>
>>   	ksmbd_debug(SMB, "Received negotiate request\n");
>> @@ -1089,6 +1089,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) {
>> +		int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
>> +		int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount);
>> +
>> +		if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) {
>
> overflow check needed for 32 bit arch?
Okay, will fix it on v3.
Thanks!
>
>> +			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 1da67217698d..da17b21ac685 100644
>> --- a/fs/ksmbd/smb_common.c
>> +++ b/fs/ksmbd/smb_common.c
>> @@ -229,13 +229,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);
>
> unsigned
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>

  reply	other threads:[~2021-09-22 23:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 22:51 [PATCH v2 1/3] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
2021-09-21 22:51 ` [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate Namjae Jeon
2021-09-22 14:17   ` Ralph Boehme
2021-09-22 23:13     ` Namjae Jeon [this message]
2021-09-23  0:12       ` ronnie sahlberg
2021-09-23  0:25         ` Namjae Jeon
2021-09-21 22:51 ` [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request Namjae Jeon
2021-09-22  0:39   ` ronnie sahlberg
2021-09-22  4:35     ` Namjae Jeon
2021-09-22  4:56       ` ronnie sahlberg
2021-09-22  5:35         ` Namjae Jeon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKYAXd-+jfKqonB8=At-znCUBpF-J8Z3NfK=vvUJOfcUMFemoQ@mail.gmail.com' \
    --to=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=ronniesahlberg@gmail.com \
    --cc=slow@samba.org \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.