Bug 14374 - Segfault when using SMBC_opendir_ctx() routine for share folder that contains incorrect symbols in any file name.
Summary: Segfault when using SMBC_opendir_ctx() routine for share folder that contains...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.10.14
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-09 17:34 UTC by Sergey Popov
Modified: 2020-06-04 10:34 UTC (History)
2 users (show)

See Also:


Attachments
git-am fix for master. (1020 bytes, patch)
2020-05-11 19:12 UTC, Jeremy Allison
no flags Details
git-am fix for master. (3.22 KB, patch)
2020-05-11 19:41 UTC, Jeremy Allison
no flags Details
git-am fix for master. (9.34 KB, patch)
2020-05-11 23:01 UTC, Jeremy Allison
no flags Details
Incorrect file name from data_dir (26 bytes, application/octet-stream)
2020-05-12 07:41 UTC, Sergey Popov
no flags Details
back-ported git-am fix for 4.12.next. (9.93 KB, patch)
2020-05-18 17:14 UTC, Jeremy Allison
asn: review+
Details
back-ported git-am fix for 4.11.next. (9.85 KB, patch)
2020-05-18 17:25 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Popov 2020-05-09 17:34:32 UTC
We are developing the utility on Linux Platform that receives information about specified share Windows-folder using SMBC_opendir_ctx() routine from libsmbclient.so. Unfortunately, we have detected a crash (segfault error) when share folder contains some incorrect symbols (0x00, for example) in any file name. This may happens if the share folder contains files after recovery (as in our case).

Internal routine parse_finfo_id_both_directory_info() cannot convert described file names to unicode charset, return NT_STATUS_OK but assign a nullptr to member file_info::name. Our instance of file_info struct in gdb:

(gdb) p *finfo
$23 = {size = 56591, mode = 32, uid = 0, gid = 0, btime_ts = {tv_sec = 1542900517, tv_nsec = 128496300}, mtime_ts = {tv_sec = 1480419618, tv_nsec = 0}, atime_ts = {tv_sec = 1542900517, tv_nsec = 128496300}, ctime_ts = {tv_sec = 1542900517, tv_nsec = 136496300}, name = 0x0, short_name = 0x14957b0 "BC3E~1"}

After that strdup() routine was called without any checks for nullptr in file_info::name member. Next is our backtrace on segfault:

#0  __strlen_sse2 () at ../sysdeps/x86_64/strlen.S:31
#1  0x00007f5bbaff4a9e in __GI___strdup (s=0x0) at strdup.c:41
#2  0x00007f5bbca86144 in add_dirplus (finfo=0x1c27a90, dir=0x150cd30) at ../source3/libsmb/libsmb_dir.c:191
#3  dir_list_fn (mnt=<optimized out>, finfo=finfo@entry=0x1c27a90,
    mask=mask@entry=0x15da990 "<some path>", state=state@entry=0x150cd30)
    at ../source3/libsmb/libsmb_dir.c:346
#4  0x00007f5bba1ea32e in cli_smb2_list (cli=cli@entry=0x1675700,
    pathname=pathname@entry=0x15da990 "<some path>", attribute=attribute@entry=22,
    fn=fn@entry=0x7f5bbca86050 <dir_list_fn>, state=state@entry=0x150cd30) at ../source3/libsmb/cli_smb2_fnum.c:1019
#5  0x00007f5bba1d527d in cli_list (cli=0x1675700, mask=0x15da990 "<some path>",
    attribute=attribute@entry=22, fn=fn@entry=0x7f5bbca86050 <dir_list_fn>, state=state@entry=0x150cd30) at ../source3/libsmb/clilist.c:974
#6  0x00007f5bbca86d43 in SMBC_opendir_ctx (context=0x14c8090,
    fname=fname@entry=0xcdc198 "smb://<host>/<some path>")
    at ../source3/libsmb/libsmb_dir.c:936

Internal routine add_dirplus() contains the following code:

191    info->name = SMB_STRDUP(finfo->name);
192    if (info->name == NULL) {
193        SAFE_FREE(info);
194        SAFE_FREE(new_entry);
195        dir->dir_error = ENOMEM;
196        return -1;
197    }
198
199    if (finfo->short_name) {
200        info->short_name = SMB_STRDUP(finfo->short_name);
201    } else {
202        info->short_name = SMB_STRDUP("");
203    }

As you can see, no any checks for nullptr in finfo->name, but for finfo->short_name member that check is exists. We have fixed libsmbclient.so code as next:

