diff mbox series

[SMB3] allow files to be created with backslash in file name

Message ID CAH2r5msH3LZuF69UFcfgtG7XXurMDc=-Ab7Ct4XwfARR8d+wRA@mail.gmail.com
State New
Headers show
Series [SMB3] allow files to be created with backslash in file name | expand

Commit Message

Steve French Jan. 1, 2021, 3:35 a.m. UTC
Backslash is reserved in Windows (and SMB2/SMB3 by default) but
allowed in POSIX so must be remapped when POSIX extensions are
not enabled.

The default mapping for SMB3 mounts ("SFM") allows mapping backslash
(ie 0x5C in UTF8) to 0xF026 in UCS-2 (using the Unicode remapping
range reserved for these characters), but this was not mapped by
cifs.ko (unlike asterisk, greater than, question mark etc).  This patch
fixes that to allow creating files and directories with backslash
in the file or directory name.

Before this patch:
   touch "/mnt2/filewith\slash"
would return
   touch: setting times of '/mnt2/filewith\slash': Invalid argument

With the patch touch and mkdir with the backslash in the name works.

This problem was found while debugging xfstest 453 - see
    https://bugzilla.kernel.org/show_bug.cgi?id=210961

This patch may be even more important to Samba, as alternative ways of
storing these files can create more problems. Interestingly Samba
server reports local files with backslashes in them over the wire
without remapping, even though these are illegal in SMB3 which would
cause confusion on the client(s).  Has anyone tried Windows mounting
to Samba and viewing files with locally created Linux files that
include these reserved characters (presumably Windows clients won't
like it either)?

This patch does allow creating/viewing files with remotely with '\' in
the file name from Linux (cifs.ko) but does not completely fix xfstest
453 kernel (more investigation of this test is needed).  Test 453
creates filenames with 'horrifying'
(using their term) sequences of arbitrary bytes in file names.

Reported-by: Xiaoli Feng <xifeng@redhat.com>

Comments

Jeremy Allison Jan. 1, 2021, 6 a.m. UTC | #1
On Thu, Dec 31, 2020 at 09:35:23PM -0600, Steve French via samba-technical wrote:
>
>This patch may be even more important to Samba, as alternative ways of
>storing these files can create more problems. Interestingly Samba
>server reports local files with backslashes in them over the wire
>without remapping, even though these are illegal in SMB3 which would
>cause confusion on the client(s).  Has anyone tried Windows mounting

Samba should mangle names containing '\' to 8.3 names.

The code in is_legal_name() should catch names containing
'\' and report them as needing mapping to 8.3.

