From 51932fd5fe9c3697e344344628172ddd70cd9c9a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 25 Aug 2014 16:21:24 -0700 Subject: [PATCH 01/10] s3: smbd: srvstr_push() was changed to never return -1, so don't check for that as an error. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/srvstr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/smbd/srvstr.c b/source3/smbd/srvstr.c index c069dd4..648c69f 100644 --- a/source3/smbd/srvstr.c +++ b/source3/smbd/srvstr.c @@ -65,7 +65,7 @@ ssize_t message_push_string(uint8 **outbuf, const char *str, int flags) result = srvstr_push((char *)tmp, SVAL(tmp, smb_flg2), tmp + buf_size, str, grow_size, flags); - if (result == (size_t)-1) { + if (result == 0) { DEBUG(0, ("srvstr_push failed\n")); return -1; } -- 2.1.0.rc2.206.gedb03e5 From 9249bbbec6c575ed34f36c357588720a866c410c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 25 Aug 2014 17:05:47 -0700 Subject: [PATCH 02/10] s3: smbd: Ensure types for all variables called 'len' used in srvstr_push() are correct. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/trans2.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 1e2c02e..70d29f2 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1594,7 +1594,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, uint64_t file_size = 0; uint64_t allocation_size = 0; uint64_t file_index = 0; - uint32_t len; + size_t len = 0; struct timespec mdate_ts, adate_ts, cdate_ts, create_date_ts; time_t mdate = (time_t)0, adate = (time_t)0, create_date = (time_t)0; char *nameptr; @@ -3077,7 +3077,8 @@ NTSTATUS smbd_do_qfsinfo(struct smbXsrv_connection *xconn, int *ret_data_len) { char *pdata, *end_data; - int data_len = 0, len; + int data_len = 0; + size_t len = 0; const char *vname = volume_label(talloc_tos(), SNUM(conn)); int snum = SNUM(conn); const char *fstype = lp_fstype(SNUM(conn)); @@ -3187,9 +3188,9 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAvail=%d\n", (unsigned int)st.st_ex_dev, (u STR_NOALIGN|STR_TERMINATE); SCVAL(pdata,l2_vol_cch,len); data_len = l2_vol_szVolLabel + len; - DEBUG(5,("smbd_do_qfsinfo : time = %x, namelen = %d, name = %s\n", + DEBUG(5,("smbd_do_qfsinfo : time = %x, namelen = %u, name = %s\n", (unsigned)convert_timespec_to_time_t(st.st_ex_ctime), - len, vname)); + (unsigned)len, vname)); break; case SMB_QUERY_FS_ATTRIBUTE_INFO: @@ -4426,6 +4427,7 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, uint64_t allocation_size = 0; uint64_t file_index = 0; uint32_t access_mask = 0; + size_t len = 0; if (INFO_LEVEL_IS_UNIX(info_level) && !lp_unix_extensions()) { return NT_STATUS_INVALID_LEVEL; @@ -4727,7 +4729,6 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, case SMB_QUERY_FILE_ALT_NAME_INFO: case SMB_FILE_ALTERNATE_NAME_INFORMATION: { - int len; char mangled_name[13]; DEBUG(10,("smbd_do_qfilepathinfo: SMB_FILE_ALTERNATE_NAME_INFORMATION\n")); if (!name_to_8_3(base_name,mangled_name, @@ -4746,7 +4747,6 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, case SMB_QUERY_FILE_NAME_INFO: { - int len; /* this must be *exactly* right for ACLs on mapped drives to work */ @@ -4777,7 +4777,6 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, case SMB_QUERY_FILE_ALL_INFO: case SMB_FILE_ALL_INFORMATION: { - int len; unsigned int ea_size = estimate_ea_size(conn, fsp, smb_fname); DEBUG(10,("smbd_do_qfilepathinfo: SMB_FILE_ALL_INFORMATION\n")); @@ -4810,7 +4809,6 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, case 0xFF12:/*SMB2_FILE_ALL_INFORMATION*/ { - int len; unsigned int ea_size = estimate_ea_size(conn, fsp, smb_fname); DEBUG(10,("smbd_do_qfilepathinfo: SMB2_FILE_ALL_INFORMATION\n")); @@ -5010,7 +5008,7 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, case SMB_QUERY_FILE_UNIX_LINK: { - int len; + int 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); - if (len == -1) { + if (link_len == -1) { return map_nt_error_from_unix(errno); } - buffer[len] = 0; + buffer[link_len] = 0; len = srvstr_push(dstart, flags2, pdata, buffer, PTR_DIFF(dend, pdata), -- 2.1.0.rc2.206.gedb03e5 From 83ee798e7697af2110b1701eadb32490edc8285f Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 25 Aug 2014 17:11:58 -0700 Subject: [PATCH 03/10] s3: smbd: Change the function signature of srvstr_push() from returning a length to returning an NTSTATUS with a length param. srvstr_push_fn() now returns an NTSTATUS reporting any string conversion failure. We need to get serious about returning character set conversion errors inside smbd. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- lib/util/samba_util.h | 1 + lib/util/string_wrappers.h | 8 +- source3/include/safe_string.h | 1 + source3/modules/vfs_default.c | 10 ++- source3/smbd/lanman.c | 9 ++- source3/smbd/proto.h | 4 +- source3/smbd/reply.c | 30 ++++++-- source3/smbd/srvstr.c | 57 ++++++++++++-- source3/smbd/trans2.c | 174 ++++++++++++++++++++++++++++++------------ 9 files changed, 221 insertions(+), 73 deletions(-) diff --git a/lib/util/samba_util.h b/lib/util/samba_util.h index f1f4c2d..528d373 100644 --- a/lib/util/samba_util.h +++ b/lib/util/samba_util.h @@ -63,6 +63,7 @@ do { \ #include "lib/util/memory.h" +#include "../libcli/util/ntstatus.h" #include "lib/util/string_wrappers.h" /** diff --git a/lib/util/string_wrappers.h b/lib/util/string_wrappers.h index fcc088c..1feea8c 100644 --- a/lib/util/string_wrappers.h +++ b/lib/util/string_wrappers.h @@ -57,6 +57,8 @@ char * __unsafe_string_function_usage_here__(void); size_t __unsafe_string_function_usage_here_size_t__(void); +NTSTATUS __unsafe_string_function_usage_here_NTSTATUS__(void); + #define CHECK_STRING_SIZE(d, len) (sizeof(d) != (len) && sizeof(d) != sizeof(char *)) /* if the compiler will optimize out function calls, then use this to tell if we are @@ -68,10 +70,10 @@ size_t __unsafe_string_function_usage_here_size_t__(void); ? __unsafe_string_function_usage_here_size_t__() \ : push_string_check_fn(dest, src, dest_len, flags)) -#define srvstr_push(base_ptr, smb_flags2, dest, src, dest_len, flags) \ +#define srvstr_push(base_ptr, smb_flags2, dest, src, dest_len, flags, ret_len) \ (CHECK_STRING_SIZE(dest, dest_len) \ - ? __unsafe_string_function_usage_here_size_t__() \ - : srvstr_push_fn(base_ptr, smb_flags2, dest, src, dest_len, flags)) + ? __unsafe_string_function_usage_here_NTSTATUS__() \ + : srvstr_push_fn(base_ptr, smb_flags2, dest, src, dest_len, flags, ret_len)) /* This allows the developer to choose to check the arguments to strlcpy. if the compiler will optimize out function calls, then diff --git a/source3/include/safe_string.h b/source3/include/safe_string.h index 03878b4..e77017c 100644 --- a/source3/include/safe_string.h +++ b/source3/include/safe_string.h @@ -62,6 +62,7 @@ #endif /* !_SPLINT_ */ +#include "../libcli/util/ntstatus.h" #include "lib/util/string_wrappers.h" #endif diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 3430cd0..3a3943b 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1178,10 +1178,16 @@ 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; inum_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(*out_data); + TALLOC_FREE(shadow_data); + return status; + } cur_pdata += 2 * sizeof(SHADOW_COPY_LABEL); DEBUGADD(10,("Label[%u]: '%s'\n",i,shadow_data->labels[i])); } diff --git a/source3/smbd/lanman.c b/source3/smbd/lanman.c index b7c74e9..ac4873d 100644 --- a/source3/smbd/lanman.c +++ b/source3/smbd/lanman.c @@ -3655,8 +3655,13 @@ static bool api_RNetServerGetInfo(struct smbd_server_connection *sconn, } if (uLevel != 20) { - srvstr_push(NULL, 0, p, info.info101->server_name, 16, - STR_ASCII|STR_UPPER|STR_TERMINATE); + size_t len = 0; + status = srvstr_push(NULL, 0, p, info.info101->server_name, 16, + STR_ASCII|STR_UPPER|STR_TERMINATE, &len); + if (!NT_STATUS_IS_OK(status)) { + errcode = W_ERROR_V(ntstatus_to_werror(status)); + goto out; + } } p += 16; if (uLevel > 0) { diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 692f582..bb4d5f6 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -1019,8 +1019,8 @@ bool is_share_read_only_for_token(const char *username, /* The following definitions come from smbd/srvstr.c */ -size_t srvstr_push_fn(const char *base_ptr, uint16 smb_flags2, void *dest, - const char *src, int dest_len, int flags); +NTSTATUS srvstr_push_fn(const char *base_ptr, uint16 smb_flags2, void *dest, + const char *src, int dest_len, int flags, size_t *ret_len); ssize_t message_push_string(uint8 **outbuf, const char *str, int flags); /* The following definitions come from smbd/statcache.c */ diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 4cb446f..8dba79b 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -1099,6 +1099,8 @@ void reply_ioctl(struct smb_request *req) switch (ioctl_code) { case IOCTL_QUERY_JOB_INFO: { + NTSTATUS status; + size_t len = 0; files_struct *fsp = file_fsp( req, SVAL(req->vwv+0, 0)); if (!fsp) { @@ -1109,15 +1111,25 @@ void reply_ioctl(struct smb_request *req) /* Job number */ SSVAL(p, 0, print_spool_rap_jobid(fsp->print_file)); - srvstr_push((char *)req->outbuf, req->flags2, p+2, + status = srvstr_push((char *)req->outbuf, req->flags2, p+2, lp_netbios_name(), 15, - STR_TERMINATE|STR_ASCII); + STR_TERMINATE|STR_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); + END_PROFILE(SMBioctl); + return; + } if (conn) { - srvstr_push((char *)req->outbuf, req->flags2, + status = srvstr_push((char *)req->outbuf, req->flags2, p+18, lp_servicename(talloc_tos(), SNUM(conn)), - 13, STR_TERMINATE|STR_ASCII); + 13, STR_TERMINATE|STR_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); + END_PROFILE(SMBioctl); + return; + } } else { memset(p+18, 0, 13); } @@ -5740,6 +5752,7 @@ void reply_printqueue(struct smb_request *req) char *p = blob; time_t qtime = spoolss_Time_to_time_t(&info[i].info2.submitted); int qstatus; + size_t len = 0; uint16_t qrapjobid = pjobid_to_rap(sharename, info[i].info2.job_id); @@ -5754,9 +5767,12 @@ void reply_printqueue(struct smb_request *req) SSVAL(p, 5, qrapjobid); SIVAL(p, 7, info[i].info2.size); SCVAL(p, 11, 0); - srvstr_push(blob, req->flags2, p+12, - info[i].info2.notify_name, 16, STR_ASCII); - + status = srvstr_push(blob, req->flags2, p+12, + info[i].info2.notify_name, 16, STR_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); + goto out; + } if (message_push_blob( &req->outbuf, data_blob_const( diff --git a/source3/smbd/srvstr.c b/source3/smbd/srvstr.c index 648c69f..e6a8541 100644 --- a/source3/smbd/srvstr.c +++ b/source3/smbd/srvstr.c @@ -24,16 +24,56 @@ /* Make sure we can't write a string past the end of the buffer */ -size_t srvstr_push_fn(const char *base_ptr, uint16 smb_flags2, void *dest, - const char *src, int dest_len, int flags) +NTSTATUS srvstr_push_fn(const char *base_ptr, uint16 smb_flags2, void *dest, + const char *src, int dest_len, int flags, size_t *ret_len) { + size_t len; + int saved_errno; + NTSTATUS status; + if (dest_len < 0) { - return 0; + return NT_STATUS_INVALID_PARAMETER; } + saved_errno = errno; + errno = 0; + /* 'normal' push into size-specified buffer */ - 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); + /* + * Paranoia - Filter out STATUS_MORE_ENTRIES. + * I don't think we can get this but it has a + * specific meaning to the client. + */ + if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { + status = NT_STATUS_UNSUCCESSFUL; + } + } + 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; + } + return status; } /******************************************************************* @@ -45,8 +85,9 @@ ssize_t message_push_string(uint8 **outbuf, const char *str, int flags) { size_t buf_size = smb_len(*outbuf) + 4; size_t grow_size; - size_t result; + size_t result = 0; uint8 *tmp; + NTSTATUS status; /* * We need to over-allocate, now knowing what srvstr_push will @@ -62,10 +103,10 @@ ssize_t message_push_string(uint8 **outbuf, const char *str, int flags) return -1; } - result = srvstr_push((char *)tmp, SVAL(tmp, smb_flg2), - tmp + buf_size, str, grow_size, flags); + status = srvstr_push((char *)tmp, SVAL(tmp, smb_flg2), + tmp + buf_size, str, grow_size, flags, &result); - if (result == 0) { + if (!NT_STATUS_IS_OK(status)) { DEBUG(0, ("srvstr_push failed\n")); return -1; } diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 70d29f2..bdecc60 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1602,6 +1602,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, bool was_8_3; int off; int pad = 0; + NTSTATUS status; *out_of_space = false; @@ -1684,9 +1685,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, if (flags2 & FLAGS2_UNICODE_STRINGS) { p += ucs2_align(base_data, p, 0); } - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE); + STR_TERMINATE, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { SCVAL(nameptr, -1, len - 2); @@ -1722,9 +1726,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, } p += 27; nameptr = p - 1; - len = srvstr_push(base_data, flags2, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE | STR_NOALIGN); + STR_TERMINATE | STR_NOALIGN, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { len -= 2; @@ -1747,7 +1754,6 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, { struct ea_list *file_list = NULL; size_t ea_len = 0; - NTSTATUS status; DEBUG(10,("smbd_marshall_dir_entry: SMB_FIND_EA_LIST\n")); if (!name_list) { @@ -1787,9 +1793,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, /* Push the ea_data followed by the name. */ p += fill_ea_buffer(ctx, p, space_remaining, conn, name_list); nameptr = p; - len = srvstr_push(base_data, flags2, + status = srvstr_push(base_data, flags2, p + 1, fname, PTR_DIFF(end_data, p+1), - STR_TERMINATE | STR_NOALIGN); + STR_TERMINATE | STR_NOALIGN, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { len -= 2; @@ -1842,9 +1851,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, memset(mangled_name,'\0',12); } mangled_name[12] = 0; - len = srvstr_push(base_data, flags2, + status = srvstr_push(base_data, flags2, p+2, mangled_name, 24, - STR_UPPER|STR_UNICODE); + STR_UPPER|STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } if (len < 24) { memset(p + 2 + len,'\0',24 - len); } @@ -1853,9 +1865,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, memset(p,'\0',26); } p += 2 + 24; - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(q,0,len); p += len; @@ -1889,9 +1904,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, SOFF_T(p,0,file_size); p += 8; SOFF_T(p,0,allocation_size); p += 8; SIVAL(p,0,mode); p += 4; - len = srvstr_push(base_data, flags2, + status = srvstr_push(base_data, flags2, p + 4, fname, PTR_DIFF(end_data, p+4), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(p,0,len); p += 4 + len; @@ -1932,9 +1950,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, SIVAL(p,0,ea_size); /* Extended attributes */ p +=4; } - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(q, 0, len); p += len; @@ -1964,9 +1985,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p += 4; /* this must *not* be null terminated or w2k gets in a loop trying to set an acl on a dir (tridge) */ - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(p, -4, len); p += len; @@ -2011,9 +2035,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p += 4; SIVAL(p,0,0); p += 4; /* Unknown - reserved ? */ SBVAL(p,0,file_index); p += 8; - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(q, 0, len); p += len; @@ -2069,9 +2096,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, memset(mangled_name,'\0',12); } mangled_name[12] = 0; - len = srvstr_push(base_data, flags2, + status = srvstr_push(base_data, flags2, p+2, mangled_name, 24, - STR_UPPER|STR_UNICODE); + STR_UPPER|STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SSVAL(p, 0, len); if (len < 24) { memset(p + 2 + len,'\0',24 - len); @@ -2083,9 +2113,12 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p += 26; SSVAL(p,0,0); p += 2; /* Reserved ? */ SBVAL(p,0,file_index); p += 8; - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE_ASCII); + STR_TERMINATE_ASCII, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(q,0,len); p += len; @@ -2121,17 +2154,23 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, DEBUG(10,("smbd_marshall_dir_entry: SMB_FIND_FILE_UNIX\n")); p = store_file_unix_basic(conn, p, NULL, &smb_fname->st); - len = srvstr_push(base_data, flags2, p, + status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), - STR_TERMINATE); + STR_TERMINATE, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } } else { DEBUG(10,("smbd_marshall_dir_entry: SMB_FIND_FILE_UNIX_INFO2\n")); p = store_file_unix_basic_info2(conn, p, NULL, &smb_fname->st); nameptr = p; p += 4; - len = srvstr_push(base_data, flags2, p, fname, - PTR_DIFF(end_data, p), 0); + status = srvstr_push(base_data, flags2, p, fname, + PTR_DIFF(end_data, p), 0, &len); + if (!NT_STATUS_IS_OK(status)) { + return false; + } SIVAL(nameptr, 0, len); } @@ -3181,11 +3220,14 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAvail=%d\n", (unsigned int)st.st_ex_dev, (u * this call so try fixing this by adding a terminating null to * the pushed string. The change here was adding the STR_TERMINATE. JRA. */ - len = srvstr_push( + status = srvstr_push( pdata, flags2, pdata+l2_vol_szVolLabel, vname, PTR_DIFF(end_data, pdata+l2_vol_szVolLabel), - STR_NOALIGN|STR_TERMINATE); + STR_NOALIGN|STR_TERMINATE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } SCVAL(pdata,l2_vol_cch,len); data_len = l2_vol_szVolLabel + len; DEBUG(5,("smbd_do_qfsinfo : time = %x, namelen = %u, name = %s\n", @@ -3218,9 +3260,12 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAvail=%d\n", (unsigned int)st.st_ex_dev, (u SIVAL(pdata,4,255); /* Max filename component length */ /* NOTE! the fstype must *not* be null terminated or win98 won't recognise it and will think we can't do long filenames */ - len = srvstr_push(pdata, flags2, pdata+12, fstype, + status = srvstr_push(pdata, flags2, pdata+12, fstype, PTR_DIFF(end_data, pdata+12), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } SIVAL(pdata,8,len); data_len = 12 + len; if (max_data_bytes >= 16 && data_len > max_data_bytes) { @@ -3234,8 +3279,11 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAvail=%d\n", (unsigned int)st.st_ex_dev, (u case SMB_QUERY_FS_LABEL_INFO: case SMB_FS_LABEL_INFORMATION: - len = srvstr_push(pdata, flags2, pdata+4, vname, - PTR_DIFF(end_data, pdata+4), 0); + status = srvstr_push(pdata, flags2, pdata+4, vname, + PTR_DIFF(end_data, pdata+4), 0, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } data_len = 4 + len; SIVAL(pdata,0,len); break; @@ -3251,9 +3299,12 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAvail=%d\n", (unsigned int)st.st_ex_dev, (u (str_checksum(get_local_machine_name())<<16)); /* Max label len is 32 characters. */ - len = srvstr_push(pdata, flags2, pdata+18, vname, + status = srvstr_push(pdata, flags2, pdata+18, vname, PTR_DIFF(end_data, pdata+18), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } SIVAL(pdata,12,len); data_len = 18+len; @@ -4735,10 +4786,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, True,conn->params)) { return NT_STATUS_NO_MEMORY; } - len = srvstr_push(dstart, flags2, + status = srvstr_push(dstart, flags2, pdata+4, mangled_name, PTR_DIFF(dend, pdata+4), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } data_size = 4 + len; SIVAL(pdata,0,len); *fixed_portion = 8; @@ -4750,10 +4804,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, /* this must be *exactly* right for ACLs on mapped drives to work */ - len = srvstr_push(dstart, flags2, + status = srvstr_push(dstart, flags2, pdata+4, dos_fname, PTR_DIFF(dend, pdata+4), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } DEBUG(10,("smbd_do_qfilepathinfo: SMB_QUERY_FILE_NAME_INFO\n")); data_size = 4 + len; SIVAL(pdata,0,len); @@ -4796,10 +4853,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, pdata += 24; SIVAL(pdata,0,ea_size); pdata += 4; /* EA info */ - len = srvstr_push(dstart, flags2, + status = srvstr_push(dstart, flags2, pdata+4, dos_fname, PTR_DIFF(dend, pdata+4), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } SIVAL(pdata,0,len); pdata += 4 + len; data_size = PTR_DIFF(pdata,(*ppdata)); @@ -4833,10 +4893,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, pdata += 0x60; - len = srvstr_push(dstart, flags2, + status = srvstr_push(dstart, flags2, pdata+4, dos_fname, PTR_DIFF(dend, pdata+4), - STR_UNICODE); + STR_UNICODE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } SIVAL(pdata,0,len); pdata += 4 + len; data_size = PTR_DIFF(pdata,(*ppdata)); @@ -5030,10 +5093,13 @@ NTSTATUS smbd_do_qfilepathinfo(connection_struct *conn, return map_nt_error_from_unix(errno); } buffer[link_len] = 0; - len = srvstr_push(dstart, flags2, + status = srvstr_push(dstart, flags2, pdata, buffer, PTR_DIFF(dend, pdata), - STR_TERMINATE); + STR_TERMINATE, &len); + if (!NT_STATUS_IS_OK(status)) { + return status; + } pdata += len; data_size = PTR_DIFF(pdata,(*ppdata)); @@ -8602,6 +8668,8 @@ static void call_trans2ioctl(connection_struct *conn, { char *pdata = *ppdata; files_struct *fsp = file_fsp(req, SVAL(req->vwv+15, 0)); + NTSTATUS status; + size_t len = 0; /* check for an invalid fid before proceeding */ @@ -8625,12 +8693,20 @@ static void call_trans2ioctl(connection_struct *conn, /* Job number */ SSVAL(pdata, 0, print_spool_rap_jobid(fsp->print_file)); - srvstr_push(pdata, req->flags2, pdata + 2, + status = srvstr_push(pdata, req->flags2, pdata + 2, lp_netbios_name(), 15, - STR_ASCII|STR_TERMINATE); /* Our NetBIOS name */ - srvstr_push(pdata, req->flags2, pdata+18, + STR_ASCII|STR_TERMINATE, &len); /* Our NetBIOS name */ + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); + return; + } + status = srvstr_push(pdata, req->flags2, pdata+18, lp_servicename(talloc_tos(), SNUM(conn)), 13, - STR_ASCII|STR_TERMINATE); /* Service name */ + STR_ASCII|STR_TERMINATE, &len); /* Service name */ + if (!NT_STATUS_IS_OK(status)) { + reply_nterror(req, status); + return; + } send_trans2_replies(conn, req, NT_STATUS_OK, *pparams, 0, *ppdata, 32, max_data_bytes); return; -- 2.1.0.rc2.206.gedb03e5 From aec6e8a5a4a41a38a37c18e4936db236e9f42199 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 11:36:41 -0700 Subject: [PATCH 04/10] s3: smbd: Change smbd_marshall_dir_entry() to return an NTSTATUS. Returns STATUS_MORE_ENTRIES on out of space. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/trans2.c | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index bdecc60..1a145c7 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1570,7 +1570,7 @@ static bool smbd_dirptr_lanman2_mode_fn(TALLOC_CTX *ctx, return true; } -static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, +static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, connection_struct *conn, uint16_t flags2, uint32_t info_level, @@ -1647,7 +1647,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, "for padding (wanted %u, had %d)\n", (unsigned int)pad, space_remaining )); - return false; /* Not finished - just out of space */ + return STATUS_MORE_ENTRIES; /* Not finished - just out of space */ } off += pad; @@ -1689,7 +1689,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { @@ -1730,7 +1730,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p, fname, PTR_DIFF(end_data, p), STR_TERMINATE | STR_NOALIGN, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { @@ -1757,7 +1757,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, DEBUG(10,("smbd_marshall_dir_entry: SMB_FIND_EA_LIST\n")); if (!name_list) { - return false; + return NT_STATUS_INVALID_PARAMETER; } if (requires_resume_key) { SIVAL(p,0,reskey); @@ -1787,7 +1787,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, "(wanted %u, had %d)\n", (unsigned int)PTR_DIFF(p + 255 + ea_len,pdata), space_remaining )); - return False; /* Not finished - just out of space */ + return STATUS_MORE_ENTRIES; /* Not finished - just out of space */ } /* Push the ea_data followed by the name. */ @@ -1797,7 +1797,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p + 1, fname, PTR_DIFF(end_data, p+1), STR_TERMINATE | STR_NOALIGN, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } if (flags2 & FLAGS2_UNICODE_STRINGS) { if (len > 2) { @@ -1855,7 +1855,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p+2, mangled_name, 24, STR_UPPER|STR_UNICODE, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } if (len < 24) { memset(p + 2 + len,'\0',24 - len); @@ -1869,7 +1869,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(q,0,len); p += len; @@ -1908,7 +1908,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p + 4, fname, PTR_DIFF(end_data, p+4), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(p,0,len); p += 4 + len; @@ -1954,7 +1954,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(q, 0, len); p += len; @@ -1989,7 +1989,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(p, -4, len); p += len; @@ -2039,7 +2039,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(q, 0, len); p += len; @@ -2100,7 +2100,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, p+2, mangled_name, 24, STR_UPPER|STR_UNICODE, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SSVAL(p, 0, len); if (len < 24) { @@ -2117,7 +2117,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE_ASCII, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(q,0,len); p += len; @@ -2158,7 +2158,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, fname, PTR_DIFF(end_data, p), STR_TERMINATE, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } } else { DEBUG(10,("smbd_marshall_dir_entry: SMB_FIND_FILE_UNIX_INFO2\n")); @@ -2169,7 +2169,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, status = srvstr_push(base_data, flags2, p, fname, PTR_DIFF(end_data, p), 0, &len); if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } SIVAL(nameptr, 0, len); } @@ -2198,7 +2198,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, break; default: - return false; + return NT_STATUS_INVALID_LEVEL; } if (PTR_DIFF(p,pdata) > space_remaining) { @@ -2207,7 +2207,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, "(wanted %u, had %d)\n", (unsigned int)PTR_DIFF(p,pdata), space_remaining )); - return false; /* Not finished - just out of space */ + return STATUS_MORE_ENTRIES; /* Not finished - just out of space */ } /* Setup the last entry pointer, as an offset from base_data */ @@ -2215,7 +2215,7 @@ static bool smbd_marshall_dir_entry(TALLOC_CTX *ctx, /* Advance the data pointer to the next slot */ *ppdata = p; - return true; + return NT_STATUS_OK; } bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, @@ -2248,6 +2248,7 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, struct smbd_dirptr_lanman2_state state; bool ok; uint64_t last_entry_off = 0; + NTSTATUS status; ZERO_STRUCT(state); state.conn = conn; @@ -2289,7 +2290,7 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, *got_exact_match = state.got_exact_match; - ok = smbd_marshall_dir_entry(ctx, + status = smbd_marshall_dir_entry(ctx, conn, flags2, info_level, @@ -2309,11 +2310,11 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, &last_entry_off); TALLOC_FREE(fname); TALLOC_FREE(smb_fname); - if (*out_of_space) { + if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { dptr_SeekDir(dirptr, prev_dirpos); return false; } - if (!ok) { + if (!NT_STATUS_IS_OK(status)) { return false; } -- 2.1.0.rc2.206.gedb03e5 From c9849794a0b54511a13be9e4c56d1b4cbd6e3487 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 11:40:19 -0700 Subject: [PATCH 05/10] s3: smbd: smbd_marshall_dir_entry() no longer needs explicit 'out_of_space' parameter. Handle this in the caller when it returns STATUS_MORE_ENTRIES. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/trans2.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 1a145c7..b950820 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1586,7 +1586,6 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, char *base_data, char **ppdata, char *end_data, - bool *out_of_space, uint64_t *last_entry_off) { char *p, *q, *pdata = *ppdata; @@ -1604,8 +1603,6 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, int pad = 0; NTSTATUS status; - *out_of_space = false; - ZERO_STRUCT(mdate_ts); ZERO_STRUCT(adate_ts); ZERO_STRUCT(create_date_ts); @@ -1642,7 +1639,6 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, pad -= off; if (pad && pad > space_remaining) { - *out_of_space = true; DEBUG(9,("smbd_marshall_dir_entry: out of space " "for padding (wanted %u, had %d)\n", (unsigned int)pad, @@ -1782,7 +1778,6 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, /* We need to determine if this entry will fit in the space available. */ /* Max string size is 255 bytes. */ if (PTR_DIFF(p + 255 + ea_len,pdata) > space_remaining) { - *out_of_space = true; DEBUG(9,("smbd_marshall_dir_entry: out of space " "(wanted %u, had %d)\n", (unsigned int)PTR_DIFF(p + 255 + ea_len,pdata), @@ -2202,7 +2197,6 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, } if (PTR_DIFF(p,pdata) > space_remaining) { - *out_of_space = true; DEBUG(9,("smbd_marshall_dir_entry: out of space " "(wanted %u, had %d)\n", (unsigned int)PTR_DIFF(p,pdata), @@ -2306,11 +2300,11 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, base_data, ppdata, end_data, - out_of_space, &last_entry_off); TALLOC_FREE(fname); TALLOC_FREE(smb_fname); if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { + *out_of_space = true; dptr_SeekDir(dirptr, prev_dirpos); return false; } -- 2.1.0.rc2.206.gedb03e5 From 11b40f42b714f52ef3cac7a004c83d8ccb04d3ca Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 14:49:37 -0700 Subject: [PATCH 06/10] s3: smbd: SMB2 - change smbd_dirptr_lanman2_entry() to return an NTSTATUS. Handle the errors correctly at the top level inside the SMB2 server. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 2 +- source3/smbd/smb2_find.c | 15 ++++++++++----- source3/smbd/trans2.c | 14 ++++++++------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index f78ce45..20ab75d 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -185,7 +185,7 @@ bool smbd_dirptr_get_entry(TALLOC_CTX *ctx, uint32_t *_mode, long *_prev_offset); -bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, +NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, connection_struct *conn, struct dptr_struct *dirptr, uint16 flags2, diff --git a/source3/smbd/smb2_find.c b/source3/smbd/smb2_find.c index 45b0890..af9995e 100644 --- a/source3/smbd/smb2_find.c +++ b/source3/smbd/smb2_find.c @@ -432,14 +432,13 @@ static struct tevent_req *smbd_smb2_find_send(TALLOC_CTX *mem_ctx, true); while (true) { - bool ok; bool got_exact_match = false; bool out_of_space = false; int space_remaining = in_output_buffer_length - off; SMB_ASSERT(space_remaining >= 0); - ok = smbd_dirptr_lanman2_entry(state, + status = smbd_dirptr_lanman2_entry(state, conn, fsp->dptr, smbreq->flags2, @@ -462,12 +461,18 @@ static struct tevent_req *smbd_smb2_find_send(TALLOC_CTX *mem_ctx, off = (int)PTR_DIFF(pdata, base_data); - if (!ok) { - if (num > 0) { + if (!NT_STATUS_IS_OK(status)) { + if (NT_STATUS_EQUAL(status, NT_STATUS_ILLEGAL_CHARACTER)) { + /* + * Bad character conversion on name. Ignore this + * entry. + */ + continue; + } else if (num > 0) { SIVAL(state->out_output_buffer.data, last_entry_off, 0); tevent_req_done(req); return tevent_req_post(req, ev); - } else if (out_of_space) { + } else if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { tevent_req_nterror(req, NT_STATUS_INFO_LENGTH_MISMATCH); return tevent_req_post(req, ev); } else { diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index b950820..2d6c261 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -2212,7 +2212,7 @@ static NTSTATUS smbd_marshall_dir_entry(TALLOC_CTX *ctx, return NT_STATUS_OK; } -bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, +NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, connection_struct *conn, struct dptr_struct *dirptr, uint16 flags2, @@ -2279,7 +2279,7 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, &mode, &prev_dirpos); if (!ok) { - return false; + return NT_STATUS_END_OF_FILE; } *got_exact_match = state.got_exact_match; @@ -2306,14 +2306,14 @@ bool smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { *out_of_space = true; dptr_SeekDir(dirptr, prev_dirpos); - return false; + return status; } if (!NT_STATUS_IS_OK(status)) { - return false; + return status; } *_last_entry_off = last_entry_off; - return true; + return NT_STATUS_OK; } static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, @@ -2337,13 +2337,14 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, { uint8_t align = 4; const bool do_pad = true; + NTSTATUS status; if (info_level >= 1 && info_level <= 3) { /* No alignment on earlier info levels. */ align = 1; } - return smbd_dirptr_lanman2_entry(ctx, conn, dirptr, flags2, + status = smbd_dirptr_lanman2_entry(ctx, conn, dirptr, flags2, path_mask, dirtype, info_level, requires_resume_key, dont_descend, ask_sharemode, align, do_pad, @@ -2351,6 +2352,7 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, space_remaining, out_of_space, got_exact_match, last_entry_off, name_list); + return NT_STATUS_IS_OK(status); } /**************************************************************************** -- 2.1.0.rc2.206.gedb03e5 From dbab1bd7ad67c3ff0e8c50f9d3f7fba1043e5f91 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 14:54:56 -0700 Subject: [PATCH 07/10] s3: smbd: Remove unneeded 'out_of_space' parameter from smbd_dirptr_lanman2_entry(). This can now be handled by checking for the STATUS_MORE_ENTRIES error return. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 1 - source3/smbd/smb2_find.c | 2 -- source3/smbd/trans2.c | 10 ++++++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index 20ab75d..5a8e3bd 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -201,7 +201,6 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, char *base_data, char *end_data, int space_remaining, - bool *out_of_space, bool *got_exact_match, int *_last_entry_off, struct ea_list *name_list); diff --git a/source3/smbd/smb2_find.c b/source3/smbd/smb2_find.c index af9995e..2dab86b 100644 --- a/source3/smbd/smb2_find.c +++ b/source3/smbd/smb2_find.c @@ -433,7 +433,6 @@ static struct tevent_req *smbd_smb2_find_send(TALLOC_CTX *mem_ctx, while (true) { bool got_exact_match = false; - bool out_of_space = false; int space_remaining = in_output_buffer_length - off; SMB_ASSERT(space_remaining >= 0); @@ -454,7 +453,6 @@ static struct tevent_req *smbd_smb2_find_send(TALLOC_CTX *mem_ctx, base_data, end_data, space_remaining, - &out_of_space, &got_exact_match, &last_entry_off, NULL); diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 2d6c261..e4d64e8 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -2228,7 +2228,6 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, char *base_data, char *end_data, int space_remaining, - bool *out_of_space, bool *got_exact_match, int *_last_entry_off, struct ea_list *name_list) @@ -2251,7 +2250,6 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, state.has_wild = dptr_has_wild(dirptr); state.got_exact_match = false; - *out_of_space = false; *got_exact_match = false; p = strrchr_m(path_mask,'/'); @@ -2304,7 +2302,6 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, TALLOC_FREE(fname); TALLOC_FREE(smb_fname); if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { - *out_of_space = true; dptr_SeekDir(dirptr, prev_dirpos); return status; } @@ -2339,6 +2336,8 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, const bool do_pad = true; NTSTATUS status; + *out_of_space = false; + if (info_level >= 1 && info_level <= 3) { /* No alignment on earlier info levels. */ align = 1; @@ -2350,8 +2349,11 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, align, do_pad, ppdata, base_data, end_data, space_remaining, - out_of_space, got_exact_match, + got_exact_match, last_entry_off, name_list); + if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { + *out_of_space = true; + } return NT_STATUS_IS_OK(status); } -- 2.1.0.rc2.206.gedb03e5 From 5b772ca741ea45d74ab04ea7b9c5b249fd927f7d Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 15:05:24 -0700 Subject: [PATCH 08/10] s3: smbd: Change get_lanman2_dir_entry() to return the full NTSTATUS. Handle the errors correctly at the level above inside the SMB1 server. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/trans2.c | 52 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index e4d64e8..6a66f3b 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -2313,7 +2313,7 @@ NTSTATUS smbd_dirptr_lanman2_entry(TALLOC_CTX *ctx, return NT_STATUS_OK; } -static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, +static NTSTATUS get_lanman2_dir_entry(TALLOC_CTX *ctx, connection_struct *conn, struct dptr_struct *dirptr, uint16 flags2, @@ -2327,23 +2327,19 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, char *base_data, char *end_data, int space_remaining, - bool *out_of_space, bool *got_exact_match, int *last_entry_off, struct ea_list *name_list) { uint8_t align = 4; const bool do_pad = true; - NTSTATUS status; - - *out_of_space = false; if (info_level >= 1 && info_level <= 3) { /* No alignment on earlier info levels. */ align = 1; } - status = smbd_dirptr_lanman2_entry(ctx, conn, dirptr, flags2, + return smbd_dirptr_lanman2_entry(ctx, conn, dirptr, flags2, path_mask, dirtype, info_level, requires_resume_key, dont_descend, ask_sharemode, align, do_pad, @@ -2351,10 +2347,6 @@ static bool get_lanman2_dir_entry(TALLOC_CTX *ctx, space_remaining, got_exact_match, last_entry_off, name_list); - if (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) { - *out_of_space = true; - } - return NT_STATUS_IS_OK(status); } /**************************************************************************** @@ -2628,7 +2620,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd out_of_space = True; finished = False; } else { - finished = !get_lanman2_dir_entry(ctx, + ntstatus = get_lanman2_dir_entry(ctx, conn, dirptr, req->flags2, @@ -2636,14 +2628,24 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd requires_resume_key,dont_descend, ask_sharemode, &p,pdata,data_end, - space_remaining, &out_of_space, + space_remaining, &got_exact_match, &last_entry_off, ea_list); + if (NT_STATUS_EQUAL(ntstatus, + NT_STATUS_ILLEGAL_CHARACTER)) { + /* + * Bad character conversion on name. Ignore this + * entry. + */ + continue; + } + if (NT_STATUS_EQUAL(ntstatus, STATUS_MORE_ENTRIES)) { + out_of_space = true; + } else { + finished = !NT_STATUS_IS_OK(ntstatus); + } } - if (finished && out_of_space) - finished = False; - if (!finished && !out_of_space) numentries++; @@ -3004,7 +3006,7 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd out_of_space = True; finished = False; } else { - finished = !get_lanman2_dir_entry(ctx, + ntstatus = get_lanman2_dir_entry(ctx, conn, dirptr, req->flags2, @@ -3012,14 +3014,24 @@ total_data=%u (should be %u)\n", (unsigned int)total_data, (unsigned int)IVAL(pd requires_resume_key,dont_descend, ask_sharemode, &p,pdata,data_end, - space_remaining, &out_of_space, + space_remaining, &got_exact_match, &last_entry_off, ea_list); + if (NT_STATUS_EQUAL(ntstatus, + NT_STATUS_ILLEGAL_CHARACTER)) { + /* + * Bad character conversion on name. Ignore this + * entry. + */ + continue; + } + if (NT_STATUS_EQUAL(ntstatus, STATUS_MORE_ENTRIES)) { + out_of_space = true; + } else { + finished = !NT_STATUS_IS_OK(ntstatus); + } } - if (finished && out_of_space) - finished = False; - if (!finished && !out_of_space) numentries++; -- 2.1.0.rc2.206.gedb03e5 From 23abf89763eed3178b6ccf2f7724d73e03c10033 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 26 Aug 2014 16:39:56 -0700 Subject: [PATCH 09/10] s3: smbd: Fix a tricky slow-path case - don't return a mangled name for a name that cannot be converted. For a name that contains an illegal Windows character, the directory listing code returns the mangled 8.3 name as the primary name for the file. If the original (non-mangled) filename cannot be converted to UCS2 on the wire via iconv due to conversion error, we should skip that name when returning a directory listing, as we can't map back from a returned 8.3 name to a usable non-mangled filename if the client sends it back to us. As this is only done in a very slow path (name must be mangled) I don't feel too bad about using a talloc/free pair here. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- source3/smbd/trans2.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index 6a66f3b..f1ccc7e 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -1469,6 +1469,31 @@ static bool smbd_dirptr_lanman2_match_fn(TALLOC_CTX *ctx, /* Mangle fname if it's an illegal name. */ if (mangle_must_mangle(dname, state->conn->params)) { + /* + * Slow path - ensure we can push the original name as UCS2. If + * not, then just don't return this name. + */ + NTSTATUS status; + size_t ret_len = 0; + size_t len = (strlen(dname) + 2) * 4; /* Allow enough space. */ + uint8 *tmp = talloc_array(talloc_tos(), + uint8, + len); + + status = srvstr_push(NULL, + FLAGS2_UNICODE_STRINGS, + tmp, + dname, + len, + STR_TERMINATE, + &ret_len); + + TALLOC_FREE(tmp); + + if (!NT_STATUS_IS_OK(status)) { + return false; + } + ok = name_to_8_3(dname, mangled_name, true, state->conn->params); if (!ok) { -- 2.1.0.rc2.206.gedb03e5 From 90dbca9247e5a800dfc837d3d1ba582868c20d75 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 27 Aug 2014 13:15:29 -0700 Subject: [PATCH 10/10] Add test suite for iconv conversion fail of bad names over SMB1/SMB3. Bug 10775 - smbd crashes when accessing garbage filenames https://bugzilla.samba.org/show_bug.cgi?id=10775 Signed-off-by: Jeremy Allison --- selftest/target/Samba3.pm | 37 ++++++++ source3/script/tests/test_smbclient_s3.sh | 144 ++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+) diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index 5544105..de40ced 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -873,6 +873,9 @@ sub provision($$$$$$) my $msdfs_deeppath="$msdfs_shrdir/deeppath"; push(@dirs,$msdfs_deeppath); + my $badnames_shrdir="$shrdir/badnames"; + push(@dirs,$badnames_shrdir); + # this gets autocreated by winbindd my $wbsockdir="$prefix_abs/winbindd"; my $wbsockprivdir="$lockdir/winbindd_privileged"; @@ -925,6 +928,36 @@ sub provision($$$$$$) symlink "msdfs:$server_ip\\ro-tmp", "$msdfs_shrdir/msdfs-src1"; symlink "msdfs:$server_ipv6\\ro-tmp", "$msdfs_shrdir/deeppath/msdfs-src2"; + ## + ## create bad names in $badnames_shrdir + ## + ## (An invalid name, would be mangled to 8.3). + my $badname_target = "$badnames_shrdir/\340|\231\216\377\177"; + unless (open(BADNAME_TARGET, ">$badname_target")) { + warn("Unable to open $badname_target"); + return undef; + } + close(BADNAME_TARGET); + chmod 0666, $badname_target; + + ## (A bad name, would not be mangled to 8.3). + my $badname_target = "$badnames_shrdir/\240\276\346\327\377\177"; + unless (open(BADNAME_TARGET, ">$badname_target")) { + warn("Unable to open $badname_target"); + return undef; + } + close(BADNAME_TARGET); + chmod 0666, $badname_target; + + ## (A bad good name). + my $badname_target = "$badnames_shrdir/blank.txt"; + unless (open(BADNAME_TARGET, ">$badname_target")) { + warn("Unable to open $badname_target"); + return undef; + } + close(BADNAME_TARGET); + chmod 0666, $badname_target; + my $conffile="$libdir/server.conf"; my $nss_wrapper_pl = "$ENV{PERL} $self->{srcdir}/lib/nss_wrapper/nss_wrapper.pl"; @@ -1182,6 +1215,10 @@ sub provision($$$$$$) fruit:metadata = netatalk fruit:locking = netatalk fruit:encoding = native + +[badname-tmp] + path = $badnames_shrdir + guest ok = yes "; close(CONF); diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh index 596cd42..67ac94a 100755 --- a/source3/script/tests/test_smbclient_s3.sh +++ b/source3/script/tests/test_smbclient_s3.sh @@ -722,6 +722,146 @@ EOF fi } +# Test accessing an share with bad names (won't convert). +test_bad_names() +{ + cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/badname-tmp -I $SERVER_IP $ADDARGS -c ls 2>&1' + eval echo "$cmd" + out=`eval $cmd` + ret=$? + + if [ $ret != 0 ] ; then + echo "$out" + echo "failed accessing badname-tmp (SMB1) with error $ret" + false + return + fi + + echo "$out" | wc -l 2>&1 | grep 6 + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep of number of lines (1) failed with $ret" + false + fi + + echo "$out" | grep 'Domain=.*OS=.*Server=' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (1) failed with $ret" + false + fi + + echo "$out" | grep '^ \. *D' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (2) failed with $ret" + false + fi + + echo "$out" | grep '^ \.\. *D' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (3) failed with $ret" + false + fi + + echo "$out" | grep '^ blank.txt *N' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (4) failed with $ret" + false + fi + + echo "$out" | grep '^ *$' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (5) failed with $ret" + false + fi + + echo "$out" | grep 'blocks of size.*blocks available' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - grep (6) failed with $ret" + false + fi + + # Now check again with -mSMB3 + cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/badname-tmp -I $SERVER_IP -mSMB3 $ADDARGS -c ls 2>&1' + eval echo "$cmd" + out=`eval $cmd` + ret=$? + + if [ $ret != 0 ] ; then + echo "$out" + echo "failed accessing badname-tmp (SMB3) with error $ret" + false + return + fi + + echo "$out" | wc -l 2>&1 | grep 6 + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep of number of lines (1) failed with $ret" + false + fi + + echo "$out" | grep 'Domain=.*OS=.*Server=' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (1) failed with $ret" + false + fi + + echo "$out" | grep '^ \. *D' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (2) failed with $ret" + false + fi + + echo "$out" | grep '^ \.\. *D' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (3) failed with $ret" + false + fi + + echo "$out" | grep '^ blank.txt *N' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (4) failed with $ret" + false + fi + + echo "$out" | grep '^ *$' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (5) failed with $ret" + false + fi + + echo "$out" | grep 'blocks of size.*blocks available' + ret=$? + if [ $ret != 0 ] ; then + echo "$out" + echo "failed listing \\badname-tmp - SMB3 grep (6) failed with $ret" + false + fi +} LOGDIR_PREFIX=test_smbclient_s3 @@ -798,6 +938,10 @@ testit "list with backup privilege" \ test_backup_privilege_list || \ failed=`expr $failed + 1` +testit "list a share with bad names (won't convert)" \ + test_bad_names || \ + failed=`expr $failed + 1` + testit "rm -rf $LOGDIR" \ rm -rf $LOGDIR || \ failed=`expr $failed + 1` -- 2.1.0.rc2.206.gedb03e5