| Message ID | Y76gvH9ADxSgAxSw@sernet.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix posix 311 symlink creation mode | expand |
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>
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
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
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.
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
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
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
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