Indeed if I check this locally with smbclient I get
(for a share containing a file created with:

$ touch 'file with \ in it'

I see the file:

FI32YH~P

listed from smbclient -mSMB3

Check how you have Samba configured Steve.
Jeremy Allison Jan. 1, 2021, 7:58 p.m. UTC | #2
On Fri, Jan 01, 2021 at 09:12:14AM -0600, Steve French wrote:
>On Fri, Jan 1, 2021 at 12:00 AM Jeremy Allison <jra@samba.org> wrote:
>>
>> On Thu, Dec 31, 2020 at 09:35:23PM -0600, Steve French via samba-technical wrote:
>> >
>> >This patch may be even more important to Samba, as alternative ways of
>> >storing these files can create more problems. Interestingly Samba
>> >server reports local files with backslashes in them over the wire
>> >without remapping, even though these are illegal in SMB3 which would
>> >cause confusion on the client(s).  Has anyone tried Windows mounting
>>
>> Samba should mangle names containing '\' to 8.3 names.
>
>You were right mangled names was enabled.  But that is also
>interesting - it does expose a bug in smbclient.
>
>When you connect smbclient - doing a ls of a subdirectory with
>reserved characters worked, but doing an ls of the parent (root
>directory of share) caused smbclient to disconnect.  See below
>
>smb: \> ls rsvd-chars
>  rsvd-chars                          D        0  Fri Jan  1 08:55:49 2021
>
>556368460 blocks of size 1024. 296010296 blocks available
>smb: \> ls
>  .                                   D        0  Fri Jan  1 08:54:28 2021
>  ..                                  D        0  Thu Dec 31 21:42:28 2020
>  topdir                              D        0  Mon Dec 14 16:01:25 2020
>  lock1.txt                           A      200  Fri Dec 18 12:28:18 2020
>  lock_rw_shared.dat                  A      200  Fri Dec 18 12:28:18 2020
>  lock_rw_exclusive.dat               A      200  Fri Dec 18 12:28:18 2020
>  autounlock.txt                      A      200  Fri Dec 18 12:28:18 2020
>is_bad_finfo_name: bad finfo->name
>NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
>smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
>connection is disconnected now

Can you log a bug please and give full setup instructions
to reproduce. This isn't enough to show me what the bug is.
I need a directory listing from the Server side to show
me what files are in the root of the share.

Also, you neglect to tell me what Samba version you are
using (which is a pre-requisite for a bug report Steve,
you know this :-).
Jeremy Allison Jan. 1, 2021, 8:06 p.m. UTC | #3
On Fri, Jan 01, 2021 at 09:12:14AM -0600, Steve French wrote:
>On Fri, Jan 1, 2021 at 12:00 AM Jeremy Allison <jra@samba.org> wrote:
>>
>> On Thu, Dec 31, 2020 at 09:35:23PM -0600, Steve French via samba-technical wrote:
>> >
>> >This patch may be even more important to Samba, as alternative ways of
>> >storing these files can create more problems. Interestingly Samba
>> >server reports local files with backslashes in them over the wire
>> >without remapping, even though these are illegal in SMB3 which would
>> >cause confusion on the client(s).  Has anyone tried Windows mounting
>>
>> Samba should mangle names containing '\' to 8.3 names.
>
>You were right mangled names was enabled.  But that is also
>interesting - it does expose a bug in smbclient.
>
>When you connect smbclient - doing a ls of a subdirectory with
>reserved characters worked, but doing an ls of the parent (root
>directory of share) caused smbclient to disconnect.  See below
>
>smb: \> ls rsvd-chars
>  rsvd-chars                          D        0  Fri Jan  1 08:55:49 2021
>
>556368460 blocks of size 1024. 296010296 blocks available
>smb: \> ls
>  .                                   D        0  Fri Jan  1 08:54:28 2021
>  ..                                  D        0  Thu Dec 31 21:42:28 2020
>  topdir                              D        0  Mon Dec 14 16:01:25 2020
>  lock1.txt                           A      200  Fri Dec 18 12:28:18 2020
>  lock_rw_shared.dat                  A      200  Fri Dec 18 12:28:18 2020
>  lock_rw_exclusive.dat               A      200  Fri Dec 18 12:28:18 2020
>  autounlock.txt                      A      200  Fri Dec 18 12:28:18 2020
>is_bad_finfo_name: bad finfo->name
>NT_STATUS_INVALID_NETWORK_RESPONSE listing \*

This is coming from the following code which is designed to
protect the client from a malicious server returning a '\' or '/'
character in an filename component.

/****************************************************************************
  Check if a returned directory name is safe.
****************************************************************************/

static NTSTATUS is_bad_name(bool windows_names, const char *name)
{
         const char *bad_name_p = NULL;

         bad_name_p = strchr(name, '/');
         if (bad_name_p != NULL) {
                 /*
                  * Windows and POSIX names can't have '/'.
                  * Server is attacking us.
                  */
                 return NT_STATUS_INVALID_NETWORK_RESPONSE;
         }
         if (windows_names) {
                 bad_name_p = strchr(name, '\\');
                 if (bad_name_p != NULL) {
                         /*
                          * Windows names can't have '\\'.
                          * Server is attacking us.
                          */
                         return NT_STATUS_INVALID_NETWORK_RESPONSE;
                 }
         }
         return NT_STATUS_OK;
}
Jeremy Allison Jan. 2, 2021, 2:58 a.m. UTC | #4
On Fri, Jan 01, 2021 at 05:57:08PM -0500, Steve French wrote:
>4.12.4 Ubuntu
>
>On Fri, Jan 1, 2021, 14:58 Jeremy Allison <jra@samba.org> wrote:
>> >is_bad_finfo_name: bad finfo->name
>> >NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
>> >smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
>> >connection is disconnected now
>>
>> Can you log a bug please and give full setup instructions
>> to reproduce. This isn't enough to show me what the bug is.
>> I need a directory listing from the Server side to show
>> me what files are in the root of the share.
>>
>> Also, you neglect to tell me what Samba version you are
>> using (which is a pre-requisite for a bug report Steve,
>> you know this :-).

To channel Rowland, this isn't a bug report, this is an
anecdote :-).

