ksmbd does not preserve supplementary groups

[bol-van]

Ksmbd 3.4.8 does not allow access to the file system based on supplimentary group In the same scenario if samba4-server used instead of ksmbd - all works as expected.

For example, i am "user" with main group "user" and extra group "sftp" There's dir "dir" and some content inside. "NO ACCESS" means i cant see anything inside "dir" when connected as "user" "OK" means I can see content inside "dir"

chown dir sftp:sftp chmod dir 770 NO ACCESS chown dir sftp:sftp chmod dir 775 OK chown dir sftp:user chmod dir 770 OK

[namjaejeon]

Can you elaborate more ? Where did you command chown/chmod ? through smb client ? or on local share directory ? So my questions are:

1. Where did you command chown/chmod ? through smb client ? or on local share directory ?
2. NO ACCESS => Where did you access file ? through smb client ?
3. if you use cifs.ko(mount -t cifs) as smb client, Let me know your mount options.

[bol-van]

I did chown/chmod on the systems where ksmbd runs. SMB client is windows. net use X: \ksmbd-server\share /user:user

ksmbd-server is openwrt. just install samba4-server instead of ksmbd and it works

[lekzz]

I also have this issue after switching from samba to ksmbd. Supplementary/extra groups defined in /etc/group using "usermod -a -G group user" are ignored, which results in no access if there are no other/world rights.

So for example using the user/groups from the OP and no other/world rights, there is access to a file/dir when either the user or the group matches the user or primary group: -rw-rw---- user:group -rw-rw---- otheruser:group -rw-rw---- user:othergroup

However there is no access for extra groups, not even read: -rw-rw---- otheruser:sftp

Server and client are running kernel 6.10.11, client is using systemd mount: [Mount] What=//server/share Where=/mnt/mount Type=cifs Options=uid=1000,gid=1000,credentials=/home/user/.smbcred

Which results in the following mount: //server/share on /mnt/mount type cifs (rw,relatime,vers=3.1.1,cache=strict,username=user,domain=domain.com,uid=1000,forceuid,gid=1000,forcegid,addr=ip.add.re.ss,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,reparse=nfs,rsize=4194304,wsize=4194304,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)

EDIT: forgot to mention that setting "force group (S)" on the share can be a sort of workaround, although probably not advised and only usable in single user scenarios. So in the no access example if "force group = sftp" is used, then access is possible.

[atheik]

@namjaejeon,

This looks like the same issue as https://github.com/cifsd-team/ksmbd-tools/issues/200#issuecomment-2352791784.

[namjaejeon]

@atheik Thanks for your check:), I am working it now.

[namjaejeon]

@atheik Can you review these patches ? https://github.com/namjaejeon/ksmbd/commit/c69e41c6fb65c963b7ed743a843b79d1689e1140 https://github.com/namjaejeon/ksmbd-tools/commit/e8f456af63dacc216b8efb4af330fdf2baeceabd

[atheik]

@namjaejeon,

Thank you for the fix! I left some preliminary comments. I will test your patches more later this week.

[namjaejeon]

@atheik I have updated ksmbd patch as you pointed out. https://github.com/namjaejeon/ksmbd/commit/01d9be533081c56a922181a07e87654e6d1c5698 Feel free to check this patch at this week. Thanks for your review!

[namjaejeon]

@atheik Hmm.. I need to add login_response_extension ipc command for backward compatibility. I need more time for this. I will share if it is ready. Thanks!

[namjaejeon]

@atheik Can you check these patches ? https://github.com/namjaejeon/ksmbd/commit/4a6642e1f11a843704b22a4138f5d9e6d09614ed https://github.com/namjaejeon/ksmbd-tools/commit/1f87e4ec4f4b1add301425c16fe6028e7ca03f38

Thanks!

[atheik]

@namjaejeon,

Thanks again. I will do some testing.

By the way, here is an alternative approach:

[PATCH] ksmbd: add support for supplementary groups (click the arrow to expand)

