2021-09-24 0:54 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> 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;
>> }
>>
>>
>