Here's what I did to show this doesn't happen.

$ mkdir /tmp/chartest
$ touch /tmp/chartest/fil\\e.dat
$ ls -l /tmp/chartest/
total 0
-rw-rw-r-- 1 user user 0 Jan  1 18:52 'fil\e.dat'

Edit /usr/local/samba/etc/smb.conf, add:

[chartest]
	path = /tmp/chartest
	guest ok = yes
	read only = no

Restart smbd. Run:

$ /usr/local/samba/bin/smbclient //127.0.0.1/chartest -Uuser%password -mSMB3
smb: \> ls
   .                                   D        0  Fri Jan  1 18:52:10 2021
   ..                                  D        0  Fri Jan  1 18:51:38 2021
   FI7KDO~J.DAT                        N        0  Fri Jan  1 18:52:10 2021

IF YOU WANT ME TO INVESTIGATE THIS IS NEED A SIMILAR
LEVEL OF DETAIL (sigh). But I *know* you *know*
this...
Steve French Jan. 2, 2021, 3:49 a.m. UTC | #5
I wasn't able to setup a second independent repro of this until
tonight, but the smbclient behavior (bug) does appear to be dependent
on the version of smbclient (see below).  Newer versions failed
(including current Samba, 4.14-pre and 4.12.5 Ubuntu).  These were all
attempted on the same machine, and the repro was simple. See below.

Recreation:
create two files with reserved characters (locally on the server) in
the root of the share (in this case xfs, running on the current stable
kernel,
5.10.4 although that is unlikely to matter)
$ ls /scratch
'file-\-backslash'  'file-?-question'

I also created a subdirectory with additional files with reserved
characters, although that may not matter.

$ ls /scratch/rsvd-chars
'dir-with-trailing-space '  'file-\backslash'    'file space'
 file                       'file->greaterthan'
'file-*asterisk'            'file-?question'

I exported the /scratch directory with smb.conf configured as

[scratch]
   comment = scratch share for testing
   browseable = yes
   path = /scratch
   guest ok = yes
   read only = no
   ea support = yes
   create mask = 0777
   directory mask = 0777
   vfs objects = acl_xattr
   map acl inherit = yes
   strict allocate = yes
   map acl inherit = yes
   mangled names = no

Connecting with smbclient and doing a simple ls causes the disconnect:
$ smbclient --version
Version 4.12.5-Ubuntu
$ smbclient //localhost/scratch -U testuser
Enter SAMBA\testuser's password:
Try "help" to get a list of possible commands.
smb: \> ls
  .                                   D        0  Fri Jan  1 21:19:52 2021
  ..                                  D        0  Thu Dec 31 21:42:28 2020
  rsvd-chars                          D        0  Fri Jan  1 09:14:04 2021
  file-?-question                     N        0  Fri Jan  1 21:19:42 2021
is_bad_finfo_name: bad finfo->name
NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
connection is disconnected now

wireshark trace shows a simple open (SMB3.1.1 create) followed by
level 37 SMB3.1.1 find request, then smbclient appears to drop the tcp
session.

Running smbclient with -d 10 (debug level) shows these three messages

dos_clean_name [\*]
unix_clean_name [\*]
is_bad_finfo_name: bad finfo->name

Trying the same thing with *older* smbclient worked, although there is
a possibility that other differences in build with the two smbclients
could be the reason that the older one worked and the newer one
failed:

