Fix posix 311 symlink creation mode

Fix posix 311 symlink creation mode

[Volker Lendecke]

[-- Attachment #1: Type: text/plain, Size: 133 bytes --]

Hi!

Attached find a patch that fixes an uninitialized memory read when
creating symlinks using the smb311 posix extensions.

Volker

[-- Attachment #2: posix-symlink.diff --]
[-- Type: text/x-diff, Size: 971 bytes --]

From 482fa85ef97505626b6b146155834e6bc36012fa Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Wed, 11 Jan 2023 12:37:58 +0100
Subject: [PATCH] cifs: Fix uninitialized memory read for smb311 posix symlink
 create

If smb311 posix is enabled, we send the intended mode for file
creation in the posix create context. Instead of using what's there on
the stack, create the mfsymlink file with 0644.

Signed-off-by: Volker Lendecke <vl@samba.org>
---
 fs/cifs/link.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/cifs/link.c b/fs/cifs/link.c
index bd374feeccaa..a5a097a69983 100644
--- a/fs/cifs/link.c
+++ b/fs/cifs/link.c
@@ -428,6 +428,7 @@ smb3_create_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
 	oparms.disposition = FILE_CREATE;
 	oparms.fid = &fid;
 	oparms.reconnect = false;
+	oparms.mode = 0644;
 
 	rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL,
 		       NULL, NULL);
-- 
2.30.2

Re: Fix posix 311 symlink creation mode

[Paulo Alcantara]

Volker Lendecke <Volker.Lendecke@sernet.de> writes:

> Attached find a patch that fixes an uninitialized memory read when
> creating symlinks using the smb311 posix extensions.
>
> Volker
> From 482fa85ef97505626b6b146155834e6bc36012fa Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl@samba.org>
> Date: Wed, 11 Jan 2023 12:37:58 +0100
> Subject: [PATCH] cifs: Fix uninitialized memory read for smb311 posix symlink
>  create
>
> If smb311 posix is enabled, we send the intended mode for file
> creation in the posix create context. Instead of using what's there on
> the stack, create the mfsymlink file with 0644.
>
> Signed-off-by: Volker Lendecke <vl@samba.org>
> ---
>  fs/cifs/link.c | 1 +
>  1 file changed, 1 insertion(+)

Looks good to me.  Thanks!

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

Re: Fix posix 311 symlink creation mode

[Steve French]

Should this be 0777 instead of 0644?

I noticed the man page for symlink says:

     "On Linux, the permissions of an ordinary symbolic link are not
       used in any operations; the permissions are always 0777 (read,
       write, and execute for all user categories), and can't be
       changed."

On Wed, Jan 11, 2023 at 6:14 AM Volker Lendecke
<Volker.Lendecke@sernet.de> wrote:
>
> Hi!
>
> Attached find a patch that fixes an uninitialized memory read when
> creating symlinks using the smb311 posix extensions.
>
> Volker



-- 
Thanks,

Steve

Re: Fix posix 311 symlink creation mode

[Volker Lendecke]

Am Wed, Jan 11, 2023 at 10:21:07AM -0600 schrieb Steve French:
> Should this be 0777 instead of 0644?
> 
> I noticed the man page for symlink says:
> 
>      "On Linux, the permissions of an ordinary symbolic link are not
>        used in any operations; the permissions are always 0777 (read,
>        write, and execute for all user categories), and can't be
>        changed."

I thought about that as well. If you "ls -l" such a symlink once
created, it will show as 0777, probably the client does it
somehow. The problem however is that if you create it with 0777
server-side, everybody can mess with that file that the client view as
a symlink. That's why I thought that 0644 is the best approximation.

Regards, Volker

Re: Fix posix 311 symlink creation mode

[Tom Talpey]

On 1/11/2023 11:51 AM, Volker Lendecke wrote:
> Am Wed, Jan 11, 2023 at 10:21:07AM -0600 schrieb Steve French:
>> Should this be 0777 instead of 0644?
>>
>> I noticed the man page for symlink says:
>>
>>       "On Linux, the permissions of an ordinary symbolic link are not
>>         used in any operations; the permissions are always 0777 (read,
>>         write, and execute for all user categories), and can't be
>>         changed."
> 
> I thought about that as well. If you "ls -l" such a symlink once
> created, it will show as 0777, probably the client does it
> somehow. The problem however is that if you create it with 0777
> server-side, everybody can mess with that file that the client view as
> a symlink. That's why I thought that 0644 is the best approximation.

Yeah, because this is an mfsymlink, which is really just a file with
specific contents, you might even argue 0444 is safest.

Definitely not 0777.

Tom.

Re: Fix posix 311 symlink creation mode

[Steve French]

tentatively merged into cifs-2.6.git for-next pending testing

On Wed, Jan 11, 2023 at 10:51 AM Volker Lendecke
<Volker.Lendecke@sernet.de> wrote:
>
> Am Wed, Jan 11, 2023 at 10:21:07AM -0600 schrieb Steve French:
> > Should this be 0777 instead of 0644?
> >
> > I noticed the man page for symlink says:
> >
> >      "On Linux, the permissions of an ordinary symbolic link are not
> >        used in any operations; the permissions are always 0777 (read,
> >        write, and execute for all user categories), and can't be
> >        changed."
>
> I thought about that as well. If you "ls -l" such a symlink once
> created, it will show as 0777, probably the client does it
> somehow. The problem however is that if you create it with 0777
> server-side, everybody can mess with that file that the client view as
> a symlink. That's why I thought that 0644 is the best approximation.
>
> Regards, Volker



-- 
Thanks,

Steve

Re: Fix posix 311 symlink creation mode

[David Disseldorp]

Hi Volker,

On Wed, 11 Jan 2023 12:42:52 +0100, Volker Lendecke wrote:

> If smb311 posix is enabled, we send the intended mode for file
> creation in the posix create context. Instead of using what's there on
> the stack, create the mfsymlink file with 0644.
> 
> Signed-off-by: Volker Lendecke <vl@samba.org>

Good catch. I think this deserves a Fixes tag as per
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
E.g.
Fixes: ce558b0e17f8a ("smb3: Add posix create context for smb3.11 posix mounts")
Preferably also with "Cc: stable@vger.kernel.org" for applicable
backports.

oparms.mode appears to be uninitialized in cifs_create_mf_symlink()
(#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY) and various other codepaths
but isn't currently used further down the call chain... This looks like
an accident waiting to happen.

Cheers, David

Re: Fix posix 311 symlink creation mode

[Steve French]

updated

On Thu, Jan 12, 2023 at 7:24 AM David Disseldorp <ddiss@suse.de> wrote:
>
> Hi Volker,
>
> On Wed, 11 Jan 2023 12:42:52 +0100, Volker Lendecke wrote:
>
> > If smb311 posix is enabled, we send the intended mode for file
> > creation in the posix create context. Instead of using what's there on
> > the stack, create the mfsymlink file with 0644.
> >
> > Signed-off-by: Volker Lendecke <vl@samba.org>
>
> Good catch. I think this deserves a Fixes tag as per
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> E.g.
> Fixes: ce558b0e17f8a ("smb3: Add posix create context for smb3.11 posix mounts")
> Preferably also with "Cc: stable@vger.kernel.org" for applicable
> backports.
>
> oparms.mode appears to be uninitialized in cifs_create_mf_symlink()
> (#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY) and various other codepaths
> but isn't currently used further down the call chain... This looks like
> an accident waiting to happen.
>
> Cheers, David



-- 
Thanks,

Steve