Bug 10775 - smbd crashes when accessing garbage filenames
smbd crashes when accessing garbage filenames
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.1.9
All Solaris
: P5 major
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-22 05:39 UTC by Jonathan Matthew
Modified: 2014-11-08 00:19 UTC (History)
1 user (show)

See Also:


Attachments
debug 10 log showing a file create crashing (14.17 KB, text/x-log)
2014-08-22 05:39 UTC, Jonathan Matthew
no flags Details
packet capture (12.66 KB, application/vnd.tcpdump.pcap)
2014-08-22 22:50 UTC, Jonathan Matthew
no flags Details
crashy files (3.50 KB, application/x-tar)
2014-08-22 22:51 UTC, Jonathan Matthew
no flags Details
git-am fix for master. (56.59 KB, patch)
2014-08-28 19:36 UTC, Jeremy Allison
no flags Details
Updated git-am fix for master/4.1.x (56.86 KB, patch)
2014-08-29 17:22 UTC, Jeremy Allison
no flags Details
Current proposed fix for master. (59.23 KB, patch)
2014-09-12 23:38 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Matthew 2014-08-22 05:39:23 UTC
Created attachment 10215 [details]
debug 10 log showing a file create crashing

Samba 4.1.9 on omnios r151010 is crashing when Windows 7 clients using SMB2 attempt to open files with names containing garbage.  Samba is not involved in the creation of the files (so it's not samba's fault they have weird names), it's just being used to look at them.

The files are generated by buggy code running on a linux box that nfs mounts off the omnios box which also runs samba.  The code typically forgets to null terminate strings or uses sprintf badly when constructing filenames, so the filename contains stack garbage or other mysteries.  The code is being written by hundreds of students working on assignments (we're a university faculty), so of course they introduce exciting new bugs constantly.

The stack trace is the same as bug 10716, but I have the patch from that bug applied.  The test case from bug 10716 no longer crashes, but the garbage filenames we're seeing here still cause it to crash.

We're using gnu libiconv 1.14 (from pkgsrc) and we have unix charset = UTF-8.

One thing that seems notable is that the client is using a mangled filename, which is ascii, so the initial encoding checks in smbd_smb2_request_process_create don't catch it.

Some debug log attached.  Lots of "convert_string_talloc: Conversion error: Illegal multibyte sequence", but apparently not where it'd cause the operation to fail early.

I've copied some files that trigger the crash to a separate samba instance, so I can test patches there if that helps.
Comment 1 Jeremy Allison 2014-08-22 16:45:03 UTC
I'm pretty sure this is a duplicate of #10716. Can you try the patch attached there. It's ready to go into 4.1.next, 4.0.next - just waiting for the next release.

Cheers,

Jeremy.

*** This bug has been marked as a duplicate of bug 10716 ***
Comment 2 Jeremy Allison 2014-08-22 16:46:08 UTC
Ah, sorry. Didn't notice you've got the patch from bug 10716 already attached.

Reopening...
Comment 3 Jeremy Allison 2014-08-22 16:48:43 UTC
Can you upload a tarball of the files in the directory with the 'bad' names, plus a wireshark capture trace showing an attempt to open a file that crashes smbd please ?

I need to reproduce using smbclient so I can create a good regression test case when I fix this :-).

Thanks,

Jeremy.
Comment 4 Jonathan Matthew 2014-08-22 22:50:35 UTC
Created attachment 10220 [details]
packet capture

This starts after the client has already connected and authenticated, but it has the important bits - the find request that lists the weird files, and the create request that crashes smbd.
Comment 5 Jonathan Matthew 2014-08-22 22:51:13 UTC
Created attachment 10221 [details]
crashy files
Comment 6 Jeremy Allison 2014-08-22 22:59:24 UTC
Ah, got it. The client is getting access to these files via the short-name, which we're still creating even after the iconv call fails. We shouldn't - if the iconv fails we should filter these files out completely from the directory listing.
Comment 7 Jeremy Allison 2014-08-23 00:08:50 UTC
Ok, I think I see how to fix this.... Give me the weekend to fix it up and I'll give you a patch to try.

Thanks !

Jeremy.
Comment 8 Jeremy Allison 2014-08-26 22:40:38 UTC
Still working on the fix for master for this.
Comment 9 Jeremy Allison 2014-08-27 15:19:53 UTC
OK, got a working patch for master for this. Just working on adding a regression test !
Comment 10 Jeremy Allison 2014-08-28 19:36:53 UTC
Created attachment 10237 [details]
git-am fix for master.

This patchset addresses an interesting bug
we've had for a while.

Consider a directory exported by Samba as
the default utf8. An "evil" (tm) user with
write access in the directory creates files
within it with the non-utf8 sequences such
as:

\340|\231\216\377\177

and

\240\276\346\327\377\177

You can also create such files over NFS,
if the directory is so exported.

When a client asks us to list a directory,
we read the filenames and try and write
them as UCS2 into the outgoing buffer.
The conversion fails, and so a name of a
double zero byte is written. However,
the mangling of the bad name to an alternate
8.3 name works, and so the client has a
name it can use to access these files.

So the client requests an open on the 8.3
name, we map it back internally to the
(bad) unconvertable name, and later when
we're trying to do share mode things with
that name, we blow up and (harmlessly)
segfault. Oops :-).

I thought about this for a while, and
decided the best way to fix this is to
simply not return such files in a directory
listing. We could simply return an error on
a directory listing containing such names,
but then that would allow anyone who can
write into a directory to create a non-convertable
filename, and prevent listing of existing good
files that are there for all other users.

This was a little tricky to implement the
right way, as our main function to push
strings into a client return buffer, srvstr_push()
doesn't report conversion errors back. What
we really need to do is convert the listing
functions to propagate an NTSTATUS return
all the way back to the top level SMB1/SMB2
file server code, so we can sanely and in
one place make policy decisions like what
to do in conversion failures etc. Doing this
actually simplified quite a bit of code,
and actually allowed me to remove a hated
voodoo 'bool *' flag parameter (out_of_space),
so I'm quite pleased with how much easier
it makes this code to understand :-).

The early work in this patchset fixes
up the return paths to propagate an NTSTATUS,
and the last parts actually do the fix to
filter out the bad conversions.

There's only one tricky case, explained
in patch [0009]. The last patch is a regression
test suite change that ensures we behave
correctly in the face of bad filenames, including
those containing a non-mappable Windows
character (such as :).
Comment 11 Jeremy Allison 2014-08-29 17:22:47 UTC
Created attachment 10240 [details]
Updated git-am fix for master/4.1.x

After first round of review from ddiss@suse.de (Thanks David !).
Comment 12 Jeremy Allison 2014-09-12 23:38:05 UTC
Created attachment 10287 [details]
Current proposed fix for master.
Comment 13 Jeremy Allison 2014-11-08 00:19:43 UTC
I think we're just going to fix this in 4.2.x. It's a bit invasive to back-port to 4.1.x or earlier.

Jeremy.