$ /usr/local/samba/bin/smbclient //localhost/share --version
Version 4.11.0pre1-GIT-e63e391a1ad
$ /usr/local/samba/bin/smbclient //localhost/scratch -U testuser
smb: \> ls
  .                                   D        0  Fri Jan  1 21:19:52 2021
  ..                                  D        0  Thu Dec 31 21:42:28 2020
  rsvd-chars                          D        0  Fri Jan  1 09:14:04 2021
  file-?-question                     N        0  Fri Jan  1 21:19:42 2021
  file-\-backslash                    N        0  Fri Jan  1 21:19:52 2021

556368460 blocks of size 1024. 295948832 blocks available

I also tried it with current smbclient and it also failed:

$ ./bin/smbclient --version
Version 4.14.0pre1-DEVELOPERBUILD
$ ./bin/smbclient //localhost/scratch -U testuser
Enter SAMBA\testuser's password:
Try "help" to get a list of possible commands.
smb: \> ls
  .                                   D        0  Fri Jan  1 21:19:52 2021
  ..                                  D        0  Thu Dec 31 21:42:28 2020
  rsvd-chars                          D        0  Fri Jan  1 09:14:04 2021
  file-?-question                     N        0  Fri Jan  1 21:19:42 2021
is_bad_finfo_name: bad finfo->name
NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
connection is disconnected now

On Fri, Jan 1, 2021 at 8:58 PM Jeremy Allison <jra@samba.org> wrote:
>
> On Fri, Jan 01, 2021 at 05:57:08PM -0500, Steve French wrote:
> >4.12.4 Ubuntu
> >
> >On Fri, Jan 1, 2021, 14:58 Jeremy Allison <jra@samba.org> wrote:
> >> >is_bad_finfo_name: bad finfo->name
> >> >NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
> >> >smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
> >> >connection is disconnected now
> >>
> >> Can you log a bug please and give full setup instructions
> >> to reproduce. This isn't enough to show me what the bug is.
> >> I need a directory listing from the Server side to show
> >> me what files are in the root of the share.
> >>
> >> Also, you neglect to tell me what Samba version you are
> >> using (which is a pre-requisite for a bug report Steve,
> >> you know this :-).
>
> To channel Rowland, this isn't a bug report, this is an
> anecdote :-).
>
> Here's what I did to show this doesn't happen.
>
> $ mkdir /tmp/chartest
> $ touch /tmp/chartest/fil\\e.dat
> $ ls -l /tmp/chartest/
> total 0
> -rw-rw-r-- 1 user user 0 Jan  1 18:52 'fil\e.dat'
>
> Edit /usr/local/samba/etc/smb.conf, add:
>
> [chartest]
>         path = /tmp/chartest
>         guest ok = yes
>         read only = no
>
> Restart smbd. Run:
>
> $ /usr/local/samba/bin/smbclient //127.0.0.1/chartest -Uuser%password -mSMB3
> smb: \> ls
>    .                                   D        0  Fri Jan  1 18:52:10 2021
>    ..                                  D        0  Fri Jan  1 18:51:38 2021
>    FI7KDO~J.DAT                        N        0  Fri Jan  1 18:52:10 2021
>
> IF YOU WANT ME TO INVESTIGATE THIS IS NEED A SIMILAR
> LEVEL OF DETAIL (sigh). But I *know* you *know*
> this...
Jeremy Allison Jan. 2, 2021, 5:25 a.m. UTC | #6
On Fri, Jan 01, 2021 at 09:49:06PM -0600, Steve French wrote:
>I exported the /scratch directory with smb.conf configured as
>
>[scratch]
>   comment = scratch share for testing
>   browseable = yes
>   path = /scratch
>   guest ok = yes
>   read only = no
>   ea support = yes
>   create mask = 0777
>   directory mask = 0777
>   vfs objects = acl_xattr
>   map acl inherit = yes
>   strict allocate = yes
>   map acl inherit = yes
>   mangled names = no
>
>Connecting with smbclient and doing a simple ls causes the disconnect:
>$ smbclient --version
>Version 4.12.5-Ubuntu
>$ smbclient //localhost/scratch -U testuser
>Enter SAMBA\testuser's password:
>Try "help" to get a list of possible commands.
>smb: \> ls
>  .                                   D        0  Fri Jan  1 21:19:52 2021
>  ..                                  D        0  Thu Dec 31 21:42:28 2020
>  rsvd-chars                          D        0  Fri Jan  1 09:14:04 2021
>  file-?-question                     N        0  Fri Jan  1 21:19:42 2021
>is_bad_finfo_name: bad finfo->name
>NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
>smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
>connection is disconnected now

