From 02ae847b45375091cc9c0ef76c49b6b1edcdb4e8 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 2 Feb 2024 08:10:54 +0100 Subject: [PATCH] smbd: return errors from token_contains_name() Invalid names in "valid users", "invalid users", "read list", "write list", "veto files" and "hide files" are logged and ignored, but a failure to contact winbind or a DC from winbind, or a memory allocation failure, now all trigger a failure of the tree connect. Manually tested with smbclient with the following hack in winbindd: ---8<--- $ git di source3/winbindd/winbindd_cache.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c index c889489dbbbc..8ccf0a28e11a 100644 --- a/source3/winbindd/winbindd_cache.c +++ b/source3/winbindd/winbindd_cache.c @@ -1821,6 +1821,13 @@ NTSTATUS wb_cache_name_to_sid(struct winbindd_domain *domain, ZERO_STRUCTP(sid); *type = SID_NAME_UNKNOWN; + if (strequal(name, "unknown")) { + return NT_STATUS_OK; + } + if (strequal(name, "iotimeout")) { + return NT_STATUS_IO_TIMEOUT; + } + status = wcache_name_to_sid(domain, domain_name, name, sid, type); if (!NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND)) { return status; ---8<--- veto files = ../unknown/file1/../slow/file2 $ bin/smbclient -U slow%x //localhost/test -c quit $ In the log: [2024/03/04 15:21:33.659356, 1, pid=977167, effective(0, 0), real(0, 0)] ../../source3/lib/util_namearray.c:128(token_contains_name) token_contains_name: lookup_name 'unknown' failed veto files = ../iotimeout/file1/../slow/file2 $ bin/smbclient -U slow%x //localhost/test -c quit tree connect failed: NT_STATUS_LOGON_FAILURE $ [2024/03/04 15:22:15.655811, 0, pid=977177, effective(0, 0), real(0, 0)] ../../source3/lib/util_namearray.c:131(token_contains_name) token_contains_name: lookup_name 'iotimeout' failed NT_STATUS_NO_SUCH_DOMAIN [2024/03/04 15:22:15.655846, 1, pid=977177, effective(0, 0), real(0, 0)] ../../source3/smbd/uid.c:381(change_to_user_impersonate) change_to_user_impersonate: SMB user slow (unix user slow) not permitted access to share test. [2024/03/04 15:22:15.655855, 0, pid=977177, effective(0, 0), real(0, 0)] ../../source3/smbd/smb2_service.c:689(make_connection_snum) make_connection_snum: Can't become connected user! Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher --- source3/include/proto.h | 3 +- source3/lib/util_namearray.c | 58 ++++++++++++++---------- source3/smbd/proto.h | 6 ++- source3/smbd/share_access.c | 85 ++++++++++++++++++++++++------------ source3/smbd/uid.c | 21 +++++++-- 5 files changed, 116 insertions(+), 57 deletions(-) diff --git a/source3/include/proto.h b/source3/include/proto.h index 7ba9a18bd89..48d38a93a79 100644 --- a/source3/include/proto.h +++ b/source3/include/proto.h @@ -324,7 +324,8 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, const char *domain, const char *sharename, const struct security_token *token, - const char *name); + const char *name, + bool *match); void set_namearray(TALLOC_CTX *mem_ctx, const char *namelist, const struct security_token *token, diff --git a/source3/lib/util_namearray.c b/source3/lib/util_namearray.c index 0b191ad9e87..0a8b01d246c 100644 --- a/source3/lib/util_namearray.c +++ b/source3/lib/util_namearray.c @@ -72,13 +72,16 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, const char *domain, const char *sharename, const struct security_token *token, - const char *name) + const char *name, + bool *match) { const char *prefix; struct dom_sid sid; enum lsa_SidType type; NTSTATUS status; + *match = false; + if (username != NULL) { size_t domain_len = domain != NULL ? strlen(domain) : 0; @@ -103,14 +106,13 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, } if (name == NULL) { - /* This is too security sensitive, better panic than return a - * result that might be interpreted in a wrong way. */ - smb_panic("substitutions failed"); + return false; } if ( string_to_sid( &sid, name ) ) { DEBUG(5,("token_contains_name: Checking for SID [%s] in token\n", name)); - return nt_token_check_sid( &sid, token ); + *match = nt_token_check_sid( &sid, token ); + return true; } if (!do_group_checks(&name, &prefix)) { @@ -122,15 +124,17 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, &sid, &type); if (!NT_STATUS_IS_OK(status)) { - DEBUG(5, ("lookup_name %s failed\n", name)); - return False; + DBG_ERR("lookup_name '%s' failed %s\n", + name, nt_errstr(status)); + return false; } if (type != SID_NAME_USER) { - DEBUG(5, ("%s is a %s, expected a user\n", - name, sid_type_lookup(type))); - return False; + DBG_WARNING("%s is a %s, expected a user\n", + name, sid_type_lookup(type)); + return true; } - return nt_token_check_sid(&sid, token); + *match = nt_token_check_sid(&sid, token); + return true; } for (/* initialized above */ ; *prefix != '\0'; prefix++) { @@ -144,17 +148,19 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, &sid, &type); if (!NT_STATUS_IS_OK(status)) { - DEBUG(5, ("lookup_name %s failed\n", name)); - return False; + DBG_ERR("lookup_name '%s' failed %s\n", + name, nt_errstr(status)); + return false; } if ((type != SID_NAME_DOM_GRP) && (type != SID_NAME_ALIAS) && (type != SID_NAME_WKN_GRP)) { - DEBUG(5, ("%s is a %s, expected a group\n", - name, sid_type_lookup(type))); - return False; + DBG_WARNING("%s is a %s, expected a group\n", + name, sid_type_lookup(type)); + return true; } if (nt_token_check_sid(&sid, token)) { + *match = true; return True; } continue; @@ -162,6 +168,7 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, if (*prefix == '&') { if (username) { if (user_in_netgroup(mem_ctx, username, name)) { + *match = true; return True; } } @@ -169,7 +176,7 @@ bool token_contains_name(TALLOC_CTX *mem_ctx, } smb_panic("got invalid prefix from do_groups_check"); } - return False; + return true; } /******************************************************************* @@ -193,6 +200,7 @@ void set_namearray(TALLOC_CTX *mem_ctx, char *namelist = NULL; const char *p = NULL; size_t num_entries; + bool ok; *_name_array = NULL; @@ -249,12 +257,16 @@ void set_namearray(TALLOC_CTX *mem_ctx, return; } - match = token_contains_name(talloc_tos(), - NULL, - NULL, - NULL, - token, - username); + ok = token_contains_name(talloc_tos(), + NULL, + NULL, + NULL, + token, + username, + &match); + if (!ok) { + continue; + } if (!match) { continue; } diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index ba7218f4d1c..90387aeb6c3 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -1011,13 +1011,15 @@ bool token_contains_name_in_list(const char *username, const char *domain, const char *sharename, const struct security_token *token, - const char **list); + const char **list, + bool *match); bool user_ok_token(const char *username, const char *domain, const struct security_token *token, int snum); bool is_share_read_only_for_token(const char *username, const char *domain, const struct security_token *token, - connection_struct *conn); + connection_struct *conn, + bool *_read_only); /* The following definitions come from smbd/srvstr.c */ diff --git a/source3/smbd/share_access.c b/source3/smbd/share_access.c index ef7c0c1f932..cd38ddd1ed4 100644 --- a/source3/smbd/share_access.c +++ b/source3/smbd/share_access.c @@ -37,24 +37,29 @@ bool token_contains_name_in_list(const char *username, const char *domain, const char *sharename, const struct security_token *token, - const char **list) + const char **list, + bool *match) { + *match = false; if (list == NULL) { - return False; + return true; } while (*list != NULL) { TALLOC_CTX *frame = talloc_stackframe(); - bool ret; + bool ok; - ret = token_contains_name(frame, username, domain, sharename, - token, *list); + ok = token_contains_name(frame, username, domain, sharename, + token, *list, match); TALLOC_FREE(frame); - if (ret) { + if (!ok) { + return false; + } + if (*match) { return true; } list += 1; } - return False; + return true; } /* @@ -75,22 +80,34 @@ bool user_ok_token(const char *username, const char *domain, { const struct loadparm_substitution *lp_sub = loadparm_s3_global_substitution(); + bool match; + bool ok; if (lp_invalid_users(snum) != NULL) { - if (token_contains_name_in_list(username, domain, - lp_servicename(talloc_tos(), lp_sub, snum), - token, - lp_invalid_users(snum))) { + ok = token_contains_name_in_list(username, domain, + lp_servicename(talloc_tos(), lp_sub, snum), + token, + lp_invalid_users(snum), + &match); + if (!ok) { + return false; + } + if (match) { DEBUG(10, ("User %s in 'invalid users'\n", username)); return False; } } if (lp_valid_users(snum) != NULL) { - if (!token_contains_name_in_list(username, domain, + ok = token_contains_name_in_list(username, domain, lp_servicename(talloc_tos(), lp_sub, snum), token, - lp_valid_users(snum))) { + lp_valid_users(snum), + &match); + if (!ok) { + return false; + } + if (!match) { DEBUG(10, ("User %s not in 'valid users'\n", username)); return False; @@ -120,34 +137,48 @@ bool user_ok_token(const char *username, const char *domain, bool is_share_read_only_for_token(const char *username, const char *domain, const struct security_token *token, - connection_struct *conn) + connection_struct *conn, + bool *_read_only) { const struct loadparm_substitution *lp_sub = loadparm_s3_global_substitution(); int snum = SNUM(conn); - bool result = conn->read_only; + bool read_only = conn->read_only; + bool match; + bool ok; if (lp_read_list(snum) != NULL) { - if (token_contains_name_in_list(username, domain, - lp_servicename(talloc_tos(), lp_sub, snum), - token, - lp_read_list(snum))) { - result = True; + ok = token_contains_name_in_list(username, domain, + lp_servicename(talloc_tos(), lp_sub, snum), + token, + lp_read_list(snum), + &match); + if (!ok) { + return false; + } + if (match) { + read_only = true; } } if (lp_write_list(snum) != NULL) { - if (token_contains_name_in_list(username, domain, - lp_servicename(talloc_tos(), lp_sub, snum), - token, - lp_write_list(snum))) { - result = False; + ok = token_contains_name_in_list(username, domain, + lp_servicename(talloc_tos(), lp_sub, snum), + token, + lp_write_list(snum), + &match); + if (!ok) { + return false; + } + if (match) { + read_only = false; } } DEBUG(10,("is_share_read_only_for_user: share %s is %s for unix user " "%s\n", lp_servicename(talloc_tos(), lp_sub, snum), - result ? "read-only" : "read-write", username)); + read_only ? "read-only" : "read-write", username)); - return result; + *_read_only = read_only; + return true; } diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c index 78ad8d6e7c2..747e0a5d3be 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -133,6 +133,7 @@ NTSTATUS check_user_share_access(connection_struct *conn, int snum = SNUM(conn); uint32_t share_access = 0; bool readonly_share = false; + bool ok; if (!user_ok_token(session_info->unix_info->unix_name, session_info->info->domain_name, @@ -140,11 +141,15 @@ NTSTATUS check_user_share_access(connection_struct *conn, return NT_STATUS_ACCESS_DENIED; } - readonly_share = is_share_read_only_for_token( + ok = is_share_read_only_for_token( session_info->unix_info->unix_name, session_info->info->domain_name, session_info->security_token, - conn); + conn, + &readonly_share); + if (!ok) { + return NT_STATUS_ACCESS_DENIED; + } share_access = create_share_access_mask(snum, readonly_share, @@ -194,6 +199,7 @@ static bool check_user_ok(connection_struct *conn, struct vuid_cache_entry *ent = NULL; uint32_t share_access = 0; NTSTATUS status; + bool ok; for (i=0; ivuid_cache->array[i]; @@ -224,10 +230,17 @@ static bool check_user_ok(connection_struct *conn, return false; } - admin_user = token_contains_name_in_list( + ok = token_contains_name_in_list( session_info->unix_info->unix_name, session_info->info->domain_name, - NULL, session_info->security_token, lp_admin_users(snum)); + NULL, + session_info->security_token, + lp_admin_users(snum), + &admin_user); + if (!ok) { + /* Log, but move on */ + DBG_ERR("Couldn't apply 'admin users'\n"); + } ent = &conn->vuid_cache->array[conn->vuid_cache->next_entry]; -- 2.47.3