if (info->name) {
    info->name = SMB_STRDUP(finfo->name);
    if (info->name == NULL) {
        SAFE_FREE(info);
        SAFE_FREE(new_entry);
        dir->dir_error = ENOMEM;
        return -1;
    }
} else {
    SAFE_FREE(info);
    SAFE_FREE(new_entry);
    dir->dir_error = EBADF;
    return -1;
}

and segfault is out.
Comment 1 Volker Lendecke 2020-05-11 13:49:11 UTC
Would it also help to add the "name!=NULL" check to parse_finfo_id_both_directory_info() itself right after the pull_string_talloc routine? I don't think that we should further process a "struct file_info" without a file name.
Comment 2 Jeremy Allison 2020-05-11 16:06:40 UTC
Yes Volker is right, once the pull_string fails we shouldn't process that file further. Can you tell us the exact byte pattern of the name that causes the iconv problem ? I'd love to add in a regression test here. Thanks !
Comment 3 Jeremy Allison 2020-05-11 18:55:12 UTC
Oh, is namelen when read from inside parse_finfo_id_both_directory_info() read as zero ?

We already return NT_STATUS_INVALID_NETWORK_RESPONSE from parse_finfo_id_both_directory_info() if the pull_string_talloc() call fails with an invalid conversion.

If that's the case, then we should just add a check for namelen == 0 inside parse_finfo_id_both_directory_info() and return NT_STATUS_INVALID_NETWORK_RESPONSE.

slen, the short name len can be zero here as it's permissible for NTFS filesystems to not store a short name.
Comment 4 Jeremy Allison 2020-05-11 19:04:41 UTC
Ah, I can't see any over-the-wire test mechanism to add finfo->name == NULL over the wire (we'd need a malicious server to test that one).

We already have a "check name from server is good" function in the form of is_bad_finfo_name() - called from both SMB1 and SMB2.

So adding the NULL check there should do the trick.
Comment 5 Jeremy Allison 2020-05-11 19:12:12 UTC
Created attachment 15967 [details]
git-am fix for master.
Comment 6 Jeremy Allison 2020-05-11 19:22:00 UTC
Comment on attachment 15967 [details]
git-am fix for master.

No, that's not quite right. We want to terminate the directory listing, not the connection..
Comment 7 Jeremy Allison 2020-05-11 19:41:25 UTC
Created attachment 15968 [details]
git-am fix for master.
Comment 8 Jeremy Allison 2020-05-11 23:01:23 UTC
Created attachment 15970 [details]
git-am fix for master.

Complete fix including regression tests.
Comment 9 Sergey Popov 2020-05-12 07:24:47 UTC
I'm sorry for late response. Unfortunately I can't attach full dump of data_dir due to privacy policy. But I can attach dump of full file name from data_dir [dir_data + 104, dir_data + 104 + namelen] that we using for a bug reproduce.
Comment 10 Sergey Popov 2020-05-12 07:41:40 UTC
Created attachment 15972 [details]
Incorrect file name from data_dir
Comment 11 Jeremy Allison 2020-05-12 18:33:44 UTC
Thanks Sergey. Can you test the patch in:

https://bugzilla.samba.org/attachment.cgi?id=15970

please ? I think it will fix you problem but it would be nice to get confirmation.
Comment 12 Sergey Popov 2020-05-12 19:50:57 UTC
(In reply to Jeremy Allison from comment #11)
Jeremy, thank you!  I'll try to test your patch and give you feedback as soon as possible.
Comment 13 Sergey Popov 2020-05-18 14:15:19 UTC
Hello, Jeremy! I have a good news. Your fix works. After applying it, segfault is gone.
Comment 14 Jeremy Allison 2020-05-18 17:14:52 UTC
Created attachment 15989 [details]
back-ported git-am fix for 4.12.next.
Comment 15 Jeremy Allison 2020-05-18 17:25:47 UTC
Created attachment 15990 [details]
back-ported git-am fix for 4.11.next.
Comment 16 Andreas Schneider 2020-05-25 12:43:13 UTC
Karolin, please apply the patches to the relevant branches! Thanks.
Comment 17 Karolin Seeger 2020-06-04 10:31:47 UTC
(In reply to Andreas Schneider from comment #16)
Pushed to autobuild-v4-{12,11}-test.
Comment 18 Karolin Seeger 2020-06-04 10:34:33 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to both branches.
Closing out bug report.

Thanks!