Well of course it disconnects. You set

"mangled names = no"

So the server returns the bad name. The smbclient
library notices the server is trying to screw with
it by sending invalid Windows names and disconnects
to protect itself.

This is by design. There is a *REASON* mangled names = yes
is the default. Otherwise you can't see valid server
filenames that contain : \ etc. etc. from a Windows client.

Even a file names AUX: has to be mangled. "mangled names = no"
is only useful for a pre-cleaned exported file system which
you can guarantee contains only Windows-compatible names.

This is not a bug, it's working as designed to protect
the client code.

There was a CVE where libsmbclient might pass up
names containing a '/' to the calling code (not
that they can exist on disk, but a malicious server
could send them) which might then treat it as a
path separator.
Steve French Jan. 3, 2021, 12:19 a.m. UTC | #7
I agree with the idea of being safe (in the smbclient in this case),
and not returning potentially dangerous file names (even if a very
remote danger to the tool, smbclient in this case), but I am not
convinced that the "user friendly" behavior is to reject the names
with the rather confusing message - especially as it would mean that
inserting a single file with an odd name into a server could make the
whole share unusable for smbclient (e.g. in a backup scenario).  I
agree with rejecting (or perhaps better skipping) it, but ... not sure
any user would understand what SMBecho has to do with a server file
name.

"NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
connection is disconnected now"

On Fri, Jan 1, 2021 at 11:25 PM Jeremy Allison <jra@samba.org> wrote:
>
> On Fri, Jan 01, 2021 at 09:49:06PM -0600, Steve French wrote:
> >I exported the /scratch directory with smb.conf configured as
> >
> >[scratch]
> >   comment = scratch share for testing
> >   browseable = yes
> >   path = /scratch
> >   guest ok = yes
> >   read only = no
> >   ea support = yes
> >   create mask = 0777
> >   directory mask = 0777
> >   vfs objects = acl_xattr
> >   map acl inherit = yes
> >   strict allocate = yes
> >   map acl inherit = yes
> >   mangled names = no
> >
> >Connecting with smbclient and doing a simple ls causes the disconnect:
> >$ smbclient --version
> >Version 4.12.5-Ubuntu
> >$ smbclient //localhost/scratch -U testuser
> >Enter SAMBA\testuser's password:
> >Try "help" to get a list of possible commands.
> >smb: \> ls
> >  .                                   D        0  Fri Jan  1 21:19:52 2021
> >  ..                                  D        0  Thu Dec 31 21:42:28 2020
> >  rsvd-chars                          D        0  Fri Jan  1 09:14:04 2021
> >  file-?-question                     N        0  Fri Jan  1 21:19:42 2021
> >is_bad_finfo_name: bad finfo->name
> >NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
> >smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
> >connection is disconnected now
>
> Well of course it disconnects. You set
>
> "mangled names = no"
>
> So the server returns the bad name. The smbclient
> library notices the server is trying to screw with
> it by sending invalid Windows names and disconnects
> to protect itself.
>
> This is by design. There is a *REASON* mangled names = yes
> is the default. Otherwise you can't see valid server
> filenames that contain : \ etc. etc. from a Windows client.
>
> Even a file names AUX: has to be mangled. "mangled names = no"
> is only useful for a pre-cleaned exported file system which
> you can guarantee contains only Windows-compatible names.
>
> This is not a bug, it's working as designed to protect
> the client code.
>
> There was a CVE where libsmbclient might pass up
> names containing a '/' to the calling code (not
> that they can exist on disk, but a malicious server
> could send them) which might then treat it as a
> path separator.
Jeremy Allison Jan. 3, 2021, 1:21 a.m. UTC | #8
On Sat, Jan 02, 2021 at 06:19:39PM -0600, Steve French wrote:
>I agree with the idea of being safe (in the smbclient in this case),
>and not returning potentially dangerous file names (even if a very
>remote danger to the tool, smbclient in this case), but I am not
>convinced that the "user friendly" behavior is to reject the names
>with the rather confusing message - especially as it would mean that
>inserting a single file with an odd name into a server could make the
>whole share unusable for smbclient (e.g. in a backup scenario).  I
>agree with rejecting (or perhaps better skipping) it, but ... not sure
>any user would understand what SMBecho has to do with a server file
>name.

