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.
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 ***
Ah, sorry. Didn't notice you've got the patch from bug 10716 already attached. Reopening...
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.
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.
Created attachment 10221 [details] crashy files
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.
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.
Still working on the fix for master for this.
OK, got a working patch for master for this. Just working on adding a regression test !
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 :).
Created attachment 10240 [details] Updated git-am fix for master/4.1.x After first round of review from ddiss@suse.de (Thanks David !).
Created attachment 10287 [details] Current proposed fix for master.
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.