s3: smbd: Fix uninitialized memory read in process_symlink_open() when used with...

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 <jra@samba.org>
Reviewed-by: Ralph Böhme <slow@samba.org>
Autobuild-User(master): Jeremy Allison <jra@samba.org>
Autobuild-Date(master): Thu May 27 17:25:43 UTC 2021 on sn-devel-184

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 4a5d57cfb7b02dd6e5319959a749d8eee56348e1..919ccf4cab88ae1b6a591703fc9a546856607ac2 100644 (file)
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -532,7 +532,7 @@ static NTSTATUS process_symlink_open(const struct files_struct *dirfsp,
 {
        struct connection_struct *conn = dirfsp->conn;
        const char *conn_rootdir = NULL;
-       struct smb_filename conn_rootdir_fname;
+       struct smb_filename conn_rootdir_fname = { 0 };
        char *link_target = NULL;
        int link_len = -1;
        struct smb_filename *oldwd_fname = NULL;
@@ -547,9 +547,15 @@ static NTSTATUS process_symlink_open(const struct files_struct *dirfsp,
        if (conn_rootdir == NULL) {
                return NT_STATUS_NO_MEMORY;
        }
-       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.
@@ -669,6 +675,7 @@ static NTSTATUS process_symlink_open(const struct files_struct *dirfsp,
 
        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) {