Dropping the connection on receipt of an invalid
name is the only safe response. We know the server
is insane and dangerous at that point and sending
invalid protocol responses.

Safer not to continue.

>"NT_STATUS_INVALID_NETWORK_RESPONSE listing \*
>smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The
>connection is disconnected now"

The SMBecho is due to the keepalive failing once
the connection was dropped.
Jeremy Allison Jan. 3, 2021, 1:25 a.m. UTC | #9
On Sat, Jan 02, 2021 at 05:21:16PM -0800, Jeremy Allison via samba-technical wrote:
>On Sat, Jan 02, 2021 at 06:19:39PM -0600, Steve French wrote:
>>I agree with the idea of being safe (in the smbclient in this case),
>>and not returning potentially dangerous file names (even if a very
>>remote danger to the tool, smbclient in this case), but I am not
>>convinced that the "user friendly" behavior is to reject the names
>>with the rather confusing message - especially as it would mean that
>>inserting a single file with an odd name into a server could make the
>>whole share unusable for smbclient (e.g. in a backup scenario).  I

FYI, as I pointed out this only happens if you *explicitly*
set a server parameter that is only expected to be set on
a share with "clean" (no non-Windows) names.

So just creating a file containing : \ etc. doesn't do
this - you have to misconfigure the server FIRST.
Steve French Jan. 3, 2021, 3:45 a.m. UTC | #10
On Sat, Jan 2, 2021 at 7:25 PM Jeremy Allison <jra@samba.org> wrote:
>
> On Sat, Jan 02, 2021 at 05:21:16PM -0800, Jeremy Allison via samba-technical wrote:
> >On Sat, Jan 02, 2021 at 06:19:39PM -0600, Steve French wrote:
> >>I agree with the idea of being safe (in the smbclient in this case),
> >>and not returning potentially dangerous file names (even if a very
> >>remote danger to the tool, smbclient in this case), but I am not
> >>convinced that the "user friendly" behavior is to reject the names
> >>with the rather confusing message - especially as it would mean that
> >>inserting a single file with an odd name into a server could make the
> >>whole share unusable for smbclient (e.g. in a backup scenario).  I
>
> FYI, as I pointed out this only happens if you *explicitly*
> set a server parameter that is only expected to be set on
> a share with "clean" (no non-Windows) names.
>
> So just creating a file containing : \ etc. doesn't do
> this - you have to misconfigure the server FIRST.

I agree that with Samba server this is less common (not sure how many
vendors set that smb.conf
parm) but note that "man smb.conf" does not warn that disabling name
mangling will break
smbclient (assuming that local files have been created on the server with one of
the various reserved characters, perhaps over NFS for example).  But
... there are many
other servers, and I wouldn't be surprised if other servers have
sometimes returned files
created by NFS or Ceph or some cluster fs that contain reserved
characters, even if
it is illegal.

> The SMBecho is due to the keepalive failing
We (SMB/CIFS developers) would know that, but I doubt that all users
would realize that
(for example) creating a file over NFS with a reserved character and
then reexporting the
file over SMB with Samba configured with managled names off, or with a
server that
is less strict than Samba.   Seems like it would be better to print a
warning like:
                    "exiting due to invalid character found in file name"
