ksmbd-tools discussion with Atte #281
Replies: 19 comments 39 replies
-
|
Continuing the discussion we had at #278 (comment), regarding Edit: These patches are out-of-date. New patches with the same approach were sent to the mailing list for review. [PATCH] ksmbd: validate share name from share config response (click the arrow to expand)From 21cb4d1c94baab0e2c87d90490ee38a4445aa074 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Mon, 26 Sep 2022 21:50:40 +0300
Subject: [PATCH] ksmbd: validate share name from share config response
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Share config response may contain the share name without casefolding as
it is known to the user space daemon. When it is present, compare it to
the share name the share config request was made with. If they differ,
casefold it and compare again. If they still differ, we have a share
config which is incompatible with the way we do share config caching.
Currently, this is the case if CONFIG_UNICODE is not set, the share
name contains non-ASCII characters, and those non-ASCII characters do
not match those in the share name known to user space. In other words,
when CONFIG_UNICODE is not set, UTF-8 share names now work but are only
case-insensitive in the ASCII range.
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
ksmbd_netlink.h | 3 ++-
mgmt/share_config.c | 20 +++++++++++++++++---
mgmt/share_config.h | 4 +++-
mgmt/tree_connect.c | 4 ++--
misc.c | 4 ++--
misc.h | 1 +
6 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/ksmbd_netlink.h b/ksmbd_netlink.h
index 1dcba2e..2db5b9c 100644
--- a/ksmbd_netlink.h
+++ b/ksmbd_netlink.h
@@ -163,7 +163,8 @@ struct ksmbd_share_config_response {
__u16 force_directory_mode;
__u16 force_uid;
__u16 force_gid;
- __u32 reserved[128]; /* Reserved room */
+ __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME];
+ __u32 reserved[112]; /* Reserved room */
__u32 veto_list_sz;
__s8 ____payload[];
};
diff --git a/mgmt/share_config.c b/mgmt/share_config.c
index 5d03970..ac76676 100644
--- a/mgmt/share_config.c
+++ b/mgmt/share_config.c
@@ -16,6 +16,7 @@
#include "user_config.h"
#include "user_session.h"
#include "../transport_ipc.h"
+#include "../misc.h"
#define SHARE_HASH_BITS 3
static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS);
@@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config *share,
return 0;
}
-static struct ksmbd_share_config *share_config_request(const char *name)
+static struct ksmbd_share_config *share_config_request(struct unicode_map *um,
+ const char *name)
{
struct ksmbd_share_config_response *resp;
struct ksmbd_share_config *share = NULL;
@@ -133,6 +135,17 @@ static struct ksmbd_share_config *share_config_request(const char *name)
if (resp->flags == KSMBD_SHARE_FLAG_INVALID)
goto out;
+ if (*resp->share_name && strcmp(resp->share_name, name)) {
+ char *cf_resp_name;
+ bool equal;
+
+ cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name);
+ equal = cf_resp_name && !strcmp(cf_resp_name, name);
+ kfree(cf_resp_name);
+ if (!equal)
+ goto out;
+ }
+
share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL);
if (!share)
goto out;
@@ -190,7 +203,8 @@ out:
return share;
}
-struct ksmbd_share_config *ksmbd_share_config_get(const char *name)
+struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um,
+ const char *name)
{
struct ksmbd_share_config *share;
@@ -202,7 +216,7 @@ struct ksmbd_share_config *ksmbd_share_config_get(const char *name)
if (share)
return share;
- return share_config_request(name);
+ return share_config_request(um, name);
}
bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
diff --git a/mgmt/share_config.h b/mgmt/share_config.h
index 7f7e89e..3fd3382 100644
--- a/mgmt/share_config.h
+++ b/mgmt/share_config.h
@@ -9,6 +9,7 @@
#include <linux/workqueue.h>
#include <linux/hashtable.h>
#include <linux/path.h>
+#include <linux/unicode.h>
struct ksmbd_share_config {
char *name;
@@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct ksmbd_share_config *share)
__ksmbd_share_config_put(share);
}
-struct ksmbd_share_config *ksmbd_share_config_get(const char *name);
+struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map *um,
+ const char *name);
bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
const char *filename);
#endif /* __SHARE_CONFIG_MANAGEMENT_H__ */
diff --git a/mgmt/tree_connect.c b/mgmt/tree_connect.c
index 08711de..ba59a45 100644
--- a/mgmt/tree_connect.c
+++ b/mgmt/tree_connect.c
@@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess,
struct sockaddr *peer_addr;
int ret;
- sc = ksmbd_share_config_get(share_name);
+ sc = ksmbd_share_config_get(conn->um, share_name);
if (!sc) {
pr_err("Failed to get share config\n");
return status;
@@ -69,7 +69,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn, struct ksmbd_session *sess,
struct ksmbd_share_config *new_sc;
ksmbd_share_config_del(sc);
- new_sc = ksmbd_share_config_get(share_name);
+ new_sc = ksmbd_share_config_get(conn->um, share_name);
if (!new_sc) {
pr_err("Failed to update stale share config\n");
status.ret = -ESTALE;
diff --git a/misc.c b/misc.c
index 360d13d..9a9352e 100644
--- a/misc.c
+++ b/misc.c
@@ -244,7 +244,7 @@ void ksmbd_conv_path_to_windows(char *path)
strreplace(path, '/', '\\');
}
-static char *casefold_sharename(struct unicode_map *um, const char *name)
+char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name)
{
char *cf_name;
int cf_len;
@@ -290,7 +290,7 @@ char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename)
name = (pos + 1);
/* caller has to free the memory */
- return casefold_sharename(um, name);
+ return ksmbd_casefold_sharename(um, name);
}
/**
diff --git a/misc.h b/misc.h
index 884a3ce..221ce29 100644
--- a/misc.h
+++ b/misc.h
@@ -21,6 +21,7 @@ int get_nlink(struct kstat *st);
void ksmbd_conv_path_to_unix(char *path);
void ksmbd_strip_last_slash(char *path);
void ksmbd_conv_path_to_windows(char *path);
+char *ksmbd_casefold_sharename(struct unicode_map *um, const char *name);
char *ksmbd_extract_sharename(struct unicode_map *um, const char *treename);
char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
--
2.37.3
[PATCH] ksmbd-tools: preserve share name case in hash tables (click the arrow to expand)From e7c5276249febcf79fed66ef7064656fcd427014 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Mon, 26 Sep 2022 22:18:13 +0300
Subject: [PATCH] ksmbd-tools: preserve share name case in hash tables
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Preserve share name case in hash tables by doing casefolding at
lookup-time. This is preferrable for a few reasons. First, ksmbd can be
build such that it's not capable of casefolding UTF-8 share names. Such
share names are then case-sensitive if they have non-ASCII characters,
and connections to them should succeed only when matching the name in
ksmbd.conf. Second, addshare should ideally preserve formatting when
modifying ksmbd.conf. Then, preserving the share name case for format
reasons is desirable. Also, since ksmbd.conf is just as often edited
with a text editor, it is important that share names can be searched
using it, which is often not possible if they are casefolded. Finally,
preserving the share name case allows for sending it to ksmbd in the
share config response, allowing ksmbd to casefold it and validate it
against the share name it knows.
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
addshare/addshare.c | 6 ++---
addshare/share_admin.c | 8 ++++---
include/linux/ksmbd_server.h | 3 ++-
include/management/share.h | 3 ++-
lib/config_parser.c | 5 ++--
lib/management/share.c | 46 ++++++++++++++++++++++++++++--------
mountd/rpc_srvsvc.c | 5 +---
7 files changed, 52 insertions(+), 24 deletions(-)
diff --git a/addshare/addshare.c b/addshare/addshare.c
index e91fe5c..80aef55 100644
--- a/addshare/addshare.c
+++ b/addshare/addshare.c
@@ -124,17 +124,17 @@ int main(int argc, char *argv[])
switch (c) {
case 'a':
g_free(share);
- share = shm_casefold_share_name(optarg, strlen(optarg));
+ share = g_strdup(optarg);
command = command_add_share;
break;
case 'd':
g_free(share);
- share = shm_casefold_share_name(optarg, strlen(optarg));
+ share = g_strdup(optarg);
command = command_del_share;
break;
case 'u':
g_free(share);
- share = shm_casefold_share_name(optarg, strlen(optarg));
+ share = g_strdup(optarg);
command = command_update_share;
break;
case 'o':
diff --git a/addshare/share_admin.c b/addshare/share_admin.c
index c637c61..11b0f14 100644
--- a/addshare/share_admin.c
+++ b/addshare/share_admin.c
@@ -113,8 +113,8 @@ static void write_remove_share_cb(gpointer key,
{
struct smbconf_group *g = (struct smbconf_group *)value;
- if (!g_ascii_strcasecmp(g->name, name)) {
- pr_info("Share `%s' removed\n", g->name);
+ if (shm_share_name_equal(g->name, name)) {
+ pr_info("Share `%s' removed\n", name);
return;
}
@@ -187,6 +187,9 @@ int command_update_share(char *smbconf, char *name, char *opts)
goto error;
}
+ g_free(existing_group->name);
+ existing_group->name = g_strdup(name);
+
g_hash_table_foreach(update_group->kv,
update_share_cb,
existing_group->kv);
@@ -199,7 +202,6 @@ int command_update_share(char *smbconf, char *name, char *opts)
close(conf_fd);
g_free(aux_name);
return 0;
-
error:
g_free(aux_name);
return -EINVAL;
diff --git a/include/linux/ksmbd_server.h b/include/linux/ksmbd_server.h
index 713193d..643e2cd 100644
--- a/include/linux/ksmbd_server.h
+++ b/include/linux/ksmbd_server.h
@@ -91,7 +91,8 @@ struct ksmbd_share_config_response {
__u16 force_directory_mode;
__u16 force_uid;
__u16 force_gid;
- __u32 reserved[128]; /* Reserved room */
+ __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME];
+ __u32 reserved[112]; /* Reserved room */
__u32 veto_list_sz;
__s8 ____payload[];
};
diff --git a/include/management/share.h b/include/management/share.h
index bb3952c..d6ed0a6 100644
--- a/include/management/share.h
+++ b/include/management/share.h
@@ -141,7 +141,6 @@ static inline int test_share_flag(struct ksmbd_share *share, int flag)
struct ksmbd_share *get_ksmbd_share(struct ksmbd_share *share);
void put_ksmbd_share(struct ksmbd_share *share);
-char *shm_casefold_share_name(char *name, size_t len);
struct ksmbd_share *shm_lookup_share(char *name);
struct smbconf_group;
@@ -150,6 +149,8 @@ int shm_add_new_share(struct smbconf_group *group);
void shm_remove_all_shares(void);
void shm_destroy(void);
+guint shm_share_name_hash(gconstpointer name);
+gboolean shm_share_name_equal(gconstpointer lname, gconstpointer rname);
int shm_init(void);
int shm_lookup_users_map(struct ksmbd_share *share,
diff --git a/lib/config_parser.c b/lib/config_parser.c
index a1dc85c..53b2e03 100644
--- a/lib/config_parser.c
+++ b/lib/config_parser.c
@@ -93,7 +93,7 @@ static int add_new_group(char *line)
while (*end && *end != ']')
end = g_utf8_find_next_char(end, NULL);
- name = shm_casefold_share_name(begin + 1, end - begin - 1);
+ name = g_strndup(begin + 1, end - begin - 1);
if (!name)
goto out_free;
@@ -261,7 +261,8 @@ static int init_smbconf_parser(void)
if (parser.groups)
return 0;
- parser.groups = g_hash_table_new(g_str_hash, g_str_equal);
+ parser.groups = g_hash_table_new(shm_share_name_hash,
+ shm_share_name_equal);
if (!parser.groups)
return -ENOMEM;
return 0;
diff --git a/lib/management/share.c b/lib/management/share.c
index 02e45df..55a3a47 100644
--- a/lib/management/share.c
+++ b/lib/management/share.c
@@ -227,16 +227,7 @@ void shm_destroy(void)
g_rw_lock_clear(&shares_table_lock);
}
-int shm_init(void)
-{
- shares_table = g_hash_table_new(g_str_hash, g_str_equal);
- if (!shares_table)
- return -ENOMEM;
- g_rw_lock_init(&shares_table_lock);
- return 0;
-}
-
-char *shm_casefold_share_name(char *name, size_t len)
+static char *shm_casefold_share_name(const char *name, size_t len)
{
char *nfdi_name, *nfdicf_name;
@@ -255,6 +246,40 @@ out_ascii:
return g_ascii_strdown(name, len);
}
+guint shm_share_name_hash(gconstpointer name)
+{
+ char *cf_name;
+ guint hash;
+
+ cf_name = shm_casefold_share_name(name, strlen(name));
+ hash = g_str_hash(cf_name);
+ g_free(cf_name);
+ return hash;
+}
+
+gboolean shm_share_name_equal(gconstpointer lname, gconstpointer rname)
+{
+ char *cf_lname, *cf_rname;
+ gboolean equal;
+
+ cf_lname = shm_casefold_share_name(lname, strlen(lname));
+ cf_rname = shm_casefold_share_name(rname, strlen(rname));
+ equal = cf_lname && cf_rname && g_str_equal(cf_lname, cf_rname);
+ g_free(cf_lname);
+ g_free(cf_rname);
+ return equal;
+}
+
+int shm_init(void)
+{
+ shares_table = g_hash_table_new(shm_share_name_hash,
+ shm_share_name_equal);
+ if (!shares_table)
+ return -ENOMEM;
+ g_rw_lock_init(&shares_table_lock);
+ return 0;
+}
+
static struct ksmbd_share *__shm_lookup_share(char *name)
{
return g_hash_table_lookup(shares_table, name);
@@ -818,6 +843,7 @@ int shm_handle_share_config_request(struct ksmbd_share *share,
resp->force_directory_mode = share->force_directory_mode;
resp->force_uid = share->force_uid;
resp->force_gid = share->force_gid;
+ strcpy(resp->share_name, share->name);
resp->veto_list_sz = share->veto_list_sz;
if (test_share_flag(share, KSMBD_SHARE_FLAG_PIPE))
diff --git a/mountd/rpc_srvsvc.c b/mountd/rpc_srvsvc.c
index 8716c34..46c01d9 100644
--- a/mountd/rpc_srvsvc.c
+++ b/mountd/rpc_srvsvc.c
@@ -169,11 +169,8 @@ static int srvsvc_share_get_info_invoke(struct ksmbd_rpc_pipe *pipe,
{
struct ksmbd_share *share;
int ret;
- gchar *share_name;
- share_name = shm_casefold_share_name(STR_VAL(hdr->share_name),
- strlen(STR_VAL(hdr->share_name)));
- share = shm_lookup_share(share_name);
+ share = shm_lookup_share(STR_VAL(hdr->share_name));
if (!share)
return 0;
--
2.37.3
|
Beta Was this translation helpful? Give feedback.
-
|
@atheik Sorry for late checking your patch. I will check them tonight! Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
I changed the commit message of |
Beta Was this translation helpful? Give feedback.
-
|
Okay. I will replace it with new one. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
I sent a v3 patch of |
Beta Was this translation helpful? Give feedback.
-
|
A v4 patch was sent. |
Beta Was this translation helpful? Give feedback.
-
|
Okay, I will check it:) Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
Regarding |
Beta Was this translation helpful? Give feedback.
-
|
@atheik FIXED. Thanks for help! |
Beta Was this translation helpful? Give feedback.
-
|
@atheik if there is no urgent fixes, I will release ksmbd/ksmbd-tools today. |
Beta Was this translation helpful? Give feedback.
-
|
There is this to reduce the overall size of ksmbd-tools: atheik@7e4d340 |
Beta Was this translation helpful? Give feedback.
-
|
This leak should also be fixed: 7232230#r86215987 |
Beta Was this translation helpful? Give feedback.
-
|
@atheik Okay, Let them be included in version 3.4.7. |
Beta Was this translation helpful? Give feedback.
-
|
Okay, congratulations on the new release! Thank you very much for your patience in answering my questions and reviewing my PRs. |
Beta Was this translation helpful? Give feedback.
-
|
@atheik Many improvements have been made in this release thanks to your contributions. Thank you very much! |
Beta Was this translation helpful? Give feedback.
-
|
Commit c197c9b broke recreation of the ipc$ group on config reload, e.g. smbclient --user=MyUserB --password= --list //127.0.0.1/MYSHARE
#
# Sharename Type Comment
# --------- ---- -------
# IPC$ IPC IPC share
# MyShare Disk
# SMB1 disabled -- no workgroup available
ksmbd.control --reload
# [ksmbd.control/110684]: INFO: Notified ksmbd.mountd of changes
smbclient --user=MyUserB --password= --list //127.0.0.1/MYSHARE
# tree connect failed: NT_STATUS_BAD_NETWORK_NAMEThis patch was made against commit 8b2649f. [PATCH] ksmbd-tools: fix recreation of ipc$ group on config reload (click the arrow to expand)From dafd2c5a4e29bf5c64ed4eb094adfabf672f626f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Wed, 19 Oct 2022 18:53:27 +0300
Subject: [PATCH] ksmbd-tools: fix recreation of ipc$ group on config reload
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit c197c9b broke recreation of the ipc$ group on config reload.
Since the ipc$ share is killed on config reload, we must do a new
group lookup every time cp_add_ipc_group() is called, so that a new
ipc$ group is created when necessary.
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
lib/config_parser.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/config_parser.c b/lib/config_parser.c
index 53b2e03..a704fdc 100644
--- a/lib/config_parser.c
+++ b/lib/config_parser.c
@@ -639,6 +639,7 @@ static int cp_add_ipc_group(void)
char *comment = NULL, *guest = NULL;
int ret = 0;
+ ipc_group = g_hash_table_lookup(parser.groups, "ipc$");
if (ipc_group)
return ret;
--
2.38.0
|
Beta Was this translation helpful? Give feedback.
-
|
No problem, BTW, I am thinking 3.4.7 version release included the following patches tonight. 27f4774 (HEAD -> master, origin/master, origin/HEAD) ksmbd-tools: fix recreation of ipc$ group on config reload |
Beta Was this translation helpful? Give feedback.
-
Okay. There is a similar bug for recreating the default global guest account ( |
Beta Was this translation helpful? Give feedback.
-
To clarify, this bug is present since the 3.4.4 release at least. It could be much older, but 3.4.4 was the oldest release I tested. |
Beta Was this translation helpful? Give feedback.
-
Yes.
Okay. Thanks for your check! |
Beta Was this translation helpful? Give feedback.
-
|
Here's a reproducer for the bug (assuming smbclient --user=nobody --password= --list //127.0.0.1
#
# Sharename Type Comment
# --------- ---- -------
# IPC$ IPC IPC share
# MyShare Disk
# SMB1 disabled -- no workgroup available
ksmbd.control --reload
# [ksmbd.control/95520]: INFO: Notified ksmbd.mountd of changes
smbclient --user=nobody --password= --list //127.0.0.1
# session setup failed: NT_STATUS_LOGON_FAILUREAnd here's a patch. [PATCH] ksmbd-tools: recreate default global guest account on config reload (click the arrow to expand)From 4680e7d2e7a9be8165fd09cb885902debb691628 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Fri, 21 Oct 2022 15:14:55 +0300
Subject: [PATCH] ksmbd-tools: recreate default global guest account on config
reload
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The default global guest account is created if no global guest account
was specified in the config file. However, unlike the user-specified
global guest account, the default one is created only on initial
startup. This is a problem since the global guest account is killed on
config reload and should then be recreated. When determining whether to
create the default global guest account, check whether the global guest
account exists by doing a user lookup with its name, rather than just
checking that the name is non-NULL. Also, change some inconsistent and
redundant logging messages for failing usm_add_new_user() calls.
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
adduser/user_admin.c | 2 +-
tools/config_parser.c | 10 +++++++---
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/adduser/user_admin.c b/adduser/user_admin.c
index 4b2dab0..564db49 100644
--- a/adduser/user_admin.c
+++ b/adduser/user_admin.c
@@ -341,7 +341,7 @@ int command_add_user(char *pwddb, char *account, char *password)
/* pswd is already g_strdup-ed */
if (usm_add_new_user(account, pswd)) {
- pr_err("Could not add new user `%s'\n", account);
+ pr_err("Unable to add new user `%s'\n", account);
return -EINVAL;
}
diff --git a/tools/config_parser.c b/tools/config_parser.c
index 2cecc46..5a87c90 100644
--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -365,7 +365,7 @@ static int cp_add_global_guest_account(gpointer _v)
if (usm_add_new_user(cp_get_group_kv_string(_v),
g_strdup("NULL"))) {
- pr_err("Unable to add guest account\n");
+ pr_err("Unable to add new user `%s'\n", (char *) _v);
return -ENOMEM;
}
@@ -593,6 +593,8 @@ static void global_conf_update(struct smbconf_group *group)
static void global_conf_fixup_missing(void)
{
+ struct ksmbd_user *user;
+
/*
* Set default global parameters which were not specified
* in smb.conf
@@ -615,8 +617,10 @@ static void global_conf_fixup_missing(void)
if (global_conf.sessions_cap <= 0)
global_conf.sessions_cap = KSMBD_CONF_DEFAULT_SESS_CAP;
- if (!global_conf.guest_account &&
- cp_add_global_guest_account(KSMBD_CONF_DEFAULT_GUEST_ACCOUNT))
+ user = usm_lookup_user(global_conf.guest_account);
+ if (user)
+ put_ksmbd_user(user);
+ else if (cp_add_global_guest_account(KSMBD_CONF_DEFAULT_GUEST_ACCOUNT))
pr_err("Unable to add guest account\n");
}
--
2.38.0
|
Beta Was this translation helpful? Give feedback.
-
|
Regarding the |
Beta Was this translation helpful? Give feedback.
-
|
Hm.. I have no idea for this. I think that we can refer samba behavior, How do they handle it... When I checked smb.conf of samba, There is no guest account(S). |
Beta Was this translation helpful? Give feedback.
-
|
@atheik There is a problem that data encryption will always enable if we are setting smb3 encryption = yes. and if it is no, it will be completely disable the encryption. but problem is that client can not handle it with mount option(e.g. seal). So I rename parameter to server smb encrypt with 3 option(off, desired, required). Could you please review this ? |
Beta Was this translation helpful? Give feedback.
-
|
Sure, I will take a look. |
Beta Was this translation helpful? Give feedback.
-
|
Both commits look good to me. I tested with cifs-utils and smbclient, although I cannot configure either such that they truly turn off support for encryption for testing with Is there a reason for the rename? I think Samba uses the I would like to suggest using Changes if you would like to consider them (click the arrow to expand)diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index fe4174c..5cf01fd 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -280,11 +280,11 @@ Maximum length that may be used in a SMB2 WRITE request sent by a client.
Default: \fBsmb2 max write = 4MB\fR \" SMB3_DEFAULT_IOSIZE
.TP
-\fBserver smb encrypt\fR (G)
-A remote client is allowed or required to use SMB encryption.
-Setting it to \fBoff\fR globally will completely disable the encryption feature for all connections.
-Setting it to \fBdesired\fR on a share will turn on data encryption for this share for clients that support encryption.
-Setting it to \fBrequired\fR on a share will enforce data encryption for this share. i.e. clients that do not support encryption will be denied access to the share.
+\fBsmb3 encryption\fR (G)
+Client is disallowed, allowed, or required to use SMB3 encryption.
+With \fBsmb3 encryption = disabled\fR, SMB3 encryption is disallowed even if it is requested by the client.
+With \fBsmb3 encryption = auto\fR, SMB3 encryption is allowed if it is requested by the client.
+With \fBsmb3 encryption = mandatory\fR, SMB3 encryption is required.
Default: \fBserver smb encrypt = desired\fR
.TP
diff --git a/tools/config_parser.c b/tools/config_parser.c
index 9b731e3..b643e4d 100644
--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -509,16 +509,20 @@ static gboolean global_group_kv(gpointer _k, gpointer _v, gpointer user_data)
return TRUE;
}
- if (!cp_key_cmp(_k, "server smb encrypt")) {
- if (!cp_key_cmp(_v, "required")) {
- global_conf.flags |= KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION;
- global_conf.flags &= ~KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION_OFF;
- } else if (!cp_key_cmp(_v, "off")) {
+ if (!cp_key_cmp(_k, "smb3 encryption")) {
+ switch (cp_get_group_kv_config_opt(_v)) {
+ case KSMBD_CONFIG_OPT_DISABLED:
global_conf.flags |= KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION_OFF;
global_conf.flags &= ~KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION;
- } else if (!cp_key_cmp(_v, "desired")) {
+ break;
+ case KSMBD_CONFIG_OPT_MANDATORY:
+ global_conf.flags |= KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION;
+ global_conf.flags &= ~KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION_OFF;
+ break;
+ default:
global_conf.flags &= ~KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION;
global_conf.flags &= ~KSMBD_GLOBAL_FLAG_SMB3_ENCRYPTION_OFF;
+ break;
}
return TRUE;
This is unrelated, but for With \fBserver signing = disabled\fR or \fBserver signing = auto\fR, SMB2 signing is allowed if it is required by the client.Is With \fBserver signing = disabled\fR or \fBserver signing = auto\fR, SMB2 signing is allowed if it is requested by the client. |
Beta Was this translation helpful? Give feedback.
-
Agreed, Okay, I will update it:) |
Beta Was this translation helpful? Give feedback.
-
Ah, I think that there is no big difference between required or requested here. Because there is an expression "by client". |
Beta Was this translation helpful? Give feedback.
-
|
Here is another cat <<'EOF' >global_guest_account_bug.conf
[global]
guest account = MyUserC
[MyShare]
path = /home/atte/MyShare
guest account = MyUserD
guest ok = yes
EOF
sudo sh -c "ksmbd.mountd -v -n2 -c global_guest_account_bug.conf -u '' &"
# [ksmbd-worker/32920]: DEBUG: Can't open `': No such file or directory
# [ksmbd-worker/32920]: INFO: User database does not exist, only guest sessions may work
# [ksmbd-worker/32920]: DEBUG: New user `MyUserC'
# [ksmbd-worker/32920]: DEBUG: New share `IPC$'
# [ksmbd-worker/32920]: DEBUG: New user `MyUserD'
# [ksmbd-worker/32920]: DEBUG: New share `MyShare'
sudo mount -o user=MyUserC,password='' //127.0.0.1/MyShare /mnt
# [ksmbd-worker/32920]: DEBUG: treecon: Net share permits guest login // IPC$
# [ksmbd-worker/32920]: DEBUG: treecon: Net share permits guest login // MyShare
sudo umount /mnt
# [ksmbd-worker/32920]: DEBUG: Kill user `MyUserC'
sudo mount -o user=MyUserC,password='' //127.0.0.1/MyShare /mnt
# mount error(13): Permission denied
# Refer to the mount.cifs(8) manual page (e.g. man mount.cifs) and kernel log messages (dmesg)
sudo mount -o user=MyUserD,password='' //127.0.0.1/MyShare /mnt
# [ksmbd-worker/32920]: DEBUG: treecon: Net share permits guest login
# [ksmbd-worker/32920]: ERROR: treecon: User `' not found
# [ksmbd-worker/32920]: DEBUG: treecon: Net share permits guest login
sudo umount /mntThe global guest account |
Beta Was this translation helpful? Give feedback.
-
Well, considering this statement, I think the changes in the patch below are the way to fix this. Alternatively, there may be a deeper underlying issue for guest account refcounts. [PATCH] ksmbd-tools: lookup both guest accounts on tree connect (click the arrow to expand)From c7b5ee886bfbc59293ce990a01467d63dc117749 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Sat, 29 Oct 2022 03:15:48 +0300
Subject: [PATCH] ksmbd-tools: lookup both guest accounts on tree connect
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When both global guest account and share guest account exist, the ref
count of the former is decremented such that it is killed on guest
session disconnect. This is because a lookup is done only for the
latter when determining whether to give guest access on tree connect.
Fix this by always doing a lookup for both on such tree connects.
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
tools/management/tree_conn.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/tools/management/tree_conn.c b/tools/management/tree_conn.c
index 417b8b4..c344701 100644
--- a/tools/management/tree_conn.c
+++ b/tools/management/tree_conn.c
@@ -126,16 +126,19 @@ int tcm_handle_tree_connect(struct ksmbd_tree_connect_request *req,
if ((req->account_flags & KSMBD_USER_FLAG_GUEST_ACCOUNT) &&
test_share_flag(share, KSMBD_SHARE_FLAG_GUEST_OK)) {
+ struct ksmbd_user *guest_account, *global_guest_account;
+
pr_debug("treecon: Net share permits guest login\n");
- user = usm_lookup_user(share->guest_account);
- if (user) {
+ guest_account = usm_lookup_user(share->guest_account);
+ global_guest_account = usm_lookup_user(global_conf.guest_account);
+ if (guest_account) {
set_conn_flag(conn, KSMBD_TREE_CONN_FLAG_GUEST_ACCOUNT);
+ user = guest_account;
goto bind;
}
-
- user = usm_lookup_user(global_conf.guest_account);
- if (user) {
+ if (global_guest_account) {
set_conn_flag(conn, KSMBD_TREE_CONN_FLAG_GUEST_ACCOUNT);
+ user = global_guest_account;
goto bind;
}
}
--
2.38.1
|
Beta Was this translation helpful? Give feedback.
-
|
Looks good to me. I will apply this patch. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
Instead of commit 12cfe70, this seems to be sufficient: diff --git a/tools/management/session.c b/tools/management/session.c
index b3f82c5..fb465fe 100644
--- a/tools/management/session.c
+++ b/tools/management/session.c
@@ -13,6 +13,7 @@
#include "management/session.h"
#include "management/tree_conn.h"
#include "management/user.h"
+#include "management/share.h"
#include "tools.h"
static GHashTable *sessions_table;
@@ -150,6 +151,14 @@ retry:
goto retry;
}
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_connect: tree_conn->share->name: %s\n", tree_conn->share->name);
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_connect: user->name: %s\n", user->name);
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_connect: sess->user->name: %s\n", sess->user->name);
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_connect: sess->user->ref_count: %d\n", sess->user->ref_count);
+
+ if (user != sess->user)
+ get_ksmbd_user(sess->user);
+
g_rw_lock_writer_lock(&sess->update_lock);
sess->tree_conns = g_list_insert(sess->tree_conns, tree_conn, -1);
g_rw_lock_writer_unlock(&sess->update_lock);
@@ -207,10 +216,14 @@ int sm_handle_tree_disconnect(unsigned long long sess_id,
tree_conn = (struct ksmbd_tree_conn *)tc_list->data;
sess->tree_conns = g_list_remove(sess->tree_conns, tree_conn);
sess->ref_counter--;
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_disconnect: tree_conn->share->name: %s\n", tree_conn->share->name);
tcm_tree_conn_free(tree_conn);
}
g_rw_lock_writer_unlock(&sess->update_lock);
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->name: %s\n", sess->user->name);
+ fprintf(stderr, "GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->ref_count: %d\n", sess->user->ref_count);
+
put_ksmbd_user(sess->user);
__put_session(sess);
return 0;
Applying this and running the reproducer at #281 (comment): sudo sh -c "ksmbd.mountd -v -n2 -c global_guest_account_bug.conf -u '' &"
# [ksmbd-worker/49678]: DEBUG: Can't open `': No such file or directory
# [ksmbd-worker/49678]: INFO: User database does not exist, only guest sessions may work
# [ksmbd-worker/49678]: DEBUG: New user `MyUserC'
# [ksmbd-worker/49678]: DEBUG: New share `IPC$'
# [ksmbd-worker/49678]: DEBUG: New user `MyUserD'
# [ksmbd-worker/49678]: DEBUG: New share `MyShare'
sudo mount -o user=MyUserC,password='' //127.0.0.1/MyShare /mnt
# [ksmbd-worker/49678]: DEBUG: treecon: Net share permits guest login
# GUEST_REF_BUG: sm_handle_tree_connect: tree_conn->share->name: IPC$
# GUEST_REF_BUG: sm_handle_tree_connect: user->name: MyUserC
# GUEST_REF_BUG: sm_handle_tree_connect: sess->user->name: MyUserC
# GUEST_REF_BUG: sm_handle_tree_connect: sess->user->ref_count: 2
# [ksmbd-worker/49678]: DEBUG: treecon: Net share permits guest login
# GUEST_REF_BUG: sm_handle_tree_connect: tree_conn->share->name: MyShare
# GUEST_REF_BUG: sm_handle_tree_connect: user->name: MyUserD <--
# GUEST_REF_BUG: sm_handle_tree_connect: sess->user->name: MyUserC <--
# GUEST_REF_BUG: sm_handle_tree_connect: sess->user->ref_count: 2 <-- will be incremented
sudo umount /mnt
# GUEST_REF_BUG: sm_handle_tree_disconnect: tree_conn->share->name: MyShare
# GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->name: MyUserC
# GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->ref_count: 3
# GUEST_REF_BUG: sm_handle_tree_disconnect: tree_conn->share->name: IPC$
# GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->name: MyUserC
# GUEST_REF_BUG: sm_handle_tree_disconnect: sess->user->ref_count: 2
Is this approach preferable? Or, do you see from here what is the underlying issue causing the bug? |
Beta Was this translation helpful? Give feedback.
-
|
@atheik |
Beta Was this translation helpful? Give feedback.
-
|
When I wrote the man page descriptions for This change to out-of-tree ksmbd makes the parameters behave as in Samba: diff --git a/mgmt/share_config.h b/mgmt/share_config.h
index 3fd3382..8a19e54 100644
--- a/mgmt/share_config.h
+++ b/mgmt/share_config.h
@@ -43,7 +43,7 @@ static inline int share_config_create_mode(struct ksmbd_share_config *share,
else
return posix_mode & share->create_mask;
}
- return share->force_create_mode & share->create_mask;
+ return share->force_create_mode | share->create_mask;
}
static inline int share_config_directory_mode(struct ksmbd_share_config *share,
@@ -56,7 +56,7 @@ static inline int share_config_directory_mode(struct ksmbd_share_config *share,
return posix_mode & share->directory_mask;
}
- return share->force_directory_mode & share->directory_mask;
+ return share->force_directory_mode | share->directory_mask;
}
static inline int test_share_config_flag(struct ksmbd_share_config *share,
|
Beta Was this translation helpful? Give feedback.
-
|
directory_mask is mask, So it should be bitwise ANDed. and force_directory_mode should be bitwise OR. Creates a file with the current create_mask value as the mode bit. That's a mask... If so, should it be bitwise ANDed with create_mask by getting a mode bit from the variable of the SMB2 Create request? |
Beta Was this translation helpful? Give feedback.
-
Right, so the current ksmbd behavior with
Sorry, I don't understand what you mean. Surely
Sorry, what should be bitwise and'd with From here on assuming the parameters should work as in Samba and as described in ksmbd.conf(5). With POSIX SMB3.11 extensions, i.e. when If diff --git a/mgmt/share_config.h b/mgmt/share_config.h
index 3fd3382..32bef4b 100644
--- a/mgmt/share_config.h
+++ b/mgmt/share_config.h
@@ -37,26 +37,17 @@ struct ksmbd_share_config {
static inline int share_config_create_mode(struct ksmbd_share_config *share,
umode_t posix_mode)
{
- if (!share->force_create_mode) {
- if (!posix_mode)
- return share->create_mask;
- else
- return posix_mode & share->create_mask;
- }
- return share->force_create_mode & share->create_mask;
+ if (!posix_mode)
+ return share->create_mask | share->force_create_mode;
+ return (posix_mode & share->create_mask) | share->force_create_mode;
}
static inline int share_config_directory_mode(struct ksmbd_share_config *share,
umode_t posix_mode)
{
- if (!share->force_directory_mode) {
- if (!posix_mode)
- return share->directory_mask;
- else
- return posix_mode & share->directory_mask;
- }
-
- return share->force_directory_mode & share->directory_mask;
+ if (!posix_mode)
+ return share->directory_mask | share->force_directory_mode;
+ return (posix_mode & share->directory_mask) | share->force_directory_mode;
}
static inline int test_share_config_flag(struct ksmbd_share_config *share,
|
Beta Was this translation helpful? Give feedback.
-
|
@atheik Looks good to me:) Could you please send the patch to the list ? |
Beta Was this translation helpful? Give feedback.
-
|
Oops, I have now sent it to the list. Sorry for the delay. |
Beta Was this translation helpful? Give feedback.
-
|
Here's a reproducer for a NULL dereference that is like the one fixed in commit 244725b: rpcclient -U tester -c 'queryuser 0x1234' 127.0.0.1And here's a patch: [PATCH] ksmbd-tools: fix NULL deref in samr_open_user_return() (click the arrow to expand)From 7e3cae32d7db25971f49f8e433725fc8cc7cf841 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Thu, 10 Aug 2023 02:49:14 +0300
Subject: [PATCH] ksmbd-tools: fix NULL deref in samr_open_user_return()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Accessing ch->user->uid can result in NULL deref since ch->user can be
NULL. This was the case in samr_query_security_return() also before
commit 244725b. Fix the NULL deref in samr_open_user_return().
Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
mountd/rpc_samr.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mountd/rpc_samr.c b/mountd/rpc_samr.c
index e169810..bac4ce0 100644
--- a/mountd/rpc_samr.c
+++ b/mountd/rpc_samr.c
@@ -390,6 +390,9 @@ static int samr_open_user_return(struct ksmbd_rpc_pipe *pipe)
return KSMBD_RPC_EBAD_FID;
ch->refcount++;
+ if (!ch->user)
+ return KSMBD_RPC_EBAD_FID;
+
if (dce->sm_req.rid != ch->user->uid)
return KSMBD_RPC_EBAD_FID;
--
2.41.0
By the way, how do you exercise the |
Beta Was this translation helpful? Give feedback.
-
|
samr and lsarpc should be checked again. It is just tested with windows client for windows file permission update. I'm not sure if it's implemented as well as intended. It seems that smbtorture has unit testcases for samr and lsaprc. I plan to check it later, but I don't still have the time due to other issues... Okay. I will apply this patch. And I am still waiting for force create mode and force directory mode patch. |
Beta Was this translation helpful? Give feedback.
-
|
The When setting a flag with This means that this We can work around this for diff --git a/include/management/share.h b/include/management/share.h
index 5e20799..d2419bb 100644
--- a/include/management/share.h
+++ b/include/management/share.h
@@ -10,6 +10,7 @@
#include <glib.h>
+#include "linux/ksmbd_server.h"
enum share_users {
/* Admin users */
@@ -127,7 +128,10 @@ int shm_share_config(char *k, enum KSMBD_SHARE_CONF c);
static inline void set_share_flag(struct ksmbd_share *share, int flag)
{
- share->flags |= flag;
+ if (flag == KSMBD_SHARE_FLAG_INVALID)
+ share->flags = flag;
+ else
+ share->flags |= flag;
}
static inline void clear_share_flag(struct ksmbd_share *share, int flag)
@@ -137,6 +141,8 @@ static inline void clear_share_flag(struct ksmbd_share *share, int flag)
static inline int test_share_flag(struct ksmbd_share *share, int flag)
{
+ if (flag == KSMBD_SHARE_FLAG_INVALID)
+ return share->flags == flag;
return share->flags & flag;
}
diff --git a/include/management/user.h b/include/management/user.h
index d564bc1..bbe9bcd 100644
--- a/include/management/user.h
+++ b/include/management/user.h
@@ -12,6 +12,8 @@
#include <pwd.h>
#include <glib.h>
+#include "linux/ksmbd_server.h"
+
struct ksmbd_user {
char *name;
char *pass_b64;
@@ -29,14 +31,19 @@ struct ksmbd_user {
unsigned int failed_login_count;
};
-static inline void set_user_flag(struct ksmbd_user *user, int bit)
+static inline void set_user_flag(struct ksmbd_user *user, int flag)
{
- user->flags |= bit;
+ if (flag == KSMBD_USER_FLAG_INVALID)
+ user->flags = flag;
+ else
+ user->flags |= flag;
}
-static inline int test_user_flag(struct ksmbd_user *user, int bit)
+static inline int test_user_flag(struct ksmbd_user *user, int flag)
{
- return user->flags & bit;
+ if (flag == KSMBD_USER_FLAG_INVALID)
+ return user->flags == flag;
+ return user->flags & flag;
}
struct ksmbd_user *get_ksmbd_user(struct ksmbd_user *user);
What should be done about this? |
Beta Was this translation helpful? Give feedback.
-
|
This workaround was no good as explained in commit 2895584. |
Beta Was this translation helpful? Give feedback.
-
|
Okay, Good point:). How about replace KSMBD_USER_FLAG_INVALID and KSMBD_SHARE_FLAG_INVALID with KSMBD_SHARE_FLAG_AVAILABLE and KSMBD_USER_FLAG_OK ? |
Beta Was this translation helpful? Give feedback.
-
|
Looks like most calls to for i in $(seq 100); do
rpcclient -U MyUser --password MyPassword -c 'enumdomains; enumdomains; enumdomains' //127.0.0.1 >/dev/null
doneYou can see the first leak grows per connection and the second leak grows per SAMR command. |
Beta Was this translation helpful? Give feedback.
-
|
@atheik I didn't know it was possible to test like that with samba, and I didn't even know there was a smbtorture test. Thank you for finding the problems! |
Beta Was this translation helpful? Give feedback.
-
|
Sorry, I don't understand what you mean. I have not used Samba's sudo ksmbd.control -s
sudo -E valgrind --leak-check=full ksmbd.mountd -C test.conf -P test.db -v -n &
# <insert reproducer here>
sudo ksmbd.control -s |
Beta Was this translation helpful? Give feedback.
-
|
Ah, I just wanted to say that the current implementation is not well tested. |
Beta Was this translation helpful? Give feedback.
-
|
I have written |
Beta Was this translation helpful? Give feedback.
-
|
Sound great! For now, let's just show the share name(+comment) and expand further as requests come in. And we can ask VDRU what more is needed. |
Beta Was this translation helpful? Give feedback.
-
|
Why can't I reproduce memleak dump from valgrind like you ? |
Beta Was this translation helpful? Give feedback.
-
Are you sure the Then, do |
Beta Was this translation helpful? Give feedback.
-
|
Remember that you must also run I tested your patch and it does fix the memleak. Thank you for fixing it. |
Beta Was this translation helpful? Give feedback.
-
|
@atheik Okay, I will try it:) |
Beta Was this translation helpful? Give feedback.
-
@atheik Atte, Let's discuss here for new approach.(not closed PR)
Beta Was this translation helpful? Give feedback.
All reactions