[PATCH] Fix bug 10775 - smbd crashes when accessing garbage filenames

[David Disseldorp]

Hi Jeremy,

On Thu, 28 Aug 2014 13:07:58 -0700, Jeremy Allison wrote:

...
> 
> 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 [0010] is
> a regression test suite change that ensures we behave
> correctly in the face of bad filenames, including
> those containing an invalid Windows filename
> character (such as ':').
> 
> Reviews welcome !

s3: smbd: srvstr_push() was changed to never return -1, so don't check for that as an error.

Looks good.


s3: smbd: Ensure types for all variables called 'len' used in srvstr_push() are correct.

                case SMB_QUERY_FILE_UNIX_LINK:
                        {
-                               int len;
+                               ssize_t link_len = 0;
                                char *buffer = talloc_array(mem_ctx, char, PATH_MAX+1);
 
                                if (!buffer) {
@@ -5025,13 +5023,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn,
 #else
                                return NT_STATUS_DOS(ERRDOS, ERRbadlink);
 #endif
-                               len = SMB_VFS_READLINK(conn,
+                               link_len = SMB_VFS_READLINK(conn,
                                                       smb_fname->base_name,
                                                       buffer, PATH_MAX);

smb_vfs_call_readlink() returns an int, so this specific type change
isn't necessary.


s3: smbd: Change the function signature of srvstr_push() from returning a length to returning an NTSTATUS with a length param.

diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
index 3430cd0..adaf3ff 100644
--- a/source3/modules/vfs_default.c
+++ b/source3/modules/vfs_default.c
@@ -1178,10 +1178,15 @@ static NTSTATUS vfswrap_fsctl(struct vfs_handle_struct *handle,
                          shadow_data->num_volumes, fsp_str_dbg(fsp)));
                if (labels && shadow_data->labels) {
                        for (i=0; i<shadow_data->num_volumes; i++) {
-                               srvstr_push(cur_pdata, req_flags,
+                               size_t len = 0;
+                               status = srvstr_push(cur_pdata, req_flags,
                                            cur_pdata, shadow_data->labels[i],
                                            2 * sizeof(SHADOW_COPY_LABEL),
-                                           STR_UNICODE|STR_TERMINATE);
+                                           STR_UNICODE|STR_TERMINATE, &len);
+                               if (!NT_STATUS_IS_OK(status)) {
+                                       TALLOC_FREE(shadow_data);
+                                       return status;
+                               }

Looks like cur_pdata/*out_data leaks on error here.

-       return push_string_base(base_ptr, smb_flags2, dest, src,
+       len = push_string_base(base_ptr, smb_flags2, dest, src,
                                dest_len, flags);
+
+       if (errno != 0) {
+               /*
+                * Special case E2BIG, EILSEQ, EINVAL
+                * as they mean conversion errors here,
+                * but we don't generically map them as
+                * they can mean different things in
+                * generic filesystem calls (such as
+                * read xattrs).
+                */
+               if (errno == E2BIG || errno == EILSEQ || errno == EINVAL) {
+                       status = NT_STATUS_ILLEGAL_CHARACTER;
+               } else {
+                       status = map_nt_error_from_unix_common(errno);
+               }
+               DEBUG(10,("character conversion failure "
+                       "on string (%s) (%s)\n",
+                       src, strerror(errno)));
+       } else {
+               /* Success - restore untouched errno. */
+               errno = saved_errno;
+               *ret_len = len;
+               status = NT_STATUS_OK;
+       }

Can't say I'm a fan of the errno-catch error handling here, but it
looks like propagating error values all the way through to
convert_string_error_handle() would be a pretty massive change :-/


s3: smbd: Change smbd_marshall_dir_entry() to return an NTSTATUS. Returns STATUS_MORE_ENTRIES on out of space.
s3: smbd: smbd_marshall_dir_entry() no longer needs explicit 'out_of_space' parameter.

srvstr_push() EAGAIN, EINTR, ENOBUFS and EWOULDBLOCK errors would also
be propagated through as STATUS_MORE_ENTRIES. Could we get these from
the iconv layer? I think I'd prefer to keep the explicit out_of_space
parameter, rather than audit all return paths for potential
STATUS_MORE_ENTRIES values.
Am going to stop here. I'd like to see what the others think of the
approach taken here.

Cheers, David