rather than killing the session and ending up with the (to most users)
unehelpful error message.
Jeremy Allison Jan. 3, 2021, 4:13 a.m. UTC | #11
On Sat, Jan 02, 2021 at 09:45:53PM -0600, Steve French wrote:
>> So just creating a file containing : \ etc. doesn't do
>> this - you have to misconfigure the server FIRST.
>
>I agree that with Samba server this is less common (not sure how many
>vendors set that smb.conf

No one sets it by default to my knowledge.

>parm) but note that "man smb.conf" does not warn that disabling name
>mangling will break

Patches to the manpage welcome :-).

>smbclient (assuming that local files have been created on the server with one of
>the various reserved characters, perhaps over NFS for example).  But
>... there are many
>other servers, and I wouldn't be surprised if other servers have
>sometimes returned files
>created by NFS or Ceph or some cluster fs that contain reserved
>characters, even if
>it is illegal.

Sure - but that then becomes a possible CVE for these
filesystem clients if they don't protect themselves
against server attacks.

What does *your* client code do if a server returns a
filename containing a / ? If you pass it up, the upper
layers may screw things up badly.

>> The SMBecho is due to the keepalive failing
>We (SMB/CIFS developers) would know that, but I doubt that all users
>would realize that
>(for example) creating a file over NFS with a reserved character and
>then reexporting the
>file over SMB with Samba configured with managled names off, or with a
>server that
>is less strict than Samba.   Seems like it would be better to print a
>warning like:
>                    "exiting due to invalid character found in file name"
>rather than killing the session and ending up with the (to most users)
>unehelpful error message.

True. Again, patches welcome :-).
diff mbox series

Patch

From 03596841d4119840548676a9b6d916580baa618c Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 31 Dec 2020 21:12:19 -0600
Subject: [PATCH] smb3: allow files to be created with backslash in file name

Backslash is reserved in Windows (and SMB2/SMB3 by default) but
allowed in POSIX so must be remapped when POSIX extensions are
not enabled.

The default mapping for SMB3 mounts ("SFM") allows mapping backslash
(ie 0x5C in UTF8) to 0xF026 in UCS-2 (using the Unicode remapping
range reserved for these characters), but this was not mapped by
cifs.ko (unlike asterisk, greater than, question mark etc).  This patch
fixes that to allow creating files and directories with backslash
in the file or directory name.

Before this patch:
   touch "/mnt2/filewith\slash"
would return
   touch: setting times of '/mnt2/filewith\slash': Invalid argument

With the patch touch and mkdir with the backslash in the name works.

This problem was found while debugging
    https://bugzilla.kernel.org/show_bug.cgi?id=210961

Reported-by: Xiaoli Feng <xifeng@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifs_unicode.c |  6 ++++++
 fs/cifs/dir.c          | 10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 9bd03a231032..a65310c87db4 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -98,6 +98,9 @@  convert_sfm_char(const __u16 src_char, char *target)
 	case SFM_PERIOD:
 		*target = '.';
 		break;
+	case SFM_SLASH:
+		*target = '\\';
+		break;
 	default:
 		return false;
 	}
@@ -431,6 +434,9 @@  static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 	case '|':
 		dest_char = cpu_to_le16(SFM_PIPE);
 		break;
+	case '\\':
+		dest_char = cpu_to_le16(SFM_SLASH);
+		break;
 	case '.':
 		if (end_of_string)
 			dest_char = cpu_to_le16(SFM_PERIOD);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 68900f1629bf..62cc589e33cd 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -208,8 +208,14 @@  check_name(struct dentry *direntry, struct cifs_tcon *tcon)
 		     direntry->d_name.len >
 		     le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength)))
 		return -ENAMETOOLONG;
-
-	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)) {
+	/*
+	 * If POSIX extensions negotiated or if mapping reserved characters
+	 * (via SFM, the default on most mounts currently, then '\' is
+	 * remapped on the wire into the Unicode reserved range, 0xF026) then
+	 * do not need to reject get/set requests with backslash in path
+	 */
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) &&
+	    !(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SFM_CHR)) {
 		for (i = 0; i < direntry->d_name.len; i++) {
 			if (direntry->d_name.name[i] == '\\') {
 				cifs_dbg(FYI, "Invalid file name\n");
-- 
2.27.0