Re: [PATCH 4.4 13/74] cifs: fix out-of-bounds access in lease parsing

[Steve French]

On Tue, Mar 8, 2016 at 9:47 PM, Ben Hutchings <ben@xxxxxxxxxxxxxxx> wrote:
> On Mon, 2016-03-07 at 16:02 -0800, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Justin Maggard <jmaggard10@xxxxxxxxx>
>>
>> commit deb7deff2f00bdbbcb3d560dad2a89ef37df837d upstream.
>>
>> When opening a file, SMB2_open() attempts to parse the lease state from the
>> SMB2 CREATE Response.  However, the parsing code was not careful to ensure
>> that the create contexts are not empty or invalid, which can lead to out-
>> of-bounds memory access.  This can be seen easily by trying
>> to read a file from a OSX 10.11 SMB3 server.  Here is sample crash output:
>>
>> BUG: unable to handle kernel paging request at ffff8800a1a77cc6
>> IP: [] SMB2_open+0x804/0x960
>> PGD 8f77067 PUD 0
>> Oops: 0000 [#1] SMP
>> Modules linked in:
>> CPU: 3 PID: 2876 Comm: cp Not tainted 4.5.0-rc3.x86_64.1+ #14
>> Hardware name: NETGEAR ReadyNAS 314          /ReadyNAS 314          , BIOS 4.6.5 10/11/2012
>> task: ffff880073cdc080 ti: ffff88005b31c000 task.ti: ffff88005b31c000
>> RIP: 0010:[]  [] SMB2_open+0x804/0x960
>> RSP: 0018:ffff88005b31fa08  EFLAGS: 00010282
>> RAX: 0000000000000015 RBX: 0000000000000000 RCX: 0000000000000006
>> RDX: 0000000000000000 RSI: 0000000000000246 RDI: ffff88007eb8c8b0
>> RBP: ffff88005b31fad8 R08: 666666203d206363 R09: 6131613030383866
>> R10: 3030383866666666 R11: 00000000000002b0 R12: ffff8800660fd800
>> R13: ffff8800a1a77cc2 R14: 00000000424d53fe R15: ffff88005f5a28c0
>> FS:  00007f7c8a2897c0(0000) GS:ffff88007eb80000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>> CR2: ffff8800a1a77cc6 CR3: 000000005b281000 CR4: 00000000000006e0
>> Stack:
>>  ffff88005b31fa70 ffffffff88278789 00000000000001d3 ffff88005f5a2a80
>>  ffffffff00000003 ffff88005d029d00 ffff88006fde05a0 0000000000000000
>>  ffff88005b31fc78 ffff88006fde0780 ffff88005b31fb2f 0000000100000fe0
>> Call Trace:
>>  [] ? cifsConvertToUTF16+0x159/0x2d0
>>  [] smb2_open_file+0x98/0x210
>>  [] ? __kmalloc+0x1c/0xe0
>>  [] cifs_open+0x2a4/0x720
>>  [] do_dentry_open+0x1ff/0x310
>>  [] ? cifsFileInfo_get+0x30/0x30
>>  [] vfs_open+0x52/0x60
>>  [] path_openat+0x170/0xf70
>>  [] ? remove_wait_queue+0x48/0x50
>>  [] do_filp_open+0x79/0xd0
>>  [] ? __alloc_fd+0x3a/0x170
>>  [] do_sys_open+0x114/0x1e0
>>  [] SyS_open+0x19/0x20
>>  [] entry_SYSCALL_64_fastpath+0x12/0x6a
>> Code: 4d 8d 6c 07 04 31 c0 4c 89 ee e8 47 6f e5 ff 31 c9 41 89 ce 44 89 f1 48 c7 c7 28 b1 bd 88 31 c0 49 01 cd 4c 89 ee e8 2b 6f e5 ff <45> 0f b7 75 04 48 c7 c7 31 b1 bd 88 31 c0 4d 01 ee 4c 89 f6 e8
>> RIP  [] SMB2_open+0x804/0x960
>>  RSP
>> CR2: ffff8800a1a77cc6
>> ---[ end trace d9f69ba64feee469 ]---
>>
>> Signed-off-by: Justin Maggard <jmaggard@xxxxxxxxxxx>
>> Signed-off-by: Steve French <smfrench@xxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>
>> ---
>>  fs/cifs/smb2pdu.c |   24 ++++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -1109,21 +1109,25 @@ parse_lease_state(struct TCP_Server_Info
>>  {
>>       char *data_offset;
>>       struct create_context *cc;
>> -     unsigned int next = 0;
>> +     unsigned int next;
>> +     unsigned int remaining;
>>       char *name;
>>
>>       data_offset = (char *)rsp + 4 + le32_to_cpu(rsp->CreateContextsOffset);
>> +     remaining = le32_to_cpu(rsp->CreateContextsLength);
>
> What if remaining is > the response length?

Do you want to do the followon patch to check for that, or do you want me
to write up a small patch for that?

>>       cc = (struct create_context *)data_offset;
>> -     do {
>> -             cc = (struct create_context *)((char *)cc + next);
>> +     while (remaining >= sizeof(struct create_context)) {
>>               name = le16_to_cpu(cc->NameOffset) + (char *)cc;
>> -             if (le16_to_cpu(cc->NameLength) != 4 ||
>> -                 strncmp(name, "RqLs", 4)) {
>> -                     next = le32_to_cpu(cc->Next);
>> -                     continue;
>> -             }
>> -             return server->ops->parse_lease_buf(cc, epoch);
>> -     } while (next != 0);
>> +             if (le16_to_cpu(cc->NameLength) == 4 &&
>> +                 strncmp(name, "RqLs", 4) == 0)
>> +                     return server->ops->parse_lease_buf(cc, epoch);
>> +
>> +             next = le32_to_cpu(cc->Next);
>> +             if (!next)
>> +                     break;
>> +             remaining -= next;
>
> What if next > remaining?
>
> This change seems to be only scratching the surface of the security
> failure here.
>
> Ben.
>
>> +             cc = (struct create_context *)((char *)cc + next);
>> +     }
>>
>>       return 0;
>>  }
>
> --
> Ben Hutchings
> When in doubt, use brute force. - Ken Thompson



-- 
Thanks,

Steve