From 2d1c262fa6297c3e7ea445b8f72030b934d5d65d Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 26 May 2021 22:41:53 -0700 Subject: [PATCH] s3: smbd: Fix uninitialized memory read in process_symlink_open() when used with vfs_shadow_copy2(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Valgrind trace follows. ==3627798== Invalid read of size 1 ==3627798== at 0x483FF46: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==3627798== by 0x55DE412: strdup (strdup.c:41) ==3627798== by 0x4F4657E: smb_xstrdup (util.c:660) ==3627798== by 0x4C62C2E: vfs_ChDir (vfs.c:988) ==3627798== by 0x4C4A51C: process_symlink_open (open.c:656) ==3627798== by 0x4C4ADE7: non_widelink_open (open.c:862) ==3627798== by 0x4C4AFB7: fd_openat (open.c:918) ==3627798== by 0x4BBE895: openat_pathref_fsp (files.c:506) ==3627798== by 0x4C48A00: filename_convert_internal (filename.c:2027) ==3627798== by 0x4C48B77: filename_convert (filename.c:2067) ==3627798== by 0x4C32408: call_trans2qfilepathinfo (trans2.c:6173) ==3627798== by 0x4C3C5DA: handle_trans2 (trans2.c:10143) ==3627798== Address 0xda8bc90 is 96 bytes inside a block of size 217 free'd ==3627798== at 0x483DA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==3627798== by 0x4FCA3C9: _tc_free_internal (talloc.c:1222) ==3627798== by 0x4FCA481: _talloc_free_internal (talloc.c:1248) ==3627798== by 0x4FCB825: _talloc_free (talloc.c:1792) ==3627798== by 0xDB248DD: store_cwd_data (vfs_shadow_copy2.c:1473) ==3627798== by 0xDB24BEF: shadow_copy2_chdir (vfs_shadow_copy2.c:1542) ==3627798== by 0x4C662A4: smb_vfs_call_chdir (vfs.c:2257) ==3627798== by 0x4C62B48: vfs_ChDir (vfs.c:940) ==3627798== by 0x4C4A51C: process_symlink_open (open.c:656) ==3627798== by 0x4C4ADE7: non_widelink_open (open.c:862) ==3627798== by 0x4C4AFB7: fd_openat (open.c:918) ==3627798== by 0x4BBE895: openat_pathref_fsp (files.c:506) ==3627798== Block was alloc'd at ==3627798== at 0x483C7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==3627798== by 0x4FC9365: __talloc_with_prefix (talloc.c:783) ==3627798== by 0x4FC94FF: __talloc (talloc.c:825) ==3627798== by 0x4FCCFDC: __talloc_strlendup (talloc.c:2454) ==3627798== by 0x4FCD096: talloc_strdup (talloc.c:2470) ==3627798== by 0xDB24977: store_cwd_data (vfs_shadow_copy2.c:1476) ==3627798== by 0xDB24BEF: shadow_copy2_chdir (vfs_shadow_copy2.c:1542) ==3627798== by 0x4C662A4: smb_vfs_call_chdir (vfs.c:2257) ==3627798== by 0x4C62B48: vfs_ChDir (vfs.c:940) ==3627798== by 0x4C4A92D: non_widelink_open (open.c:755) ==3627798== by 0x4C4AFB7: fd_openat (open.c:918) ==3627798== by 0x4BBE895: openat_pathref_fsp (files.c:506) ==3627798== Even though SMB_VFS_CONNECTPATH() returns a const char, vfs_shadow_copy2() can free and reallocate this whilst in use inside process_symlink_open(). Take a copy to make sure we don't reference free'd memory. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14721 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Böhme Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Thu May 27 17:25:43 UTC 2021 on sn-devel-184 (cherry picked from commit 2f0cfe82907516ecf23cc385d41b8d29ed6b8c96) --- source3/smbd/open.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 0c2c381bde5..efae16bbbb5 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -477,7 +477,7 @@ static int process_symlink_open(struct connection_struct *conn, unsigned int link_depth) { const char *conn_rootdir = NULL; - struct smb_filename conn_rootdir_fname; + struct smb_filename conn_rootdir_fname = { 0 }; int fd = -1; char *link_target = NULL; int link_len = -1; @@ -493,9 +493,15 @@ static int process_symlink_open(struct connection_struct *conn, errno = ENOMEM; return -1; } - conn_rootdir_fname = (struct smb_filename) { - .base_name = discard_const_p(char, conn_rootdir), - }; + /* + * With shadow_copy2 conn_rootdir can be talloc_freed + * whilst we use it in this function. We must take a copy. + */ + conn_rootdir_fname.base_name = talloc_strdup(talloc_tos(), + conn_rootdir); + if (conn_rootdir_fname.base_name == NULL) { + return NT_STATUS_NO_MEMORY; + } /* * Ensure we don't get stuck in a symlink loop. @@ -602,6 +608,7 @@ static int process_symlink_open(struct connection_struct *conn, TALLOC_FREE(resolved_fname); TALLOC_FREE(link_target); + TALLOC_FREE(conn_rootdir_fname.base_name); if (oldwd_fname != NULL) { int ret = vfs_ChDir(conn, oldwd_fname); if (ret == -1) { -- 2.27.0