````diff From 5f31d6cf291bce1345aac1b6e74e9335f698fd6b Mon Sep 17 00:00:00 2001 From: Namjae Jeon Date: Thu, 3 Oct 2024 00:45:22 +0300 Subject: [PATCH] ksmbd: add support for supplementary groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Atte Heikkilä Signed-off-by: Atte Heikkilä --- auth.c | 4 ++++ ksmbd_netlink.h | 4 +++- mgmt/user_config.c | 10 +++++++++- mgmt/user_config.h | 3 +++ smb_common.c | 21 ++++++++++++++++++--- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/auth.c b/auth.c index 394f9c0..8d1c732 100644 --- a/auth.c +++ b/auth.c @@ -839,6 +839,10 @@ int ksmbd_krb5_authenticate(struct ksmbd_session *sess, char *in_blob, resp->spnego_blob_len); *out_len = resp->spnego_blob_len; retval = 0; + + memcpy(user->sgid, resp->payload + + resp->session_key_len + resp->spnego_blob_len, + resp->login_response.ngroups * sizeof(__u32)); out: kvfree(resp); return retval; diff --git a/ksmbd_netlink.h b/ksmbd_netlink.h index d9de08d..bccede9 100644 --- a/ksmbd_netlink.h +++ b/ksmbd_netlink.h @@ -142,7 +142,9 @@ struct ksmbd_login_response { __u16 status; __u16 hash_sz; /* hash size */ __s8 hash[KSMBD_REQ_MAX_HASH_SZ]; /* password hash */ - __u32 reserved[16]; /* Reserved room */ + __u32 reserved[15]; /* Reserved room */ + __s32 ngroups; + __s8 ____payload[]; }; /* diff --git a/mgmt/user_config.c b/mgmt/user_config.c index 279d00f..5f9036f 100644 --- a/mgmt/user_config.c +++ b/mgmt/user_config.c @@ -22,6 +22,8 @@ struct ksmbd_user *ksmbd_login_user(const char *account) goto out; user = ksmbd_alloc_user(resp); + memcpy(user->sgid, resp->____payload, + resp->ngroups * sizeof(__u32)); out: kvfree(resp); return user; @@ -48,14 +50,20 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp) kfree(user->name); kfree(user->passkey); kfree(user); - user = NULL; + return NULL; } + + user->sgid = kcalloc(resp->ngroups, sizeof(__u32), GFP_KERNEL); + if (user->sgid) + user->ngroups = resp->ngroups; + return user; } void ksmbd_free_user(struct ksmbd_user *user) { ksmbd_ipc_logout_request(user->name, user->flags); + kfree(user->sgid); kfree(user->name); kfree(user->passkey); kfree(user); diff --git a/mgmt/user_config.h b/mgmt/user_config.h index e068a19..d0d53a7 100644 --- a/mgmt/user_config.h +++ b/mgmt/user_config.h @@ -18,6 +18,9 @@ struct ksmbd_user { size_t passkey_sz; char *passkey; + + int ngroups; + unsigned int *sgid; }; static inline bool user_guest(struct ksmbd_user *user) diff --git a/smb_common.c b/smb_common.c index 613c2f8..3f6beed 100644 --- a/smb_common.c +++ b/smb_common.c @@ -781,13 +781,15 @@ int __ksmbd_override_fsids(struct ksmbd_work *work, struct ksmbd_share_config *share) { struct ksmbd_session *sess = work->sess; + struct ksmbd_user *user = sess->user; struct cred *cred; struct group_info *gi; unsigned int uid; unsigned int gid; + int i; - uid = user_uid(sess->user); - gid = user_gid(sess->user); + uid = user_uid(user); + gid = user_gid(user); if (share->force_uid != KSMBD_SHARE_INVALID_UID) uid = share->force_uid; if (share->force_gid != KSMBD_SHARE_INVALID_GID) @@ -800,11 +802,24 @@ int __ksmbd_override_fsids(struct ksmbd_work *work, cred->fsuid = make_kuid(&init_user_ns, uid); cred->fsgid = make_kgid(&init_user_ns, gid); - gi = groups_alloc(0); + gi = groups_alloc(user->ngroups); if (!gi) { abort_creds(cred); return -ENOMEM; } + + for (i = 0; i < user->ngroups; i++) { + if (gid_eq(GLOBAL_ROOT_GID, + make_kgid(&init_user_ns, user->sgid[i]))) + gi->gid[i] = cred->fsgid; + else + gi->gid[i] = make_kgid(&init_user_ns, + user->sgid[i]); + } + + if (user->ngroups) + groups_sort(gi); + set_groups(cred, gi); put_group_info(gi); -- 2.46.2 ````

[PATCH] ksmbd-tools: add support for supplementary groups (click the arrow to expand)

````diff From f4c7f49d6b30c648f61e9826efc7d701de6fee27 Mon Sep 17 00:00:00 2001 From: Namjae Jeon Date: Thu, 3 Oct 2024 00:54:30 +0300 Subject: [PATCH] ksmbd-tools: add support for supplementary groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Atte Heikkilä Signed-off-by: Atte Heikkilä --- include/linux/ksmbd_server.h | 4 ++- include/management/user.h | 3 ++ mountd/worker.c | 67 ++++++++++++++++++++++++++++++++---- tools/management/user.c | 31 +++++++++++++++++ 4 files changed, 97 insertions(+), 8 deletions(-) diff --git a/include/linux/ksmbd_server.h b/include/linux/ksmbd_server.h index 7a40c95..965b87d 100644 --- a/include/linux/ksmbd_server.h +++ b/include/linux/ksmbd_server.h @@ -76,7 +76,9 @@ struct ksmbd_login_response { __u16 status; __u16 hash_sz; __s8 hash[KSMBD_REQ_MAX_HASH_SZ]; - __u32 reserved[16]; /* Reserved room */ + __u32 reserved[15]; /* Reserved room */ + __s32 ngroups; + __s8 ____payload[]; }; struct ksmbd_share_config_request { diff --git a/include/management/user.h b/include/management/user.h index 990e8da..ea90c87 100644 --- a/include/management/user.h +++ b/include/management/user.h @@ -27,6 +27,9 @@ struct ksmbd_user { int state; GRWLock update_lock; unsigned int failed_login_count; + + int ngroups; + gid_t *sgid; }; static inline void set_user_flag(struct ksmbd_user *user, int bit) diff --git a/mountd/worker.c b/mountd/worker.c index fd01833..026d3c3 100644 --- a/mountd/worker.c +++ b/mountd/worker.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -32,17 +33,60 @@ static GThreadPool *pool; ret; \ }) +static int login_response_payload(char *account, + struct ksmbd_ipc_msg *resp_msg) +{ + int payload_sz; + struct ksmbd_user *user; + + if (account[0] == '\0') + return 0; + + user = usm_lookup_user(account); + if (!user) + return 0; + + payload_sz = sizeof(gid_t) * user->ngroups; + if (resp_msg) { + void *payload = NULL; + + if (resp_msg->type == KSMBD_EVENT_LOGIN_RESPONSE) { + struct ksmbd_login_response *resp; + + resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); + payload = resp->____payload; + } + + if (resp_msg->type == KSMBD_EVENT_SPNEGO_AUTHEN_RESPONSE) { + struct ksmbd_spnego_authen_response *resp; + + resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); + payload = resp->payload + resp->session_key_len + + resp->spnego_blob_len; + } + + if (payload) + memcpy(payload, user->sgid, payload_sz); + } + + put_ksmbd_user(user); + return payload_sz; +} + static int login_request(struct ksmbd_ipc_msg *msg) { struct ksmbd_login_request *req; struct ksmbd_login_response *resp; struct ksmbd_ipc_msg *resp_msg; + int payload_sz; - resp_msg = ipc_msg_alloc(sizeof(*resp)); + req = KSMBD_IPC_MSG_PAYLOAD(msg); + + payload_sz = login_response_payload(req->account, NULL); + resp_msg = ipc_msg_alloc(sizeof(*resp) + payload_sz); if (!resp_msg) goto out; - req = KSMBD_IPC_MSG_PAYLOAD(msg); resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); resp->status = KSMBD_USER_FLAG_INVALID; @@ -52,12 +96,14 @@ static int login_request(struct ksmbd_ipc_msg *msg) resp_msg->type = KSMBD_EVENT_LOGIN_RESPONSE; resp->handle = req->handle; + login_response_payload(req->account, resp_msg); ipc_msg_send(resp_msg); out: ipc_msg_free(resp_msg); return 0; } +/* FIXME: untested */ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) { struct ksmbd_spnego_authen_request *req; @@ -65,6 +111,7 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) struct ksmbd_ipc_msg *resp_msg = NULL; struct ksmbd_spnego_auth_out auth_out; struct ksmbd_login_request login_req; + int login_resp_payload_sz; int retval = 0; req = KSMBD_IPC_MSG_PAYLOAD(msg); @@ -87,9 +134,16 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) goto out; } + /* login */ + login_req.handle = req->handle; + strncpy(login_req.account, auth_out.user_name, + sizeof(login_req.account)); + login_resp_payload_sz = login_response_payload(login_req.account, NULL); + ipc_msg_free(resp_msg); resp_msg = ipc_msg_alloc(sizeof(*resp) + - auth_out.key_len + auth_out.blob_len); + auth_out.key_len + auth_out.blob_len + + login_resp_payload_sz); if (!resp_msg) { retval = -ENOMEM; goto out_free_auth; @@ -100,10 +154,7 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) resp->handle = req->handle; resp->login_response.status = KSMBD_USER_FLAG_INVALID; - /* login */ - login_req.handle = req->handle; - strncpy(login_req.account, auth_out.user_name, - sizeof(login_req.account)); + /* login cont. */ usm_handle_login_request(&login_req, &resp->login_response); if (!(resp->login_response.status & KSMBD_USER_FLAG_OK)) { pr_info("Unable to login user `%s'\n", login_req.account); @@ -115,6 +166,8 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) resp->spnego_blob_len = auth_out.blob_len; memcpy(resp->payload + auth_out.key_len, auth_out.spnego_blob, auth_out.blob_len); + + login_response_payload(login_req.account, resp_msg); out_free_auth: g_free(auth_out.spnego_blob); g_free(auth_out.sess_key); diff --git a/tools/management/user.c b/tools/management/user.c index 19d42a8..92c25bb 100644 --- a/tools/management/user.c +++ b/tools/management/user.c @@ -10,6 +10,7 @@ #include #include #include +#include #include "linux/ksmbd_server.h" #include "management/share.h" @@ -31,6 +32,7 @@ static void kill_ksmbd_user(struct ksmbd_user *user) g_free(user->name); g_free(user->pass_b64); g_free(user->pass); + g_free(user->sgid); g_rw_lock_clear(&user->update_lock); g_free(user); } @@ -99,6 +101,8 @@ static struct ksmbd_user *new_ksmbd_user(char *name, char *pwd) struct ksmbd_user *user; struct passwd *e; size_t pass_sz; + gid_t *sgid = NULL; + int ngroups = 1, i; user = g_try_malloc0(sizeof(struct ksmbd_user)); if (!user) @@ -120,6 +124,31 @@ static struct ksmbd_user *new_ksmbd_user(char *name, char *pwd) user->pass = base64_decode(user->pass_b64, &pass_sz); user->pass_sz = (int)pass_sz; + + if (!e) + goto out; + + do { + g_free(sgid); + sgid = g_try_malloc0(sizeof(gid_t) * ngroups); + if (!sgid) { + g_free(user); + return NULL; + } + } while (getgrouplist(name, e->pw_gid, sgid, &ngroups) < 0); + + user->ngroups = ngroups; + user->sgid = sgid; + + /* FIXME: too verbose -- move some of this to usm_add_new_user()? */ + pr_debug("username : %s, ngroups : %d\n", name, ngroups); + for (i = 0; i < ngroups; i++) { + struct group *ge = getgrgid(sgid[i]); + if (ge) + pr_debug("gid : %d(%s)\n", sgid[i], ge->gr_name); + } + +out: return user; } @@ -340,6 +369,8 @@ static void __handle_login_request(struct ksmbd_login_response *resp, sizeof(resp->account))) resp->status = KSMBD_USER_FLAG_INVALID; } + + resp->ngroups = user->ngroups; } int usm_handle_login_request(struct ksmbd_login_request *req, -- 2.46.2 ````

