Re: [PATCH v2] ksmbd: add buffer validation in session setup

From: Namjae Jeon <linkinjeon@kernel.org>
To: Marios Makassikis <mmakassikis@freebox.fr>
Cc: CIFS <linux-cifs@vger.kernel.org>
Subject: Re: [PATCH v2] ksmbd: add buffer validation in session setup
Date: Tue, 19 Oct 2021 08:17:12 +0900	[thread overview]
Message-ID: <CAKYAXd9tAmRBMLEwpb6RCOEchL58JNgUWABEv5KAsZ+uAoFWDg@mail.gmail.com> (raw)
In-Reply-To: <CAF6XXKWCn2bSV4o=r0q7gO3H029y-jLZ4CReHABEEOwk6S4T_g@mail.gmail.com>

2021-10-18 23:49 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>:
> On Mon, Oct 18, 2021 at 4:04 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> Hi Marios,
>> > +     negblob_off = le16_to_cpu(req->SecurityBufferOffset);
>> > +     negblob_len = le16_to_cpu(req->SecurityBufferLength);
>> > +     if (negblob_off < (offsetof(struct smb2_sess_setup_req, Buffer) -
>> > 4))
>> > +             return -EINVAL;
>> Like the following code, negblob is still used without buffer check.
>> We need to add buffer check for it here ?
>>
>> if (negblob->MessageType == NtLmNegotiate) {
>>
>> } else if (negblob->MessageType == NtLmAuthenticate) {
>>
>> Thanks!
>>
> Hello Namjae,
>
> I'm not sure I understand what you mean. Should I change the check to
> something like this ?
>
> +       negblob_off = le16_to_cpu(req->SecurityBufferOffset);
> +       negblob_len = le16_to_cpu(req->SecurityBufferLength);
> +       if (negblob_off < (offsetof(struct smb2_sess_setup_req, Buffer) - 4)
> ||
> +           negblob_len < sizeof(struct negotiate_message))
negblob_len < offsetof(struct negotiate_message, NegotiateFlags))

Thanks!

>
> Marios
>
>> > +
>> >       negblob = (struct negotiate_message *)((char
>> > *)&req->hdr.ProtocolId +
>> > -                     le16_to_cpu(req->SecurityBufferOffset));
>> > +                     negblob_off);
>> >
>> > -     if (decode_negotiation_token(work, negblob) == 0) {
>> > +     if (decode_negotiation_token(conn, negblob, negblob_len) == 0) {
>> >               if (conn->mechToken)
>> >                       negblob = (struct negotiate_message
>> > *)conn->mechToken;
>> >       }
>> > @@ -1736,7 +1746,7 @@ int smb2_sess_setup(struct ksmbd_work *work)
>> >                       sess->Preauth_HashValue = NULL;
>> >               } else if (conn->preferred_auth_mech ==
>> > KSMBD_AUTH_NTLMSSP) {
>> >                       if (negblob->MessageType == NtLmNegotiate) {
>> > -                             rc = ntlm_negotiate(work, negblob);
>> > +                             rc = ntlm_negotiate(work, negblob,
>> > negblob_len);
>> >                               if (rc)
>> >                                       goto out_err;
>> >                               rsp->hdr.Status =
>> > --
>> > 2.25.1
>> >
>> >
>