ksmbd: missing validation of hdr->next_offset

Commit Message

Ronnie Sahlberg Sept. 21, 2021, 5:02 a.m. UTC

List, Namjae,

I have been looking at next_offset and we need some additional
validation of when we
walk to the next pdu without validating we have enough buffer.

I think the problem is down in is_chained_smb2_message()/
init_chained_smb2_rsp()
where it advances work->next_smb2_rcv_hdr_off without checking that
the new offset is valid and contains a smb2 header and payload.


A simple reproducer is libsmb2.
Apply this patch to break "next_offset"  for compounds in libsmb2 :

And then just run this command which uses compounds:
./examples/smb2-stat-sync smb://192.168.124.221/Share/
it should oops right away.


I don't know where the best place to check for this is,  either in
init_chained_smb2_rsp()
but it was a little hard to track it down.
A different place to check this might be from
ksmbd_conn_handler_loop()
and just walk the headers and check next_offset header by header
before even trying to process any of the pdus.


regards
ronnie shalberg

Comments

Namjae Jeon Sept. 21, 2021, 5:52 a.m. UTC | #1

2021-09-21 14:02 GMT+09:00, Leif Sahlberg <[e-mail]>:
> List, Namjae,
Hi Ronnie,
>
> I have been looking at next_offset and we need some additional
> validation of when we
> walk to the next pdu without validating we have enough buffer.
>
> I think the problem is down in is_chained_smb2_message()/
> init_chained_smb2_rsp()
> where it advances work->next_smb2_rcv_hdr_off without checking that
> the new offset is valid and contains a smb2 header and payload.
>
>
> A simple reproducer is libsmb2.
> Apply this patch to break "next_offset"  for compounds in libsmb2 :
> diff --git a/lib/pdu.c b/lib/pdu.c
> index 1329388..4eab8d7 100644
> --- a/lib/pdu.c
> +++ b/lib/pdu.c
> @@ -174,6 +174,7 @@ smb2_add_compound_pdu(struct smb2_context *smb2,
>          }
>
>          pdu->header.next_command = offset;
> +        pdu->header.next_command = 0x0f0ff0f0;
>          smb2_set_uint32(&pdu->out.iov[0], 20, pdu->header.next_command);
>
>          /* Fixup flags */
>
> And then just run this command which uses compounds:
> ./examples/smb2-stat-sync smb://192.168.124.221/Share/
> it should oops right away.
>
>
> I don't know where the best place to check for this is,  either in
> init_chained_smb2_rsp()
> but it was a little hard to track it down.
> A different place to check this might be from
> ksmbd_conn_handler_loop()
> and just walk the headers and check next_offset header by header
> before even trying to process any of the pdus.
Really thanks for your review! I will fix it now.

Thanks again!
>
>
> regards
> ronnie shalberg
>
>

Patch

diff --git a/lib/pdu.c b/lib/pdu.c
index 1329388..4eab8d7 100644
--- a/lib/pdu.c
+++ b/lib/pdu.c
@@ -174,6 +174,7 @@  smb2_add_compound_pdu(struct smb2_context *smb2,
         }

         pdu->header.next_command = offset;
+        pdu->header.next_command = 0x0f0ff0f0;
         smb2_set_uint32(&pdu->out.iov[0], 20, pdu->header.next_command);

         /* Fixup flags */