(Apply on commit 8cd0f00 and commit 78bef66, respectively.)

Edit: I will add here that I don't know if my approach is any good. I did very little testing of these patches.

[namjaejeon]

By the way, here is an alternative approach:

@atheik Okay, Looks clean. Have you checked backward compatibility with the following combinations ?

1. ksmbd + supplementary groups patch & ksmbd-tools master
2. ksmbd master branch & ksmbd-tools + supplementary groups patch

[atheik]

@namjaejeon,

Have you checked backward compatibility with the following combinations ?

For your approach, I have checked these combinations:

1. ksmbd 8cd0f00 and ksmbd-tools 1f87e4e
2. ksmbd 4a6642e and ksmbd-tools 78bef66
3. ksmbd 4a6642e and ksmbd-tools 1f87e4e

For my approach, I have checked the equivalent combinations.

Neither of our approaches work with Kerberos 5. First, looks like Kerberos 5 authentication has always had intermittent failures, and here is a fix:

[PATCH] ksmbd-tools: fix encode_negTokenTarg() return value (click the arrow to expand)

````diff From 796dcbb8f880cdbefe80fbed494c72402dbcfdee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= Date: Thu, 3 Oct 2024 16:49:35 +0300 Subject: [PATCH] ksmbd-tools: fix encode_negTokenTarg() return value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kerberos 5 authentication requests have been prone to intermittent failures. This is due to encode_negTokenTarg() not having a defined return value on the success code path, which then leads to a likely authentication error in spnego_handle_authen_request(). Fix this by returning 0 on success. This bug was introduced when encode_negTokenTarg() was first added in commit 9aacb96. Signed-off-by: Atte Heikkilä --- tools/management/spnego.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/management/spnego.c b/tools/management/spnego.c index 6a9d207..a5a79b4 100644 --- a/tools/management/spnego.c +++ b/tools/management/spnego.c @@ -303,6 +303,7 @@ static int encode_negTokenTarg(char *in_blob, int in_len, g_free(sup_oid); g_free(krb5_oid); + return 0; } int spnego_handle_authen_request(struct ksmbd_spnego_authen_request *req, -- 2.46.2 ````

Next, both of our approaches hit ksmbd: SPNEGO_AUTHEN_REQUEST failure due to failed ipc_validate_msg() in ipc_msg_send_request() in ksmbd_ipc_spnego_authen_request().

Here's new version of my approach that works with Kerberos 5:

[PATCH v2] ksmbd: add support for supplementary groups (click the arrow to expand)

````diff From b645c0e5c99cc9930f1108c5abe8fd7046a73622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= Date: Thu, 3 Oct 2024 19:01:26 +0300 Subject: [PATCH v2] ksmbd: add support for supplementary groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Namjae Jeon Signed-off-by: Atte Heikkilä --- auth.c | 4 ++++ ksmbd_netlink.h | 4 +++- mgmt/user_config.c | 37 +++++++++++++++++++++++++++++++------ mgmt/user_config.h | 3 +++ smb_common.c | 21 ++++++++++++++++++--- transport_ipc.c | 3 ++- 6 files changed, 61 insertions(+), 11 deletions(-) diff --git a/auth.c b/auth.c index 394f9c0..10efcde 100644 --- a/auth.c +++ b/auth.c @@ -839,6 +839,10 @@ int ksmbd_krb5_authenticate(struct ksmbd_session *sess, char *in_blob, resp->spnego_blob_len); *out_len = resp->spnego_blob_len; retval = 0; + + memcpy(user->sgid, resp->payload + + resp->session_key_len + resp->spnego_blob_len, + user->ngroups * sizeof(__u32)); out: kvfree(resp); return retval; diff --git a/ksmbd_netlink.h b/ksmbd_netlink.h index d9de08d..bccede9 100644 --- a/ksmbd_netlink.h +++ b/ksmbd_netlink.h @@ -142,7 +142,9 @@ struct ksmbd_login_response { __u16 status; __u16 hash_sz; /* hash size */ __s8 hash[KSMBD_REQ_MAX_HASH_SZ]; /* password hash */ - __u32 reserved[16]; /* Reserved room */ + __u32 reserved[15]; /* Reserved room */ + __s32 ngroups; + __s8 ____payload[]; }; /* diff --git a/mgmt/user_config.c b/mgmt/user_config.c index 279d00f..328463f 100644 --- a/mgmt/user_config.c +++ b/mgmt/user_config.c @@ -22,6 +22,22 @@ struct ksmbd_user *ksmbd_login_user(const char *account) goto out; user = ksmbd_alloc_user(resp); + /* IDEAS + * + * In ksmbd-tools, we now have login_response_payload() to deal + * with the sgid payload being in different places depending + * on whether we have login_response or spnego_authen_response. + * + * Should this memcpy() and the one in ksmbd_krb5_authenticate() + * be moved to a common function? + * + * e.g. + * + * int ksmbd_login_payload(struct ksmbd_user *user, void *payload); + * + */ + memcpy(user->sgid, resp->____payload, + user->ngroups * sizeof(__u32)); out: kvfree(resp); return user; @@ -44,13 +60,21 @@ struct ksmbd_user *ksmbd_alloc_user(struct ksmbd_login_response *resp) if (user->passkey) memcpy(user->passkey, resp->hash, resp->hash_sz); - if (!user->name || !user->passkey) { - kfree(user->name); - kfree(user->passkey); - kfree(user); - user = NULL; - } + if (!user->name || !user->passkey) + goto err_free; + + user->sgid = kcalloc(resp->ngroups, sizeof(__u32), GFP_KERNEL); + if (user->sgid) + user->ngroups = resp->ngroups; + else if (resp->ngroups > 0) + goto err_free; + return user; +err_free: + kfree(user->name); + kfree(user->passkey); + kfree(user); + return NULL; } void ksmbd_free_user(struct ksmbd_user *user) @@ -58,6 +82,7 @@ void ksmbd_free_user(struct ksmbd_user *user) ksmbd_ipc_logout_request(user->name, user->flags); kfree(user->name); kfree(user->passkey); + kfree(user->sgid); kfree(user); } diff --git a/mgmt/user_config.h b/mgmt/user_config.h index e068a19..d0d53a7 100644 --- a/mgmt/user_config.h +++ b/mgmt/user_config.h @@ -18,6 +18,9 @@ struct ksmbd_user { size_t passkey_sz; char *passkey; + + int ngroups; + unsigned int *sgid; }; static inline bool user_guest(struct ksmbd_user *user) diff --git a/smb_common.c b/smb_common.c index 613c2f8..3f6beed 100644 --- a/smb_common.c +++ b/smb_common.c @@ -781,13 +781,15 @@ int __ksmbd_override_fsids(struct ksmbd_work *work, struct ksmbd_share_config *share) { struct ksmbd_session *sess = work->sess; + struct ksmbd_user *user = sess->user; struct cred *cred; struct group_info *gi; unsigned int uid; unsigned int gid; + int i; - uid = user_uid(sess->user); - gid = user_gid(sess->user); + uid = user_uid(user); + gid = user_gid(user); if (share->force_uid != KSMBD_SHARE_INVALID_UID) uid = share->force_uid; if (share->force_gid != KSMBD_SHARE_INVALID_GID) @@ -800,11 +802,24 @@ int __ksmbd_override_fsids(struct ksmbd_work *work, cred->fsuid = make_kuid(&init_user_ns, uid); cred->fsgid = make_kgid(&init_user_ns, gid); - gi = groups_alloc(0); + gi = groups_alloc(user->ngroups); if (!gi) { abort_creds(cred); return -ENOMEM; } + + for (i = 0; i < user->ngroups; i++) { + if (gid_eq(GLOBAL_ROOT_GID, + make_kgid(&init_user_ns, user->sgid[i]))) + gi->gid[i] = cred->fsgid; + else + gi->gid[i] = make_kgid(&init_user_ns, + user->sgid[i]); + } + + if (user->ngroups) + groups_sort(gi); + set_groups(cred, gi); put_group_info(gi); diff --git a/transport_ipc.c b/transport_ipc.c index 85662f3..f6600c1 100644 --- a/transport_ipc.c +++ b/transport_ipc.c @@ -469,7 +469,8 @@ static int ipc_validate_msg(struct ipc_msg_table_entry *entry) struct ksmbd_spnego_authen_response *resp = entry->response; msg_sz = sizeof(struct ksmbd_spnego_authen_response) + - resp->session_key_len + resp->spnego_blob_len; + resp->session_key_len + resp->spnego_blob_len + + resp->login_response.ngroups * sizeof(__u32); } else if (entry->type == KSMBD_EVENT_SHARE_CONFIG_REQUEST) { struct ksmbd_share_config_response *resp = entry->response; -- 2.46.2 ````

[PATCH v2] ksmbd-tools: add support for supplementary groups (click the arrow to expand)

````diff From a135d91ef28fb78cf573d9049637a92833e19d5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= Date: Thu, 3 Oct 2024 19:01:46 +0300 Subject: [PATCH v2] ksmbd-tools: add support for supplementary groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Namjae Jeon Signed-off-by: Atte Heikkilä --- include/linux/ksmbd_server.h | 4 ++- include/management/user.h | 3 ++ mountd/worker.c | 66 ++++++++++++++++++++++++++++++++---- tools/management/user.c | 31 +++++++++++++++++ 4 files changed, 96 insertions(+), 8 deletions(-) diff --git a/include/linux/ksmbd_server.h b/include/linux/ksmbd_server.h index 7a40c95..965b87d 100644 --- a/include/linux/ksmbd_server.h +++ b/include/linux/ksmbd_server.h @@ -76,7 +76,9 @@ struct ksmbd_login_response { __u16 status; __u16 hash_sz; __s8 hash[KSMBD_REQ_MAX_HASH_SZ]; - __u32 reserved[16]; /* Reserved room */ + __u32 reserved[15]; /* Reserved room */ + __s32 ngroups; + __s8 ____payload[]; }; struct ksmbd_share_config_request { diff --git a/include/management/user.h b/include/management/user.h index 990e8da..ea90c87 100644 --- a/include/management/user.h +++ b/include/management/user.h @@ -27,6 +27,9 @@ struct ksmbd_user { int state; GRWLock update_lock; unsigned int failed_login_count; + + int ngroups; + gid_t *sgid; }; static inline void set_user_flag(struct ksmbd_user *user, int bit) diff --git a/mountd/worker.c b/mountd/worker.c index fd01833..936e40d 100644 --- a/mountd/worker.c +++ b/mountd/worker.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -32,17 +33,60 @@ static GThreadPool *pool; ret; \ }) +static int login_response_payload(char *account, + struct ksmbd_ipc_msg *resp_msg) +{ + int payload_sz; + struct ksmbd_user *user; + + if (account[0] == '\0') + return 0; + + user = usm_lookup_user(account); + if (!user) + return 0; + + payload_sz = sizeof(gid_t) * user->ngroups; + if (resp_msg) { + void *payload = NULL; + + if (resp_msg->type == KSMBD_EVENT_LOGIN_RESPONSE) { + struct ksmbd_login_response *resp; + + resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); + payload = resp->____payload; + } + + if (resp_msg->type == KSMBD_EVENT_SPNEGO_AUTHEN_RESPONSE) { + struct ksmbd_spnego_authen_response *resp; + + resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); + payload = resp->payload + resp->session_key_len + + resp->spnego_blob_len; + } + + if (payload) + memcpy(payload, user->sgid, payload_sz); + } + + put_ksmbd_user(user); + return payload_sz; +} + static int login_request(struct ksmbd_ipc_msg *msg) { struct ksmbd_login_request *req; struct ksmbd_login_response *resp; struct ksmbd_ipc_msg *resp_msg; + int payload_sz; - resp_msg = ipc_msg_alloc(sizeof(*resp)); + req = KSMBD_IPC_MSG_PAYLOAD(msg); + + payload_sz = login_response_payload(req->account, NULL); + resp_msg = ipc_msg_alloc(sizeof(*resp) + payload_sz); if (!resp_msg) goto out; - req = KSMBD_IPC_MSG_PAYLOAD(msg); resp = KSMBD_IPC_MSG_PAYLOAD(resp_msg); resp->status = KSMBD_USER_FLAG_INVALID; @@ -52,6 +96,7 @@ static int login_request(struct ksmbd_ipc_msg *msg) resp_msg->type = KSMBD_EVENT_LOGIN_RESPONSE; resp->handle = req->handle; + login_response_payload(req->account, resp_msg); ipc_msg_send(resp_msg); out: ipc_msg_free(resp_msg); @@ -65,6 +110,7 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) struct ksmbd_ipc_msg *resp_msg = NULL; struct ksmbd_spnego_auth_out auth_out; struct ksmbd_login_request login_req; + int login_resp_payload_sz; int retval = 0; req = KSMBD_IPC_MSG_PAYLOAD(msg); @@ -87,9 +133,16 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) goto out; } + /* login */ + login_req.handle = req->handle; + strncpy(login_req.account, auth_out.user_name, + sizeof(login_req.account)); + login_resp_payload_sz = login_response_payload(login_req.account, NULL); + ipc_msg_free(resp_msg); resp_msg = ipc_msg_alloc(sizeof(*resp) + - auth_out.key_len + auth_out.blob_len); + auth_out.key_len + auth_out.blob_len + + login_resp_payload_sz); if (!resp_msg) { retval = -ENOMEM; goto out_free_auth; @@ -100,10 +153,7 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) resp->handle = req->handle; resp->login_response.status = KSMBD_USER_FLAG_INVALID; - /* login */ - login_req.handle = req->handle; - strncpy(login_req.account, auth_out.user_name, - sizeof(login_req.account)); + /* login cont. */ usm_handle_login_request(&login_req, &resp->login_response); if (!(resp->login_response.status & KSMBD_USER_FLAG_OK)) { pr_info("Unable to login user `%s'\n", login_req.account); @@ -115,6 +165,8 @@ static int spnego_authen_request(struct ksmbd_ipc_msg *msg) resp->spnego_blob_len = auth_out.blob_len; memcpy(resp->payload + auth_out.key_len, auth_out.spnego_blob, auth_out.blob_len); + + login_response_payload(login_req.account, resp_msg); out_free_auth: g_free(auth_out.spnego_blob); g_free(auth_out.sess_key); diff --git a/tools/management/user.c b/tools/management/user.c index 19d42a8..92c25bb 100644 --- a/tools/management/user.c +++ b/tools/management/user.c @@ -10,6 +10,7 @@ #include #include #include +#include #include "linux/ksmbd_server.h" #include "management/share.h" @@ -31,6 +32,7 @@ static void kill_ksmbd_user(struct ksmbd_user *user) g_free(user->name); g_free(user->pass_b64); g_free(user->pass); + g_free(user->sgid); g_rw_lock_clear(&user->update_lock); g_free(user); } @@ -99,6 +101,8 @@ static struct ksmbd_user *new_ksmbd_user(char *name, char *pwd) struct ksmbd_user *user; struct passwd *e; size_t pass_sz; + gid_t *sgid = NULL; + int ngroups = 1, i; user = g_try_malloc0(sizeof(struct ksmbd_user)); if (!user) @@ -120,6 +124,31 @@ static struct ksmbd_user *new_ksmbd_user(char *name, char *pwd) user->pass = base64_decode(user->pass_b64, &pass_sz); user->pass_sz = (int)pass_sz; + + if (!e) + goto out; + + do { + g_free(sgid); + sgid = g_try_malloc0(sizeof(gid_t) * ngroups); + if (!sgid) { + g_free(user); + return NULL; + } + } while (getgrouplist(name, e->pw_gid, sgid, &ngroups) < 0); + + user->ngroups = ngroups; + user->sgid = sgid; + + /* FIXME: too verbose -- move some of this to usm_add_new_user()? */ + pr_debug("username : %s, ngroups : %d\n", name, ngroups); + for (i = 0; i < ngroups; i++) { + struct group *ge = getgrgid(sgid[i]); + if (ge) + pr_debug("gid : %d(%s)\n", sgid[i], ge->gr_name); + } + +out: return user; } @@ -340,6 +369,8 @@ static void __handle_login_request(struct ksmbd_login_response *resp, sizeof(resp->account))) resp->status = KSMBD_USER_FLAG_INVALID; } + + resp->ngroups = user->ngroups; } int usm_handle_login_request(struct ksmbd_login_request *req, -- 2.46.2 ````

Unfortunately, for my approach, the change I made to ipc_validate_msg() means that new ksmbd-tools would not be compatible with old ksmbd when using Kerberos 5 authentication. (As a side note, looking at commit f27e0ed, I can't say I understand the motivation.)

[namjaejeon]

As a side note, looking at commit https://github.com/namjaejeon/ksmbd/commit/f27e0edb0c8d8cae2b645093026ebac7d3bf9e96, I can't say I understand the motivation

I have forwarded a mail to your gmail. There is vulnerability report regarding to this. and proved memory overrun and slab out of bounds. Let me know if you don't understand motivation after reading a mail. Thanks.

[namjaejeon]

@atheik the patches look good. user struct is allocated with kmalloc in ksmbd_alloc_user. So we need to initialize ->ngroup and ->sgid as zero or NULL. I will directly update it.

[namjaejeon]

Unfortunately, for my approach, the change I made to ipc_validate_msg() means that new ksmbd-tools would not be compatible with old ksmbd when using Kerberos 5 authentication

That was why I have tried to add new login response extension. We need to add login response validation for ->payload of login_response struct in ksmbd module. If it is added, It will make compatibility issue on normal as well as Kerberos 5 authentication.

[namjaejeon]

If it is added, It will make compatibility issue on normal as well as Kerberos 5 authentication.

Please Ignore. it is not true.

[namjaejeon]

@atheik If you are agree, I will mix your patch + login response extension.

[namjaejeon]

@atheik Please review them. https://github.com/namjaejeon/ksmbd/commit/46dcf5164c905aca7674d0017118d8b7538c4737 https://github.com/namjaejeon/ksmbd-tools/commit/ac9c98c1e8281c2740541e01f20a0de223471666

[atheik]

@namjaejeon,

You need to gid_t *sgid = NULL in new_ksmbd_user() like in my approach. Otherwise, g_free(sgid) frees uninitialized memory on the first iteration of the do-while loop.

What exactly is the point of mixing our approaches if ksmbd remains limited by KSMBD_MAX_GROUPS? Declarations of struct ksmbd_login_response_ext now differ between ksmbd and ksmbd-tools.

Kerberos 5 support is broken with this approach (Edit: these patches do not work without Kerberos 5 either):

[ksmbd 46dcf51 | ksmbd-tools ac9c98c + "encode_negTokenTarg() fix"] mount failure (click the arrow to expand)

cifs-utils mount command: ```sh su -- root -c 'mount -o sec=krb5,cruid=atte //pohjola.localdomain/issue200 /mnt' ``` ksmbd-tools log: ```` [ksmbd.mountd(worker)/17007]: INFO: Authenticated user `atte' [ksmbd.mountd(worker)/17007]: ERROR: nl_recv() failed, check dmesg, error: Input data out of range ```` cifs and ksmbd log: ```` [14770.471907] CIFS: Attempting to mount //pohjola.localdomain/issue200 [14770.557534] ksmbd: connect success: accepted new connection [14770.557632] ksmbd: RFC1002 header 248 bytes [14770.557653] ksmbd: no length check for command [14770.557656] ksmbd: SMB2 data length 0 offset 0 [14770.557659] ksmbd: SMB2 len 100 [14770.557661] ksmbd: client requested dialect 0x311 [14770.557663] ksmbd: selected \x02SMB 3.1.1 dialect [14770.557665] ksmbd: conn->dialect 0x311 [14770.557666] ksmbd: Received negotiate request [14770.557668] ksmbd: decoding 4 negotiate contexts [14770.557670] ksmbd: deassemble SMB2_PREAUTH_INTEGRITY_CAPABILITIES context [14770.557672] ksmbd: deassemble SMB2_ENCRYPTION_CAPABILITIES context [14770.557673] ksmbd: Cipher ID = 0x2 [14770.557674] ksmbd: deassemble SMB2_NETNAME_NEGOTIATE_CONTEXT_ID context [14770.557676] ksmbd: deassemble SMB2_POSIX_EXTENSIONS_AVAILABLE context [14770.557683] ksmbd: assemble SMB2_PREAUTH_INTEGRITY_CAPABILITIES context [14770.557686] ksmbd: assemble SMB2_ENCRYPTION_CAPABILITIES context [14770.557688] ksmbd: assemble SMB2_POSIX_EXTENSIONS_AVAILABLE context [14770.557689] ksmbd: negotiate context offset 224, count 3 [14770.557692] ksmbd: credits: requested[10] granted[1] total_granted[1] [14770.563279] ksmbd: RFC1002 header 881 bytes [14770.563298] ksmbd: SMB2 data length 793 offset 88 [14770.563302] ksmbd: SMB2 len 881 [14770.563305] ksmbd: Received request for session setup [14772.801020] ksmbd: dumping generated AES encryption keys [14772.801027] ksmbd: Cipher type 2 [14772.801030] ksmbd: Session Id 1 [14772.801033] ksmbd: Session Key 99 48 00 4a 7d 27 d3 19 2a 29 4b 2e 79 62 b0 5f [14772.801036] ksmbd: ServerIn Key 9b be 93 d9 bd 77 39 f2 70 83 fd 76 08 8b 88 c0 [14772.801039] ksmbd: ServerOut Key d2 6e 20 b0 06 ae 40 63 a9 83 07 df 66 0f c1 c2 [14772.801054] ksmbd: dumping generated AES signing keys [14772.801055] ksmbd: Session Id 1 [14772.801056] ksmbd: Session Key 99 48 00 4a 7d 27 d3 19 2a 29 4b 2e 79 62 b0 5f [14772.801057] ksmbd: Signing Key 19 16 b2 20 52 de 34 b3 76 fb 07 32 d9 e8 20 9c [14772.801060] ksmbd: credits: requested[130] granted[130] total_granted[130] [14772.801332] ksmbd: RFC1002 header 124 bytes [14772.801344] ksmbd: skip to check tree connect request [14772.801346] ksmbd: SMB2 data length 52 offset 72 [14772.801348] ksmbd: SMB2 len 124 [14772.801358] ksmbd: tree connect request for tree ipc$ treename \\pohjola.localdomain\IPC$ [14772.801368] ksmbd: credits: requested[64] granted[64] total_granted[193] [14772.801521] CIFS: VFS: BAD_NETWORK_NAME: \\pohjola.localdomain\IPC$ [14772.801527] CIFS: VFS: \\pohjola.localdomain failed to connect to IPC (rc=-2) [14772.801537] CIFS: VFS: session 000000003a6ac4bf has no tcon available for a dfs referral request [14772.801592] ksmbd: RFC1002 header 132 bytes [14772.801606] ksmbd: skip to check tree connect request [14772.801607] ksmbd: SMB2 data length 60 offset 72 [14772.801608] ksmbd: SMB2 len 132 [14772.801614] ksmbd: tree connect request for tree issue200 treename \\pohjola.localdomain\issue200 [14772.801622] ksmbd: credits: requested[64] granted[64] total_granted[256] [14772.801771] CIFS: VFS: BAD_NETWORK_NAME: \\pohjola.localdomain\issue200 [14772.801819] ksmbd: RFC1002 header 68 bytes [14772.801838] ksmbd: skip to check tree connect request [14772.801840] ksmbd: SMB2 len 68 [14772.801842] ksmbd: request [14772.801849] ksmbd: credits: requested[10] granted[10] total_granted[265] [14772.801946] CIFS: VFS: cifs_mount failed w/return code = -2 ````

This message was also present with my approach:

[ksmbd 46dcf51] field-spanning write (click the arrow to expand)

```` [14656.776585] ------------[ cut here ]------------ [14656.776590] memcpy: detected field-spanning write (size 793) of single field "req->spnego_blob" at /home/atte/ksmbd/transport_ipc.c:626 (size 0) [14656.776622] WARNING: CPU: 2 PID: 13311 at /home/atte/ksmbd/transport_ipc.c:626 ksmbd_ipc_spnego_authen_request+0x10c/0x120 [ksmbd] [14656.776667] Modules linked in: ksmbd(OE) crc32_generic libdes cmac nls_utf8 cifs cifs_arc4 nls_ucs2_utils cifs_md4 dns_resolver netfs rdma_cm iw_cm ib_cm ib_core snd_seq_dummy snd_hrtimer snd_seq snd_seq_device ccm ip6t_REJECT nf_reject_ipv6 ip6table_filter ip6_tables ipt_REJECT nf_reject_ipv4 xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c xt_tcpudp iptable_filter btusb btrtl btintel btbcm btmtk mousedev rt2800usb rt2x00usb rt2800lib rt2x00lib mac80211 libarc4 cfg80211 ath3k bluetooth rfkill snd_hda_codec_via snd_hda_codec_generic snd_hda_codec_hdmi snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core r8169 snd_hwdep snd_pcm intel_powerclamp snd_timer realtek snd mdio_devres soundcore libphy coretemp kvm_intel iTCO_wdt intel_pmc_bxt kvm iTCO_vendor_support gpio_ich at24 pcspkr intel_cstate intel_uncore i2c_i801 i7core_edac i2c_smbus asus_atk0110 lpc_ich i2c_mux acpi_cpufreq mac_hid zram i2c_dev loop nfnetlink ip_tables x_tables ext4 crc32c_generic mbcache jbd2 xts dm_crypt [14656.776737] gf128mul crypto_simd cryptd cbc encrypted_keys trusted asn1_encoder tee dm_mod amdgpu amdxcp drm_buddy crc16 nouveau firewire_ohci crc32c_intel sha512_ssse3 drm_gpuvm ata_generic firewire_core sr_mod sha256_ssse3 hid_generic pata_acpi mxm_wmi sha1_ssse3 usbhid cdrom drm_exec crc_itu_t pata_jmicron gpu_sched radeon drm_ttm_helper ttm video wmi i2c_algo_bit drm_suballoc_helper drm_display_helper cec vfio_pci vfio_pci_core vfio_iommu_type1 vfio iommufd [14656.776770] Unloaded tainted modules: ksmbd(OE):3 [last unloaded: crc32_generic] [14656.776776] CPU: 2 UID: 0 PID: 13311 Comm: kworker/2:2 Tainted: G W OE 6.11.1-arch1-1 #1 5d189185854f2d35e5fd15c112844f59d15980c4 [14656.776781] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE [14656.776782] Hardware name: System manufacturer System Product Name/P7P55D, BIOS 2101 10/20/2011 [14656.776784] Workqueue: ksmbd-io handle_ksmbd_work [ksmbd] [14656.776811] RIP: 0010:ksmbd_ipc_spnego_authen_request+0x10c/0x120 [ksmbd] [14656.776839] Code: eb d8 80 3d 2f cb 01 00 00 75 ac 31 c9 48 c7 c2 e8 96 a2 c2 4c 89 ee 48 c7 c7 30 97 a2 c2 c6 05 13 cb 01 00 01 e8 54 b1 fb cc <0f> 0b eb 89 31 db eb b0 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 [14656.776841] RSP: 0018:ffffbb858f90bd50 EFLAGS: 00010286 [14656.776843] RAX: 0000000000000000 RBX: ffff9f2bfaaf200e RCX: 0000000000000027 [14656.776845] RDX: ffff9f2d73721a48 RSI: 0000000000000001 RDI: ffff9f2d73721a40 [14656.776847] RBP: ffff9f2bfaaf2000 R08: 0000000000000000 R09: ffffbb858f90bbd0 [14656.776848] R10: ffffffff916b3fe8 R11: 0000000000000003 R12: ffff9f2bfaaf585c [14656.776849] R13: 0000000000000319 R14: ffff9f2bb41d6000 R15: 0000000000000322 [14656.776851] FS: 0000000000000000(0000) GS:ffff9f2d73700000(0000) knlGS:0000000000000000 [14656.776853] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [14656.776855] CR2: 000079cf8e392568 CR3: 0000000156c22000 CR4: 00000000000006f0 [14656.776856] Call Trace: [14656.776859] [14656.776861] ? ksmbd_ipc_spnego_authen_request+0x10c/0x120 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.776888] ? __warn.cold+0x8e/0xe8 [14656.776892] ? ksmbd_ipc_spnego_authen_request+0x10c/0x120 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.776921] ? report_bug+0xff/0x140 [14656.776925] ? handle_bug+0x3c/0x80 [14656.776929] ? exc_invalid_op+0x17/0x70 [14656.776931] ? asm_exc_invalid_op+0x1a/0x20 [14656.776935] ? ksmbd_ipc_spnego_authen_request+0x10c/0x120 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.776963] ksmbd_krb5_authenticate+0x26/0x170 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.776985] smb2_sess_setup+0x666/0x1060 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.777012] ? ksmbd_smb2_check_message+0x3f8/0x690 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.777038] handle_ksmbd_work+0x16c/0x450 [ksmbd e05220d9a8e5112ceaa6f9b8ce5e75bdc7199f9d] [14656.777065] process_one_work+0x17e/0x330 [14656.777070] worker_thread+0x2ce/0x3f0 [14656.777074] ? __pfx_worker_thread+0x10/0x10 [14656.777077] kthread+0xd2/0x100 [14656.777081] ? __pfx_kthread+0x10/0x10 [14656.777085] ret_from_fork+0x34/0x50 [14656.777088] ? __pfx_kthread+0x10/0x10 [14656.777091] ret_from_fork_asm+0x1a/0x30 [14656.777097] [14656.777098] ---[ end trace 0000000000000000 ]--- ````

[namjaejeon]

@atheik Oops sorry, I have given wrong link to you. I will give you a link again.

[namjaejeon]

@atheik Please review updated patches. and I have applied "[PATCH] ksmbd-tools: fix encode_negTokenTarg() return value" patch to work.ngroup branch.

https://github.com/namjaejeon/ksmbd/commit/5de47963d838822b1972b25fa195dd0d2e72b70b https://github.com/namjaejeon/ksmbd-tools/commit/02b7629257d9d02d5fdc26dcfd23adb5a2c81918

[namjaejeon]

@atheik

Regarding to field-spanning write warning, Please use this patch.

https://github.com/namjaejeon/ksmbd/commit/1bb58abdc4bf86c366949985dea1c960f5aa8d46

[atheik]

@namjaejeon,

The ksmbd 1bb58ab and ksmbd-tools 2dc8237 combination still hits ksmbd: SPNEGO_AUTHEN_REQUEST failure as per https://github.com/namjaejeon/ksmbd/issues/461#issuecomment-2391960206. (Edit: Nevermind. I had not fully run the exerciser.)

[atheik]

@namjaejeon,

With Kerberos 5, the ksmbd 1bb58ab and ksmbd-tools 78bef66 + "encode_negTokenTarg() fix" combination fails:

[ksmbd.mountd(worker)/31078]: INFO: Authenticated user `atte'
[ksmbd.mountd(worker)/31078]: ERROR: nl_recv() failed, check dmesg, error: Netlink message type is not supported

We need to check resp->login_response.status & KSMBD_USER_FLAG_EXTENSION in ksmbd_krb5_authenticate().

[namjaejeon]

@atheik Right:)

updated your point. and I have also added reserved space for future. https://github.com/namjaejeon/ksmbd/commit/be97dff012b5ba58650b4b82ba69e1d9db29add8 https://github.com/namjaejeon/ksmbd-tools/commit/e2fd5ec17c8f53cfbf5e8de51f7d910a9e70ccba

[namjaejeon]

@atheik

I have updated the patch as you pointed out. https://github.com/namjaejeon/ksmbd/commit/93f3446beca9648cf99db6413bba075c1be74b5e

[atheik]

@namjaejeon,

In what situation is gid_eq(GLOBAL_ROOT_GID, make_kgid(&init_user_ns, user->sgid[i])) true? In my testing, I think I only managed to hit the else-branch. Is it meaningful then that make_kgid(...) often ends up being called twice per iteration, i.e. is there a reason you do not have a single kgid_t kgid = make_kgid(...) before the if-else block?

Other than that, the patches look good to me. Thank you for taking the time to fix this!

[namjaejeon]

In what situation is gid_eq(GLOBAL_ROOT_GID, make_kgid(&init_user_ns, user->sgid[i])) true? In my testing, I think I only managed to hit the else-branch.

Ah, Can we set root group to supplementary groups ? I have referenced it with nfsd. I don't know nfsd doesn't allow root group in supplementary groups.

nfsd, auth.c

        } else if (flags & NFSEXP_ROOTSQUASH) {
                if (uid_eq(new->fsuid, GLOBAL_ROOT_UID))
                        new->fsuid = exp->ex_anon_uid;
                if (gid_eq(new->fsgid, GLOBAL_ROOT_GID))
                        new->fsgid = exp->ex_anon_gid;

                gi = groups_alloc(rqgi->ngroups);
                if (!gi)
                        goto oom;

                for (i = 0; i < rqgi->ngroups; i++) {
                        if (gid_eq(GLOBAL_ROOT_GID, rqgi->gid[i]))
                                gi->gid[i] = exp->ex_anon_gid;
                        else
                                gi->gid[i] = rqgi->gid[i];
                }

                /* Each thread allocates its own gi, no race */
                groups_sort(gi);
        } else {

[namjaejeon]

@atheik

Is it meaningful then that make_kgid(...) often ends up being called twice per iteration, i.e. is there a reason you do not have a single kgid_t kgid = make_kgid(...) before the if-else block?

I have updated the patch. Please see. Thanks for review! https://github.com/namjaejeon/ksmbd/commit/cc0af6a609c00072aea436f7939d1c3d30f2e76a

[atheik]

@namjaejeon,

Ah, Can we set root group to supplementary groups ?

I checked Samba's behavior and it has no such restriction:

Exerciser for supplementary group support (click the arrow to expand)

````sh # shut down the ksmbd server su -- root -c 'ksmbd.control -s' # create directory /tmp/issue461 as root su -- root -c 'mkdir /tmp/issue461' # allow writes to directory through group su -- root -c 'chmod g+w /tmp/issue461' # add a system group called issue461 su -- root -c 'groupadd issue461' # add system users called issue461{a,b} with primary group issue461 su -- root -c 'useradd -g issue461 issue461a' su -- root -c 'useradd -g issue461 issue461b' # add a system user called issue461c su -- root -c 'useradd issue461c' # add ksmbd users for system users issue461{a,b,c} ksmbd.adduser -C /tmp/issue461.conf -P /tmp/issue461.db -p mypass1 -a issue461a ksmbd.adduser -C /tmp/issue461.conf -P /tmp/issue461.db -p mypass2 -a issue461b ksmbd.adduser -C /tmp/issue461.conf -P /tmp/issue461.db -p mypass3 -a issue461c # add a ksmbd share called issue461 with path /tmp/issue461 ksmbd.addshare -C /tmp/issue461.conf -P /tmp/issue461.db -o 'path = /tmp/issue461' -a issue461 # allow read-write access ksmbd.addshare -C /tmp/issue461.conf -P /tmp/issue461.db -o 'read only = no' -u issue461 # run ksmbd and log to this terminal su -- root -c 'ksmbd.mountd -n2 -C /tmp/issue461.conf -P /tmp/issue461.db &' && sleep 1 # these should fail - creates directories /tmp/issue461/case{1,2,3} smbclient -U issue461a%mypass1 -c 'mkdir case1' //127.0.0.1/issue461 smbclient -U issue461b%mypass2 -c 'mkdir case2' //127.0.0.1/issue461 smbclient -U issue461c%mypass3 -c 'mkdir case3' //127.0.0.1/issue461 ## NT_STATUS_ACCESS_DENIED making remote directory \case1 ## NT_STATUS_ACCESS_DENIED making remote directory \case2 ## NT_STATUS_ACCESS_DENIED making remote directory \case3 # add system user issue461c to system group root su -- root -c 'usermod -aG root issue461c' su -- root -c 'ksmbd.control -r' && sleep 1 # this should fail - creates directory /tmp/issue/case4 smbclient -U issue461c%mypass3 -c 'mkdir case4' //127.0.0.1/issue461 ## NT_STATUS_ACCESS_DENIED making remote directory \case4 # change group of /tmp/issue461 to issue461 su -- root -c 'chgrp issue461 /tmp/issue461' su -- root -c 'ksmbd.control -r' && sleep 1 # these should succeed - creates directories /tmp/issue461/case{5,6} smbclient -U issue461a%mypass1 -c 'mkdir case5' //127.0.0.1/issue461 smbclient -U issue461b%mypass2 -c 'mkdir case6' //127.0.0.1/issue461 # this should fail - creates directory /tmp/issue/case7 smbclient -U issue461c%mypass3 -c 'mkdir case7' //127.0.0.1/issue461 ## NT_STATUS_ACCESS_DENIED making remote directory \case7 # add system user issue461c to system group issue461 su -- root -c 'usermod -aG issue461 issue461c' su -- root -c 'ksmbd.control -r' && sleep 1 # this should succeed - creates directory /tmp/issue461/case8 smbclient -U issue461c%mypass3 -c 'mkdir case8' //127.0.0.1/issue461 # shut down the ksmbd server su -- root -c 'ksmbd.control -s' # remove ksmbd configuration rm /tmp/issue461.conf /tmp/issue461.db su -- root -c 'rm -r /tmp/issue461' # remove system users issue461{a,b} su -- root -c 'userdel issue461a' su -- root -c 'userdel issue461b' # remove system group issue461 su -- root -c 'groupdel issue461' # remove system user issue461c su -- root -c 'userdel issue461c' ````

With this new exerciser, creating directory case4 fails in ksmbd but succeeds in Samba. The fix would be to remove the gid_eq(GLOBAL_ROOT_GID, ...) if-branch.

By the way, since we call getgrouplist() with e->pw_gid, we will also have the primary group in sgid. If sgid should only contain the supplementary groups, we can do this:

[DIFF] remove primary group from sgid (click the arrow to expand)

````diff diff --git a/tools/management/user.c b/tools/management/user.c index c711be5..cc37257 100644 --- a/tools/management/user.c +++ b/tools/management/user.c @@ -135,20 +135,17 @@ static struct ksmbd_user *new_ksmbd_user(char *name, char *pwd) g_free(user); return NULL; } - } while (getgrouplist(name, e->pw_gid, sgid, &ngroups) < 0); + } while (getgrouplist(name, KSMBD_SHARE_INVALID_GID, sgid, &ngroups) < 0); + + for (i = 0; i < ngroups; i++) + if (sgid[i] == KSMBD_SHARE_INVALID_GID) { + memmove(sgid + i, sgid + i + 1, + sizeof(gid_t) * (ngroups - i - 1)); + ngroups--; + } user->ngroups = ngroups; user->sgid = sgid; - - /* FIXME: too verbose -- move some of this to usm_add_new_user()? */ - pr_debug("username : %s, ngroups : %d\n", name, ngroups); - for (i = 0; i < ngroups; i++) { - struct group *gr = getgrgid(sgid[i]); - - if (gr != NULL) - pr_debug("gid : %d(%s)\n", sgid[i], gr->gr_name); - } - out: return user; } ````

Then, we could set KSMBD_USER_FLAG_EXTENSION only when login_response_payload_sz(...) > 0.

[namjaejeon]

@atheik

Done!, Thanks for your check and review!

https://github.com/namjaejeon/ksmbd/commit/7022bc4e6665e6326a9f5e54b385c1c76269ae32 https://github.com/namjaejeon/ksmbd-tools/commit/8139ae529802bc0e4bc4f74dcfb539eb2843885b

[namjaejeon]

@bol-van Can you confirm if your issue is fixed with ksmbd and ksmbd-tools #master branch ?

[bol-van]

Yes, it seems to work as expected. Thanks

[namjaejeon]

Thanks for your check:) Closed.