[PATCH] Fix smbd panic if we chdir() to an unreadable directory.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix smbd panic if we chdir() to an unreadable directory.

Samba - samba-technical mailing list
Hi folks,

Currently smbd will (deliberately) panic in the
vfs_ChDir() wrapper function if we chdir() to a
directory, and then the following getwd() fails.
We do this as lots of smbd internals depend on keeping
the $cwd state correctly cached and also stored on the
connection struct.

This is always true on Linux - if we can chdir()
to a directory we can always read /proc/self/cwd.

However:

bug: https://bugzilla.samba.org/show_bug.cgi?id=13027

shows that under Solaris and other Solaris-OS-forks that
this is not always the case, so on those systems
smbd panics if the share has any search-only directories
with readable subdirectories.

The following patch changes the wrapper vfs_ChDir()
function to save the $cwd state (held on the connection
struct), and if the chdir() succeeds but the following
getwd() fails, does a chdir() to the saved $cwd state and
returns fail for the original chdir() request with -1
instead of panicing.

If there's no existing $cwd state (first use of share)
or if the return to stored $cwd chdir() fails we still
panic, so this fails safe.

Bug reporter has confirmed this fixes his issue.

This is a master and 4.7.x-only fix as it relies on the
struct smb_filename plumbing through the VFS.

Please review and push if happy.

Cheers,

        Jeremy.

0001-s3-smbd-Currently-if-getwd-fails-after-a-chdir-we-pa.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix smbd panic if we chdir() to an unreadable directory.

Samba - samba-technical mailing list
Howdy!

On Fri, Oct 06, 2017 at 09:56:50PM +0000, Jeremy Allison wrote:
> Please review and push if happy.

generally looks good, but, while at it, can we use an early return and reduce the
indentation level please?

-slow
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix smbd panic if we chdir() to an unreadable directory.

Samba - samba-technical mailing list
On Sun, Oct 08, 2017 at 06:51:25PM +0200, Ralph Böhme wrote:
> Howdy!
>
> On Fri, Oct 06, 2017 at 09:56:50PM +0000, Jeremy Allison wrote:
> > Please review and push if happy.
>
> generally looks good, but, while at it, can we use an early return and reduce the
> indentation level please?

Yes of course. You know I was looking at ways to do that, but didn't
spot the obvious early return from the CHDIR(). Doh!

Thanks :-).

Jeremy.

> From cae435c8a948c1a39ab750aba24fe749e79601a2 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Sun, 8 Oct 2017 18:48:23 +0200
> Subject: [PATCH] FIXUP: early return from failing SMB_VFS_CHDIR, reducing
>  indentation level
>
> ---
>  source3/smbd/vfs.c | 92 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
> index 817e69b484b..d4fccb7eda9 100644
> --- a/source3/smbd/vfs.c
> +++ b/source3/smbd/vfs.c
> @@ -888,57 +888,63 @@ int vfs_ChDir(connection_struct *conn, const struct smb_filename *smb_fname)
>   DEBUG(4,("vfs_ChDir to %s\n", smb_fname->base_name));
>  
>   ret = SMB_VFS_CHDIR(conn, smb_fname);
> - if (ret == 0) {
> + if (ret != 0) {
> + saved_errno = errno;
> + TALLOC_FREE(saved_cwd);
> + errno = saved_errno;
> + return -1;
> + }
> +
> + /*
> + * Always replace conn->cwd_fname. We
> + * don't know if it's been modified by
> + * VFS modules in the stack.
> + */
> +
> + /* conn cache. */
> + TALLOC_FREE(conn->cwd_fname);
> + conn->cwd_fname = vfs_GetWd(conn, conn);
> + if (conn->cwd_fname == NULL) {
>   /*
> - * Always replace conn->cwd_fname. We
> - * don't know if it's been modified by
> - * VFS modules in the stack.
> + * vfs_GetWd() failed.
> + * We must be able to read cwd.
> + * Return to original directory
> + * and return -1.
>   */
> - /* conn cache. */
> - TALLOC_FREE(conn->cwd_fname);
> - conn->cwd_fname = vfs_GetWd(conn, conn);
> - if (conn->cwd_fname == NULL) {
> + saved_errno = errno;
> +
> + if (saved_cwd == NULL) {
>   /*
> - * vfs_GetWd() failed.
> - * We must be able to read cwd.
> - * Return to original directory
> - * and return -1.
> + * Failed on the very first chdir()+getwd()
> + * for this connection. We can't
> + * continue.
>   */
> - saved_errno = errno;
> -
> - if (saved_cwd == NULL) {
> - /*
> - * Failed on the very first chdir()+getwd()
> - * for this connection. We can't
> - * continue.
> - */
> - smb_panic("conn->cwd getwd failed\n");
> - /* NOTREACHED */
> - return -1;
> - }
> -
> - /* Return to the previous $cwd. */
> - ret = SMB_VFS_CHDIR(conn, saved_cwd);
> - if (ret != 0) {
> - smb_panic("conn->cwd getwd failed\n");
> - /* NOTREACHED */
> - return -1;
> - }
> - /* Restore original conn->cwd_fname. */
> - conn->cwd_fname = saved_cwd;
> - errno = saved_errno;
> - /* And fail the chdir(). */
> + smb_panic("conn->cwd getwd failed\n");
> + /* NOTREACHED */
>   return -1;
>   }
> - /* vfs_GetWd() succeeded. */
> - /* Replace global cache. */
> - SAFE_FREE(LastDir);
> - LastDir = SMB_STRDUP(smb_fname->base_name);
>  
> - DEBUG(4,("vfs_ChDir got %s\n", conn->cwd_fname->base_name));
> - } else {
> - saved_errno = errno;
> + /* Return to the previous $cwd. */
> + ret = SMB_VFS_CHDIR(conn, saved_cwd);
> + if (ret != 0) {
> + smb_panic("conn->cwd getwd failed\n");
> + /* NOTREACHED */
> + return -1;
> + }
> + /* Restore original conn->cwd_fname. */
> + conn->cwd_fname = saved_cwd;
> + errno = saved_errno;
> + /* And fail the chdir(). */
> + return -1;
>   }
> +
> + /* vfs_GetWd() succeeded. */
> + /* Replace global cache. */
> + SAFE_FREE(LastDir);
> + LastDir = SMB_STRDUP(smb_fname->base_name);
> +
> + DEBUG(4,("vfs_ChDir got %s\n", conn->cwd_fname->base_name));
> +
>   TALLOC_FREE(saved_cwd);
>   if (saved_errno != 0) {
>   errno = saved_errno;
> --
> 2.13.5
>