ksmbd-tools discussion with Atte #281
Replies: 25 comments 62 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?= <[email protected]>
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ä <[email protected]>
---
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?= <[email protected]>
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ä <[email protected]>
---
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?= <[email protected]>
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ä <[email protected]>
---
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?= <[email protected]>
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ä <[email protected]>
---
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.
-
|
Note that although reverting commit 0b87a71 fixed UAFs and this guest account bug, it (re)introduced a leak on config reload: |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for your check! Could you please fix this issue ? |
Beta Was this translation helpful? Give feedback.
-
|
For future reference, here's the UAF caused by commit 0b87a71: #303 (comment).
If I have a patch, I will post it. |
Beta Was this translation helpful? Give feedback.
-
|
Consider this Let Then, consider this client mount: mount -o user=global_guest,pass= //127.0.0.1/tmp /mntHere, the user resolved by Note that this client mount is equivalent to the previous one: mount -o user=tmp_guest,pass= //127.0.0.1/tmp /mntThe only difference is the login request preceding the tree connect, Normally, from what I understand from my testing, is that if the client Here's a workaround where we keep updating the session user with each [DIFF] session user leak fix (click the arrow to expand)diff --git a/tools/management/session.c b/tools/management/session.c
index ab4e749..37e5cf0 100644
--- a/tools/management/session.c
+++ b/tools/management/session.c
@@ -18,8 +18,16 @@
static GHashTable *sessions_table;
static GRWLock sessions_table_lock;
+static void __put_user(struct ksmbd_tree_conn *tree_conn,
+ struct ksmbd_user *user)
+{
+ (void)tree_conn;
+ put_ksmbd_user(user);
+}
+
static void kill_ksmbd_session(struct ksmbd_session *sess)
{
+ g_list_foreach(sess->tree_conns, (GFunc)__put_user, sess->user);
g_list_free_full(sess->tree_conns, (GDestroyNotify)tcm_tree_conn_free);
g_rw_lock_clear(&sess->update_lock);
g_free(sess);
@@ -141,6 +149,8 @@ retry:
}
g_rw_lock_writer_lock(&sess->update_lock);
+ put_ksmbd_user(sess->user);
+ sess->user = get_ksmbd_user(user);
sess->tree_conns = g_list_insert(sess->tree_conns, tree_conn, -1);
g_rw_lock_writer_unlock(&sess->update_lock);
return 0;
@@ -194,6 +204,8 @@ int sm_handle_tree_disconnect(unsigned long long sess_id,
sess->tree_conns = g_list_remove(sess->tree_conns, tree_conn);
sess->ref_counter--;
tcm_tree_conn_free(tree_conn);
+
+ put_ksmbd_user(sess->user);
}
g_rw_lock_writer_unlock(&sess->update_lock);
To me, in this scenario, it would still make the most sense to have I hope you have an idea on how to fix this properly. |
Beta Was this translation helpful? Give feedback.
-
We could avoid adding this workaround to the patch by removing |
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?= <[email protected]>
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ä <[email protected]>
---
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.
-
|
Here's a leak that grows per config reload when Kerberos 5 support is enabled: And here's a patch: [PATCH] ksmbd-tools: stop leaking krb5_sname_to_principal() (click the arrow to expand)From ed291072ecd02ba7a3966a96211b54532e52bd0c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Fri, 11 Oct 2024 23:26:54 +0300
Subject: [PATCH] ksmbd-tools: stop leaking krb5_sname_to_principal()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The success code path of acquire_creds_from_keytab() leaks the
krb5_principal allocated by krb5_sname_to_principal(). Since
krb5_get_init_creds_keytab() makes a copy of the principal with
krb5_copy_principal(), we can just call krb5_free_principal() after.
Signed-off-by: Atte Heikkilä <[email protected]>
---
tools/management/spnego_krb5.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/management/spnego_krb5.c b/tools/management/spnego_krb5.c
index 1444cac..94ccbea 100644
--- a/tools/management/spnego_krb5.c
+++ b/tools/management/spnego_krb5.c
@@ -164,6 +164,7 @@ static krb5_error_code acquire_creds_from_keytab(krb5_context context,
goto out_err;
}
+ krb5_free_principal(context, sprinc);
g_free(host_name);
g_free(service_name);
return 0;
--
2.47.0
|
Beta Was this translation helpful? Give feedback.
-
|
Applied, Thanks for your work! |
Beta Was this translation helpful? Give feedback.
-
|
This is an ancient bug that was introduced in commit a7e0e60. First, apply this to help reproduce the issue: [DIFF] bug reproducer (click the arrow to expand)diff --git a/mountd/worker.c b/mountd/worker.c
index f8876f2..2aec953 100644
--- a/mountd/worker.c
+++ b/mountd/worker.c
@@ -52,6 +52,9 @@ static int login_request(struct ksmbd_ipc_msg *msg)
resp_msg->type = KSMBD_EVENT_LOGIN_RESPONSE;
resp->handle = req->handle;
+ pr_info("YOU HAVE 10 SECONDS TO ISSUE A SHUTDOWN\n");
+ sleep(10);
+
ipc_msg_send(resp_msg);
out:
ipc_msg_free(resp_msg);
Then:
Looks like this: This bug can result in varying UAFs in And here's a patch: [PATCH] ksmbd-tools: prevent use-after-frees on remove_config() (click the arrow to expand)From 7efbb27a82fc86a0da821f0eff10a4ba76bf6d79 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Thu, 17 Oct 2024 22:03:40 +0300
Subject: [PATCH] ksmbd-tools: prevent use-after-frees on remove_config()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When the mountd worker process calls remove_config(), there may still
be worker pool tasks executing, which may then access already freed
memory. Fix this by calling wp_destroy() first in remove_config().
For consistency, change the order in load_config() as well.
This bug was introduced in commit a7e0e60.
Signed-off-by: Atte Heikkilä <[email protected]>
---
tools/tools.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/tools.c b/tools/tools.c
index 03a3d1c..01869f1 100644
--- a/tools/tools.c
+++ b/tools/tools.c
@@ -298,10 +298,10 @@ int load_config(char *pwddb, char *smbconf)
if (TOOL_IS_MOUNTD) {
sm_init();
- wp_init();
rpc_init();
ipc_init();
spnego_init();
+ wp_init();
}
return ret;
@@ -310,10 +310,10 @@ int load_config(char *pwddb, char *smbconf)
void remove_config(void)
{
if (TOOL_IS_MOUNTD) {
+ wp_destroy();
spnego_destroy();
ipc_destroy();
rpc_destroy();
- wp_destroy();
sm_destroy();
} else if (TOOL_IS_ADDSHARE) {
cp_smbconf_parser_destroy();
--
2.47.0
|
Beta Was this translation helpful? Give feedback.
-
|
@atheik Applied it! Thank you! |
Beta Was this translation helpful? Give feedback.
-
|
Here is another Kerberos 5 related patch: [PATCH] ksmbd-tools: disable krb5 context on acquire_creds_from_keytab() error (click the arrow to expand)From 10d2355b22d290fa053663623f987eb98801e8e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Fri, 18 Oct 2024 17:31:06 +0300
Subject: [PATCH] ksmbd-tools: disable krb5 context on
acquire_creds_from_keytab() error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When distros package Kerberos 5, they do not configure it at
package-installation time such that it is functional. If ksmbd-tools
is to be packaged with Kerberos 5 support enabled, we must then ignore
failure to setup Kerberos 5 authentication in this scenario.
In the scenario, acquire_creds_from_keytab() typically returns either
KRB5_ERR_HOST_REALM_UNKNOWN or KRB5_REALM_CANT_RESOLVE, the former
denoting failure to resolve a dotted FQDN (e.g. due to the distro
having localhost as the default hostname), while the latter denoting
failure to resolve KDC for the empty realm. With Heimdal, we also get
ENOENT when the keytab file does not exist.
Rather than trying to justify the exact set of errors to ignore for the
scenario, we ignore all of them, as there is no point in restarting the
worker process on any of them. Note that if the initial setup fails,
we do not reconfigure on config reload since we do not do so if the
setup succeeded either.
Signed-off-by: Atte Heikkilä <[email protected]>
---
tools/management/spnego_krb5.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/management/spnego_krb5.c b/tools/management/spnego_krb5.c
index 94ccbea..53ccbdc 100644
--- a/tools/management/spnego_krb5.c
+++ b/tools/management/spnego_krb5.c
@@ -331,7 +331,8 @@ static int setup_krb5_ctx(struct spnego_mech_ctx *mech_ctx)
struct spnego_krb5_ctx *krb5_ctx;
krb5_error_code krb_retval;
- if (mech_ctx->private)
+ if (mech_ctx->private ||
+ ksmbd_health_status & KSMBD_SHOULD_RELOAD_CONFIG)
return 0;
krb5_ctx = g_try_malloc0(sizeof(*krb5_ctx));
@@ -352,7 +353,8 @@ static int setup_krb5_ctx(struct spnego_mech_ctx *mech_ctx)
if (krb_retval) {
krb5_free_context(krb5_ctx->context);
g_free(krb5_ctx);
- return -EINVAL;
+ krb5_ctx = NULL;
+ pr_info("Disabled krb5 context\n");
}
mech_ctx->private = krb5_ctx;
--
2.47.0
If you choose to apply this patch, then please mention the following in the release notes:
|
Beta Was this translation helpful? Give feedback.
-
|
Maybe we also need something like |
Beta Was this translation helpful? Give feedback.
-
|
I think this is a more reasonable approach: [PATCH] ksmbd-tools: add global parameter `kerberos support' (click the arrow to expand)From f093c8b63748336bde9c69fcc3c2deb11e1742a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Sun, 27 Oct 2024 00:46:58 +0300
Subject: [PATCH] ksmbd-tools: add global parameter `kerberos support'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If ksmbd-tools is to be packaged with it built against Kerberos 5, we
must provide a way to disable it at startup. Add global parameter
`kerberos support' with default value `no'.
Signed-off-by: Atte Heikkilä <[email protected]>
---
include/tools.h | 1 +
ksmbd.conf.5.in | 6 ++++++
ksmbd.conf.example | 1 +
tools/config_parser.c | 4 ++++
tools/management/spnego_krb5.c | 2 +-
5 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/tools.h b/include/tools.h
index cf4aa4f..019f490 100644
--- a/include/tools.h
+++ b/include/tools.h
@@ -58,6 +58,7 @@ struct smbconf_global {
unsigned int gen_subauth[3];
char *krb5_keytab_file;
char *krb5_service_name;
+ int krb5_support;
char *pwddb;
char *smbconf;
pid_t pid;
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index 61919ba..ad32baa 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -172,6 +172,12 @@ If no value is given, it is \fBcifs/\fR followed by the FQDN resolved with \fBge
Default: \fBkerberos service name =
.TP
+\fBkerberos support (G)
+Support for Kerberos 5 authentication.
+For the parameter to take effect, \fBksmbd.mountd\fR must be built against Kerberos 5.
+
+Default: \fBkerberos support = no
+.TP
\fBmap to guest\fR (G)
When to map a user to the user given with \fBguest account\fR.
With \fBmap to guest = bad user\fR, map when the user does not exist.
diff --git a/ksmbd.conf.example b/ksmbd.conf.example
index b752938..d346436 100644
--- a/ksmbd.conf.example
+++ b/ksmbd.conf.example
@@ -9,6 +9,7 @@
ipc timeout = 0
kerberos keytab file =
kerberos service name =
+ kerberos support = no
map to guest = never
max active sessions = 1024
max connections = 128
diff --git a/tools/config_parser.c b/tools/config_parser.c
index 2ab5e53..810e300 100644
--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -496,6 +496,10 @@ static int process_global_conf_kv(GHashTable *kv)
global_conf.krb5_keytab_file = cp_get_group_kv_string(v);
}
+ if (cp_group_kv_steal(kv, "kerberos support", &k, &v)) {
+ global_conf.krb5_support = cp_get_group_kv_bool(v);
+ }
+
if (cp_group_kv_steal(kv, "server multi channel support", &k, &v)) {
if (cp_get_group_kv_bool(v))
global_conf.flags |=
diff --git a/tools/management/spnego_krb5.c b/tools/management/spnego_krb5.c
index 94ccbea..d1fa165 100644
--- a/tools/management/spnego_krb5.c
+++ b/tools/management/spnego_krb5.c
@@ -331,7 +331,7 @@ static int setup_krb5_ctx(struct spnego_mech_ctx *mech_ctx)
struct spnego_krb5_ctx *krb5_ctx;
krb5_error_code krb_retval;
- if (mech_ctx->private)
+ if (!global_conf.krb5_support || mech_ctx->private)
return 0;
krb5_ctx = g_try_malloc0(sizeof(*krb5_ctx));
--
2.47.0
Edit: Rebased. [PATCH] ksmbd-tools: add global parameter `kerberos support' (click the arrow to expand)From 6ba581351c0effe32bea764507df6be74af32587 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Sun, 27 Oct 2024 21:58:49 +0200
Subject: [PATCH] ksmbd-tools: add global parameter `kerberos support'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If ksmbd-tools is to be packaged with it built against Kerberos 5, we
must provide a way to disable it at startup. Add global parameter
`kerberos support' with default value `no'.
Signed-off-by: Atte Heikkilä <[email protected]>
---
include/tools.h | 1 +
ksmbd.conf.5.in | 10 ++++++++--
ksmbd.conf.example | 1 +
tools/config_parser.c | 4 ++++
tools/management/spnego_krb5.c | 2 +-
5 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/include/tools.h b/include/tools.h
index cf4aa4f..019f490 100644
--- a/include/tools.h
+++ b/include/tools.h
@@ -58,6 +58,7 @@ struct smbconf_global {
unsigned int gen_subauth[3];
char *krb5_keytab_file;
char *krb5_service_name;
+ int krb5_support;
char *pwddb;
char *smbconf;
pid_t pid;
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index 61919ba..c727dc5 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -164,13 +164,19 @@ Default: \fBipc timeout = 0\fR
Path of the keytab file for the service principal.
If no value is given, it is the default keytab resolved with \fBkrb5_kt_default\fP(3).
-Default: \fBkerberos keytab file =
+Default: \fBkerberos keytab file = \fR
.TP
\fBkerberos service name (G)
Service principal name.
If no value is given, it is \fBcifs/\fR followed by the FQDN resolved with \fBgetaddrinfo\fP(3).
-Default: \fBkerberos service name =
+Default: \fBkerberos service name = \fR
+.TP
+\fBkerberos support (G)
+Support for Kerberos 5 authentication.
+For the parameter to take effect, \fBksmbd.mountd\fR must be built against Kerberos 5.
+
+Default: \fBkerberos support = no\fR
.TP
\fBmap to guest\fR (G)
When to map a user to the user given with \fBguest account\fR.
diff --git a/ksmbd.conf.example b/ksmbd.conf.example
index b752938..d346436 100644
--- a/ksmbd.conf.example
+++ b/ksmbd.conf.example
@@ -9,6 +9,7 @@
ipc timeout = 0
kerberos keytab file =
kerberos service name =
+ kerberos support = no
map to guest = never
max active sessions = 1024
max connections = 128
diff --git a/tools/config_parser.c b/tools/config_parser.c
index cd26e69..b890b96 100644
--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -514,6 +514,10 @@ static int process_global_conf_kv(GHashTable *kv)
global_conf.krb5_keytab_file = cp_get_group_kv_string(v);
}
+ if (group_kv_steal(kv, "kerberos support", &k, &v)) {
+ global_conf.krb5_support = cp_get_group_kv_bool(v);
+ }
+
if (group_kv_steal(kv, "server multi channel support", &k, &v)) {
if (cp_get_group_kv_bool(v))
global_conf.flags |=
diff --git a/tools/management/spnego_krb5.c b/tools/management/spnego_krb5.c
index 94ccbea..d1fa165 100644
--- a/tools/management/spnego_krb5.c
+++ b/tools/management/spnego_krb5.c
@@ -331,7 +331,7 @@ static int setup_krb5_ctx(struct spnego_mech_ctx *mech_ctx)
struct spnego_krb5_ctx *krb5_ctx;
krb5_error_code krb_retval;
- if (mech_ctx->private)
+ if (!global_conf.krb5_support || mech_ctx->private)
return 0;
krb5_ctx = g_try_malloc0(sizeof(*krb5_ctx));
--
2.47.0
With this patch applied, the release notes should say: Note to existing users: You must now add Note to packagers: You can now build with |
Beta Was this translation helpful? Give feedback.
-
|
Here's two more patches: [PATCH] ksmbd-tools: remove O_APPEND from terminal stdout before splice(2) (click the arrow to expand)From 48b4e9af48c55b50250af9ec735223559ae4c041 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Sat, 19 Oct 2024 00:24:16 +0300
Subject: [PATCH] ksmbd-tools: remove O_APPEND from terminal stdout before
splice(2)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Some programs, e.g. GNU Make, set O_APPEND on the terminal and don't
cleanup when done. Since splice(2) returns EINVAL if the target fd has
it set, remove it from terminal stdout.
Signed-off-by: Atte Heikkilä <[email protected]>
---
control/control.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/control/control.c b/control/control.c
index 4e05a28..70b93c8 100644
--- a/control/control.c
+++ b/control/control.c
@@ -8,6 +8,7 @@
#define _GNU_SOURCE
#include <fcntl.h>
+#include <unistd.h>
#include <getopt.h>
#include <errno.h>
#include <sys/types.h>
@@ -146,6 +147,15 @@ static int control_list(void)
goto out_close;
}
+ if (isatty(STDOUT_FILENO) &&
+ fcntl(STDOUT_FILENO,
+ F_SETFL,
+ fcntl(STDOUT_FILENO, F_GETFL) & ~O_APPEND) < 0) {
+ ret = -errno;
+ pr_debug("Can't control terminal: %m\n");
+ goto out_close;
+ }
+
ret = cp_parse_lock();
if (ret)
goto out_close;
--
2.47.0
[PATCH] ksmbd-tools: fix ksmbd.conf(5) formatting error (click the arrow to expand)From 7b3309e5694fd9f6ef1dff1d3fa68d8d81b8d3b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Sat, 19 Oct 2024 00:24:29 +0300
Subject: [PATCH] ksmbd-tools: fix ksmbd.conf(5) formatting error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Atte Heikkilä <[email protected]>
---
ksmbd.conf.5.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index 2fea4f3..6c9761b 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -19,7 +19,7 @@ A name may contain leading and trailing tabs and spaces.
A value, which begins immediately after the equal sign, may contain leading tabs and spaces or be empty.
A value may be a list of multiple values separated by commas, tabs, and spaces.
For a list of users, all users in a system group are given by giving the group name prefixed with an at (\fB@\fR).
-A value may have a number suffix, which is either \fBK\R, \fBM\R, \fBG\R, \fBT\R, \fBP\R, or \fBE\R.
+A value may have a number suffix, which is either \fBK\fR, \fBM\fR, \fBG\fR, \fBT\fR, \fBP\fR, or \fBE\fR.
A semicolon (\fB;\fR) or a hash (\fB#\fR) marks the beginning of a comment which continues until the end of the line.
.SH SHARES
Each section name, except that of the \fBglobal\fR section, defines a shared resource, commonly referred to as a share.
--
2.47.0
|
Beta Was this translation helpful? Give feedback.
-
|
@atheik |
Beta Was this translation helpful? Give feedback.
-
|
Here's a documentation patch: [PATCH] ksmbd-tools: document duplicate key-value handling (click the arrow to expand)From 97e15552891cba0e7638797edeac0385711f0b62 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <[email protected]>
Date: Thu, 31 Oct 2024 23:37:11 +0200
Subject: [PATCH] ksmbd-tools: document duplicate key-value handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The config parser has always ignored duplicate key-values. This may
very well be surprising to users so document it. Let's also document
how duplicate sections are handled, which conversely is typical, and
improve some wording.
Signed-off-by: Atte Heikkilä <[email protected]>
---
ksmbd.conf.5.in | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index c76be44..1c8f0ea 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -21,11 +21,13 @@ A value may be a list of multiple values separated by commas, tabs, and spaces.
For a list of users, all users in a system group are given by giving the group name prefixed with an at (\fB@\fR).
A value may have a number suffix, which is either \fBK\fR, \fBM\fR, \fBG\fR, \fBT\fR, \fBP\fR, or \fBE\fR.
A semicolon (\fB;\fR) or a hash (\fB#\fR) marks the beginning of a comment which continues until the end of the line.
+If a section has the same name as a previous section, it is a continuation of that previous section, i.e. they are the same section.
+A duplicate parameter in a section has its value updated only if its previous value was empty.
.SH SHARES
Each section name, except that of the \fBglobal\fR section, defines a shared resource, commonly referred to as a share.
A section name, which is the share name, must be UTF-8, [1, 64) bytes, and is case-insensitive. \" KSMBD_REQ_MAX_SHARE_NAME
Users that may be allowed to connect to a share are those that are present in \fBksmbdpwd.db\fR(5) user database.
-A share may limit which users are allowed to connect to a particular share.
+A share may limit which users are allowed to connect to it.
When connected to a share, the user is mapped to a system user and underlying filesystem permissions are enforced.
By default, this mapping is done by name, but it may also be done by mapping all users connected to the share to a single system user and group.
When connecting as a user not in the user database, only guest sessions may work.
--
2.47.0
By the way, there has not been a Travis CI build for ksmbd-tools for a while now. |
Beta Was this translation helpful? Give feedback.
-
|
Applied it:)
And I will work it this weekend. |
Beta Was this translation helpful? Give feedback.
-
[DIFF] Fix CI (click the arrow to expand)diff --git a/.github/workflows/c-cpp.yml b/.github/workflows/c-cpp.yml
index e13bb12..7432a69 100644
--- a/.github/workflows/c-cpp.yml
+++ b/.github/workflows/c-cpp.yml
@@ -24,24 +24,40 @@ jobs:
g++ --version
pip3 install --user meson
PATH=$HOME/.local/bin:$PATH
- - name: autotools build with mit krb5
- run: |
./autogen.sh
- ./configure
+ - name: autotools build without krb5
+ run: |
+ mkdir autotools-no-krb5-build
+ cd autotools-no-krb5-build
+ ../configure
make DISTCHECK_CONFIGURE_FLAGS=--enable-krb5=no distcheck
- name: autotools build with mit krb5
run: |
- ./autogen.sh
- ./configure
+ mkdir autotools-mit-krb5-build
+ cd autotools-mit-krb5-build
+ ../configure
make DISTCHECK_CONFIGURE_FLAGS="LIBKRB5_CFLAGS='$(krb5-config.mit --cflags)' LIBKRB5_LIBS='$(krb5-config.mit --libs)' --enable-krb5" distcheck
- name: autotools build with heimdal krb5
run: |
- ./autogen.sh
- ./configure
+ mkdir autotools-heimdal-krb5-build
+ cd autotools-heimdal-krb5-build
+ ../configure
make DISTCHECK_CONFIGURE_FLAGS="LIBKRB5_CFLAGS='$(krb5-config.heimdal --cflags)' LIBKRB5_LIBS='$(krb5-config.heimdal --libs) -lasn1' --enable-krb5" distcheck
- name: meson build without krb5
run: |
- mkdir build
- cd build
+ mkdir meson-no-krb5-build
+ cd meson-no-krb5-build
meson -Dkrb5=disabled ..
meson dist
+ - name: meson build with mit krb5
+ run: |
+ mkdir meson-mit-krb5-build
+ cd meson-mit-krb5-build
+ meson -Dkrb5=enabled -Dkrb5_name=mit-krb5 ..
+ meson dist
+ - name: meson build with heimdal krb5
+ run: |
+ mkdir meson-heimdal-krb5-build
+ cd meson-heimdal-krb5-build
+ meson -Dkrb5=enabled -Dkrb5_name=heimdal-krb5 ..
+ meson dist
|
Beta Was this translation helpful? Give feedback.
-
|
@atheik Applied, thanks for your work! |
Beta Was this translation helpful? Give feedback.
-
|
@atheik most changes is made by you. Can you help write release note for ksmbd-tools-3.5.3 ? |
Beta Was this translation helpful? Give feedback.
-
|
Sure. Thank you again for taking the time to review my patches. |
Beta Was this translation helpful? Give feedback.
-
|
@atheik Looks great to me! Thanks for your help! |
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