A kernel daemon should not rely on the current thread, which is unknown and might be malicious. Before this security fix, ksmbd_override_fsids() didn't correctly override FS UID/GID which means that arbitrary user space threads could trick the kernel to impersonate arbitrary users or groups for file system access checks, leading to file system access bypass.
This was found while investigating truncate support for Landlock: https://lore.kernel.org/r/CAKYAXd8fpMJ7guizOjHgxEyyjoUwPsx3jLOPZP=wPYcbhkVXq...
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3") Cc: Hyunchul Lee hyc.lee@gmail.com Cc: Namjae Jeon linkinjeon@kernel.org Cc: Steve French smfrench@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Mickaël Salaün mic@digikod.net Link: https://lore.kernel.org/r/20220929100447.108468-1-mic@digikod.net --- fs/ksmbd/smb_common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c index 7f8ab14fb8ec..d96da872d70a 100644 --- a/fs/ksmbd/smb_common.c +++ b/fs/ksmbd/smb_common.c @@ -4,6 +4,8 @@ * Copyright (C) 2018 Namjae Jeon linkinjeon@kernel.org */
+#include <linux/user_namespace.h> + #include "smb_common.h" #include "server.h" #include "misc.h" @@ -625,8 +627,8 @@ int ksmbd_override_fsids(struct ksmbd_work *work) if (!cred) return -ENOMEM;
- cred->fsuid = make_kuid(current_user_ns(), uid); - cred->fsgid = make_kgid(current_user_ns(), gid); + cred->fsuid = make_kuid(&init_user_ns, uid); + cred->fsgid = make_kgid(&init_user_ns, gid);
gi = groups_alloc(0); if (!gi) {
base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
On Thu, Sep 29, 2022 at 12:04:47PM +0200, Mickaël Salaün wrote:
A kernel daemon should not rely on the current thread, which is unknown and might be malicious. Before this security fix, ksmbd_override_fsids() didn't correctly override FS UID/GID which means that arbitrary user space threads could trick the kernel to impersonate arbitrary users or groups for file system access checks, leading to file system access bypass.
This was found while investigating truncate support for Landlock: https://lore.kernel.org/r/CAKYAXd8fpMJ7guizOjHgxEyyjoUwPsx3jLOPZP=wPYcbhkVXq...
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3") Cc: Hyunchul Lee hyc.lee@gmail.com Cc: Namjae Jeon linkinjeon@kernel.org Cc: Steve French smfrench@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Mickaël Salaün mic@digikod.net Link: https://lore.kernel.org/r/20220929100447.108468-1-mic@digikod.net
I think this is ok. The alternative would probably be to somehow use a relevant userns when struct ksmbd_user is created when the session is established. But these are deeper ksmbd design questions. The fix proposed here itself seems good.
On 29/09/2022 13:37, Christian Brauner wrote:
On Thu, Sep 29, 2022 at 12:04:47PM +0200, Mickaël Salaün wrote:
A kernel daemon should not rely on the current thread, which is unknown and might be malicious. Before this security fix, ksmbd_override_fsids() didn't correctly override FS UID/GID which means that arbitrary user space threads could trick the kernel to impersonate arbitrary users or groups for file system access checks, leading to file system access bypass.
This was found while investigating truncate support for Landlock: https://lore.kernel.org/r/CAKYAXd8fpMJ7guizOjHgxEyyjoUwPsx3jLOPZP=wPYcbhkVXq...
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3") Cc: Hyunchul Lee hyc.lee@gmail.com Cc: Namjae Jeon linkinjeon@kernel.org Cc: Steve French smfrench@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Mickaël Salaün mic@digikod.net Link: https://lore.kernel.org/r/20220929100447.108468-1-mic@digikod.net
I think this is ok. The alternative would probably be to somehow use a relevant userns when struct ksmbd_user is created when the session is established. But these are deeper ksmbd design questions. The fix proposed here itself seems good.
That would be better indeed. I guess ksmbd works whenever the netlink peer is not in a user namespace with mapped UID/GID, but it should result in obvious access bugs otherwise (which is already the case anyway). It seems that the netlink peer must be trusted because it is the source of truth for account/user mapping anyway. This change fixes the more critical side of the issue and it should fit well for backports.
On Thu, Sep 29, 2022 at 02:18:43PM +0200, Mickaël Salaün wrote:
On 29/09/2022 13:37, Christian Brauner wrote:
On Thu, Sep 29, 2022 at 12:04:47PM +0200, Mickaël Salaün wrote:
A kernel daemon should not rely on the current thread, which is unknown and might be malicious. Before this security fix, ksmbd_override_fsids() didn't correctly override FS UID/GID which means that arbitrary user space threads could trick the kernel to impersonate arbitrary users or groups for file system access checks, leading to file system access bypass.
This was found while investigating truncate support for Landlock: https://lore.kernel.org/r/CAKYAXd8fpMJ7guizOjHgxEyyjoUwPsx3jLOPZP=wPYcbhkVXq...
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3") Cc: Hyunchul Lee hyc.lee@gmail.com Cc: Namjae Jeon linkinjeon@kernel.org Cc: Steve French smfrench@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Mickaël Salaün mic@digikod.net Link: https://lore.kernel.org/r/20220929100447.108468-1-mic@digikod.net
I think this is ok. The alternative would probably be to somehow use a relevant userns when struct ksmbd_user is created when the session is established. But these are deeper ksmbd design questions. The fix proposed here itself seems good.
That would be better indeed. I guess ksmbd works whenever the netlink peer is not in a user namespace with mapped UID/GID, but it should result in obvious access bugs otherwise (which is already the case anyway). It seems that the netlink peer must be trusted because it is the source of truth for account/user mapping anyway. This change fixes the more critical side of the issue and it should fit well for backports.
Sorry, I also forgot, Acked-by: Christian Brauner (Microsoft) brauner@kernel.org
2022-09-29 19:04 GMT+09:00, Mickaël Salaün mic@digikod.net:
A kernel daemon should not rely on the current thread, which is unknown and might be malicious. Before this security fix, ksmbd_override_fsids() didn't correctly override FS UID/GID which means that arbitrary user space threads could trick the kernel to impersonate arbitrary users or groups for file system access checks, leading to file system access bypass.
This was found while investigating truncate support for Landlock: https://lore.kernel.org/r/CAKYAXd8fpMJ7guizOjHgxEyyjoUwPsx3jLOPZP=wPYcbhkVXq...
Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3") Cc: Hyunchul Lee hyc.lee@gmail.com Cc: Namjae Jeon linkinjeon@kernel.org Cc: Steve French smfrench@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Mickaël Salaün mic@digikod.net Link: https://lore.kernel.org/r/20220929100447.108468-1-mic@digikod.net
Acked-by: Namjae Jeon linkinjeon@kernel.org
Thanks!
linux-stable-mirror@lists.linaro.org
-
Christian Brauner -
Mickaël Salaün -
Namjae Jeon