Bug 15306 - Floating point exception (FPE) via cli_pull_send at source3/libsmb/clireadwrite.c
Summary: Floating point exception (FPE) via cli_pull_send at source3/libsmb/clireadwri...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.18.0rc2
Hardware: All All
: P5 critical with 5 votes (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
: 15305 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-02-09 16:00 UTC by fouzhe
Modified: 2023-05-11 07:09 UTC (History)
1 user (show)

See Also:


Attachments
a network trace of the bug that makes smbd crash and smbclient FPE (36.53 KB, patch)
2023-03-15 13:28 UTC, fouzhe
no flags Details
smbd log, the server command is "/usr/local/samba/sbin/smbd -i &> smbd.out" (266.51 KB, patch)
2023-03-15 13:30 UTC, fouzhe
no flags Details
The network trace of the bug that makes smbd crash and smbclient FPE (20.59 KB, patch)
2023-03-15 13:40 UTC, fouzhe
no flags Details
"raw" fix for master. (517 bytes, patch)
2023-03-20 22:26 UTC, Jeremy Allison
no flags Details
"raw" fix for master. (737 bytes, patch)
2023-03-21 16:44 UTC, Jeremy Allison
no flags Details
git-am fix for master. (6.25 KB, patch)
2023-03-21 17:38 UTC, Jeremy Allison
no flags Details
git-am fix for 4.18.next (6.68 KB, patch)
2023-03-29 22:45 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.17.next (5.70 KB, patch)
2023-03-29 22:56 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description fouzhe 2023-02-09 16:00:36 UTC
I found a FPE bug and here is the STDERR of smbclient:
```
Unknown parameter encountered: "challenge"
Ignoring unknown parameter "challenge"
lpcfg_do_global_parameter: WARNING: The "lanman auth" option is deprecated
lpcfg_do_global_parameter: WARNING: The "client lanman auth" option is deprecated
lpcfg_do_global_parameter: WARNING: The "client NTLMv2 auth" option is deprecated
lpcfg_do_global_parameter: WARNING: The "raw NTLMv2 auth" option is deprecated
AddressSanitizer:DEADLYSIGNAL
=================================================================
==24749==ERROR: AddressSanitizer: FPE on unknown address 0x7f37f746e97f (pc 0x7f37f746e97f bp 0x7ffeb0889370 sp 0x7ffeb08892a0 T0)
    #0 0x7f37f746e97f in cli_pull_send ../../source3/libsmb/clireadwrite.c:379
    #1 0x7f37f746ee58 in cli_pull ../../source3/libsmb/clireadwrite.c:686
    #2 0x5593e36332ae in do_get ../../source3/client/client.c:1119
    #3 0x5593e3644076 in cmd_get ../../source3/client/client.c:1197
    #4 0x5593e364b36d in process_stdin ../../source3/client/client.c:6155
    #5 0x5593e364b36d in process ../../source3/client/client.c:6200
    #6 0x5593e364b36d in main ../../source3/client/client.c:6724
    #7 0x7f37f58fed09 in __libc_start_main ../csu/libc-start.c:308
    #8 0x5593e362f459 in _start (/usr/local/samba/bin/smbclient+0x1d459)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE ../../source3/libsmb/clireadwrite.c:379 in cli_pull_send
==24749==ABORTING
```

The cli_pull_send function in clireadwrite.c (https://gitlab.com/samba-team/samba/-/blob/master/source3/libsmb/clireadwrite.c) does not check `state->chunk_size` before dividing it in line 379:

```c
tmp64 = window_size/state->chunk_size;
```
Comment 1 Douglas Bagnall 2023-02-09 19:02:46 UTC
*** Bug 15305 has been marked as a duplicate of this bug. ***
Comment 2 fouzhe 2023-02-10 00:12:23 UTC
(In reply to Douglas Bagnall from comment #1)
Sorry, a duplicate error report was submitted due to network delays.
Comment 3 fouzhe 2023-03-11 08:10:03 UTC
Hi, what is the status of this bug?
Comment 4 Volker Lendecke 2023-03-11 12:11:58 UTC
I can't really see how this can happen. Can you send a network trace of the full SMB client connection that leads to this? See https://wiki.samba.org/index.php/Capture_Packets for information on how to get the trace.
Comment 5 fouzhe 2023-03-15 13:28:16 UTC
Created attachment 17823 [details]
a network trace of the bug that makes smbd crash and smbclient FPE
Comment 6 fouzhe 2023-03-15 13:30:26 UTC
Created attachment 17824 [details]
smbd log, the server command is "/usr/local/samba/sbin/smbd -i &> smbd.out"
Comment 7 fouzhe 2023-03-15 13:40:52 UTC
Created attachment 17825 [details]
The network trace of the bug that makes smbd crash and smbclient FPE
Comment 8 fouzhe 2023-03-15 13:45:07 UTC
(In reply to Volker Lendecke from comment #4)
Hi Volker,
Sorry for being late. I triggered this bug via man-in-the-middle fuzzing and I did not record the network trace when fuzzing since the traffic can be massive. Therefore, it is hard for me to provide the network trace. I tried to reproduce this bug these days by following the recorded fuzzing log. Fortunately, I managed to trigger this bug and record the network trace as well as smbd's log, as attached. I found that the smbd also crashed.
The network structure of the smbclient, smbd and our fuzzer is: smbclient(46806) <=> (1399)fuzzer(42028) <=> (139)smbd.
Comment 9 Jeremy Allison 2023-03-20 21:02:59 UTC
Ah, I see how it can happen.

If a malicious server returns a max_read_size of 0 in smbXcli_negprot_smb2_done()
in this section (SMB2):

        conn->smb2.server.capabilities  = IVAL(body, 24);
        conn->smb2.server.max_trans_size= IVAL(body, 28);
        conn->smb2.server.max_read_size = IVAL(body, 32);
        conn->smb2.server.max_write_size= IVAL(body, 36);
        conn->smb2.server.system_time   = BVAL(body, 40);
        conn->smb2.server.start_time    = BVAL(body, 48);

or in this section (SMB1) in cli_read_max_bufsize()

min_space = cli_state_available_size(cli, data_offset);

then we use it unmodified in:

tmp64 = window_size/state->chunk_size;

I think a simple fix is just to check state->chunk_size inside

cli_pull_send()

and return NT_STATUS_INVALID_PARAMETER there.
Comment 10 Jeremy Allison 2023-03-20 22:26:29 UTC
Created attachment 17836 [details]
"raw" fix for master.

This is reproducible if you set "smb2 max read = 0" in the [global] section of the smb.conf. Working on a regression test now.
Comment 11 Volker Lendecke 2023-03-21 07:15:15 UTC
(In reply to Jeremy Allison from comment #10)
> This is reproducible if you set "smb2 max read = 0" in the [global] section
> of the smb.conf. Working on a regression test now.

Does it make sense to fully connect to such a server at all? Not being able to read (or write or ioctl) at all is pretty limiting for a SMB connection. I'd rather check this in the negprot reply.
Comment 12 fouzhe 2023-03-21 08:00:08 UTC
(In reply to Volker Lendecke from comment #11)
On one hand, setting up a server in this way could cause the client to crash. On the other hand, I did not actually set "smb2 max read = 0" during the test, but instead triggered the bug by manipulating the exchanged packets. As a result, the server also crashed, indicating that a malicious client could be used to perform a Denial-of-Service attack on the server.
Comment 13 fouzhe 2023-03-21 08:07:18 UTC
The configure file `/usr/local/samba/etc/smb.conf` I used is
```
[global]
    workgroup = WG1
    security = user
    map to guest = Bad User
    log file = /usr/local/samba/var/log.%m
    # max log size = 50
    unix charset = UTF-8
#display charset = UTF-8
    guest account = nobody
    dos charset = cp936
    create mask = 777
    directory mask = 777
    log level = 10
    challenge = 12345678abcdef00
    lanman auth = no
    ntlm auth = disabled
    client lanman auth = no
    client NTLMv2 auth = no
    raw NTLMv2 auth = no
[sharedir]
    path = /mount/
    browseable = yes
    guest ok = yes
    writable = yes
    read only = no
    public = yes
    directory mode = 0777
    create mode = 0770
```
Comment 14 Jeremy Allison 2023-03-21 16:22:17 UTC
(In reply to fouzhe from comment #12)

I see no evidence in the logs that the server crashed at all.

I think Volker is correct, we should not connect to a server that returns zero in the negprot.
Comment 15 Jeremy Allison 2023-03-21 16:38:20 UTC
Ah ! We are already safe from this in SMB1. In smbXcli_negprot_smb1_done(), we have:

       if (server_max_xmit < 1024) {
                tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
                return;
        }

so adding the same to SMB1 is logical.
Comment 16 Jeremy Allison 2023-03-21 16:44:19 UTC
Created attachment 17837 [details]
"raw" fix for master.

Fix in smb2 negprot instead.
Comment 17 Jeremy Allison 2023-03-21 17:38:49 UTC
Created attachment 17838 [details]
git-am fix for master.

Includes regression test. Running under ci. I'll update with the MR if this passes ci.
Comment 18 Jeremy Allison 2023-03-21 19:01:28 UTC
MR: https://gitlab.com/samba-team/samba/-/merge_requests/2988
Comment 19 fouzhe 2023-03-24 03:56:38 UTC
(In reply to Jeremy Allison from comment #14)
Hi, Jeremy. 
The smbd server was terminated by SIGTERM after receiving the crafted packets as follows:

```shell
$ ASAN_OPTIONS=detect_odr_violation=0 /usr/local/samba/sbin/smbd -i &> smbd.out
fish: 'ASAN_OPTIONS=detect_odr_violati…' terminated by signal SIGTERM (Polite quit request)
```
Comment 20 Jeremy Allison 2023-03-25 02:03:17 UTC
(In reply to fouzhe from comment #19)

I think that's the server quitting after the client disconnect. There is no server crash in the logs I can see.
Comment 21 Samba QA Contact 2023-03-29 18:59:04 UTC
This bug was referenced in samba master:

006fe806782c42e860ed2cf2bc9f6b1b82c3a307
76573d6d8f168d6e6107af26a434b8c71aaf93af
Comment 22 Jeremy Allison 2023-03-29 22:45:27 UTC
Created attachment 17850 [details]
git-am fix for 4.18.next

Cherry-picked from master.
Comment 23 Jeremy Allison 2023-03-29 22:56:09 UTC
Created attachment 17851 [details]
git-am fix for 4.17.next

Back-ported from master.
Comment 24 Ralph Böhme 2023-03-31 08:11:04 UTC
Reassigning to Jule for inclusion in 4.17 and 4.18.
Comment 25 fouzhe 2023-04-01 12:14:13 UTC
Dear Samba Team,

As a security researcher, CVEs are an important performance indicator for me. I would like to inquire if your team has any plans to allocate a CVE ID for this vulnerability.

Thank you for your attention to this matter.
Comment 26 Jeremy Allison 2023-04-01 19:37:15 UTC
(In reply to fouzhe from comment #25)

No, there is no intention to request a CVE for this. It is merely a client crash caused by incorrect validation of server responses. Such things are treated as a simple bug.
Comment 27 Jule Anger 2023-04-05 11:57:35 UTC
Pushed to autobuild-v4-{18,17}-test.
Comment 28 Samba QA Contact 2023-04-05 13:03:12 UTC
This bug was referenced in samba v4-18-test:

05fcd4f3035442df1e7fc2db0c4d4e74f6f36050
e59e9eadd0e86126c56107be08c2f5b25c6bc484
Comment 29 Samba QA Contact 2023-04-05 14:09:04 UTC
This bug was referenced in samba v4-17-test:

f7e888f78ec86f34ab72640fc805df5bb9e78cbc
7fe8a7d710d939c8db0f107e6fd3cf7f0da128c8
Comment 30 Jule Anger 2023-04-05 14:38:38 UTC
Closing out bug report.

Thanks!
Comment 31 Samba QA Contact 2023-04-19 10:22:59 UTC
This bug was referenced in samba v4-18-stable (Release samba-4.18.2):

05fcd4f3035442df1e7fc2db0c4d4e74f6f36050
e59e9eadd0e86126c56107be08c2f5b25c6bc484
Comment 32 Samba QA Contact 2023-05-11 07:09:59 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.8):

f7e888f78ec86f34ab72640fc805df5bb9e78cbc
7fe8a7d710d939c8db0f107e6fd3cf7f0da128c8