* updated ksmbd (cifsd) @ 2020-12-14 1:20 Steve French 2020-12-14 12:46 ` Namjae Jeon 2020-12-14 17:45 ` Stefan Metzmacher 0 siblings, 2 replies; 13+ messages in thread From: Steve French @ 2020-12-14 1:20 UTC (permalink / raw) To: Namjae Jeon; +Cc: samba-technical, CIFS I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next ontop of 5.10 kernel. Let me know if you see any problems. xfstest results (and recent improvements) running Linux cifs.ko->ksmbd look very promising. -- Thanks, Steve ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-14 1:20 updated ksmbd (cifsd) Steve French @ 2020-12-14 12:46 ` Namjae Jeon 2020-12-15 2:28 ` Namjae Jeon 2020-12-14 17:45 ` Stefan Metzmacher 1 sibling, 1 reply; 13+ messages in thread From: Namjae Jeon @ 2020-12-14 12:46 UTC (permalink / raw) To: Steve French; +Cc: Namjae Jeon, CIFS, samba-technical 2020-12-14 10:20 GMT+09:00, Steve French via samba-technical <samba-technical@lists.samba.org>: > I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next > ontop of 5.10 kernel. Let me know if you see any problems. xfstest > results (and recent improvements) running Linux cifs.ko->ksmbd look > very promising. With the review of Ronnie, I fixed one problem in previous patch. I have sent a pull request for this patch. And I am running the number of 118 tests of xfstests on rebased smb3-kernel(+ the patch in pull request) again. I will share the result as soon as it is finished. Thanks! > > > > -- > Thanks, > > Steve > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: updated ksmbd (cifsd) 2020-12-14 12:46 ` Namjae Jeon @ 2020-12-15 2:28 ` Namjae Jeon 0 siblings, 0 replies; 13+ messages in thread From: Namjae Jeon @ 2020-12-15 2:28 UTC (permalink / raw) To: 'Steve French' Cc: 'CIFS', 'samba-technical', 'Namjae Jeon' > > I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next > > ontop of 5.10 kernel. Let me know if you see any problems. xfstest > > results (and recent improvements) running Linux cifs.ko->ksmbd look > > very promising. > With the help of Ronnie, I fixed one problem in previous patch. I have sent > a pull request for this patch. And I have also checked number of 118 tests of > xfstests were passed on rebased smb3-kernel(+ the patch in pull request). I share the xfstests's result of 5.10 kernel+ksmbd(cifsd). ---------------------->8------------------ FSTYP -- cifs PLATFORM -- Linux/x86_64 linkinjeon-Samsung-DeskTop-System 5.10.0+ #1 SMP Mon Dec 14 10:55:27 KST 2020 MKFS_OPTIONS -- //10.88.103.87/homes MOUNT_OPTIONS -- -ousername=linkinjeon,password=1234,noperm,vers=3.11,mfsymlinks,nostrictsync //10.88.103.87/homes /mnt/test cifs/001 files ... 1s generic/001 files ... 15s generic/002 files ... 2s generic/005 files ... 2s generic/006 files ... 58s generic/007 files ... 46s generic/008 files ... 1s generic/011 files ... 56s generic/013 files ... 62s generic/014 files ... 12s generic/020 files ... 3s generic/023 files ... 2s generic/024 files ... 2s generic/028 files ... 6s generic/029 files ... 2s generic/030 files ... 1s generic/032 files ... 14s generic/033 files ... 1s generic/036 files ... 11s generic/037 files ... 9s generic/069 files ... 18s generic/070 files ... 39s generic/071 files ... 1s generic/074 files ... 98s generic/080 files ... 3s generic/084 files ... 6s generic/086 files ... 2s generic/095 files ... 6s generic/098 files ... 2s generic/100 files ... 39s generic/103 files ... 2s generic/109 files ... 28s generic/113 files ... 7s generic/117 files ... 38s generic/124 files ... 7s generic/125 files ... 62s generic/129 files ... 35s generic/130 files ... 4s generic/132 files ... 16s generic/133 files ... 36s generic/135 files ... 1s generic/141 files ... 1s generic/169 files ... 1s generic/198 files ... 1s generic/207 files ... 2s generic/208 files ... 200s generic/210 files ... 0s generic/211 files ... 0s generic/212 files ... 0s generic/214 files ... 1s generic/215 files ... 3s generic/221 files ... 2s generic/225 files ... 9s generic/228 files ... 1s generic/236 files ... 2s generic/239 files ... 31s generic/241 files ... 73s generic/245 files ... 0s generic/246 files ... 1s generic/247 files ... 20s generic/248 files ... 0s generic/249 files ... 2s generic/257 files ... 2s generic/258 files ... 1s generic/286 files ... 6s generic/308 files ... 1s generic/309 files ... 2s generic/310 files ... 101s generic/313 files ... 4s generic/315 files ... 1s generic/316 files ... 2s generic/337 files ... 0s generic/339 files ... 4s generic/340 files ... 6s generic/344 files ... 11s generic/345 files ... 11s generic/346 files ... 25s generic/349 files ... 4s generic/350 files ... 3s generic/354 files ... 9s generic/360 files ... 0s generic/377 files ... 1s generic/391 files ... 10s generic/393 files ... 3s generic/394 files ... 2s generic/406 files ... 2s generic/412 files ... 1s generic/420 files ... 1s generic/422 files ... 2s generic/428 files ... 1s generic/432 files ... 1s generic/433 files ... 1s generic/436 files ... 2s generic/437 files ... 2s generic/438 files ... 69s generic/439 files ... 1s generic/443 files ... 1s generic/445 files ... 2s generic/446 files ... 14s generic/448 files ... 3s generic/451 files ... 31s generic/452 files ... 1s generic/454 files ... 2s generic/460 files ... 9s generic/464 files ... 54s generic/465 files ... 3s generic/476 files ... 18864s generic/490 files ... 3s generic/504 files ... 0s generic/523 files ... 118s generic/524 files ... 6s generic/533 files ... 2s generic/539 files ... 2s generic/551 files ... 7923s generic/567 files ... 4s generic/568 files ... 1s generic/590 files ... 106s generic/591 files ... 1s Ran: cifs/001 generic/001 generic/002 generic/005 generic/006 generic/007 generic/008 generic/011 generic/013 generic/014 generic/020 generic/023 generic/024 generic/028 generic/029 generic/030 generic/032 generic/033 generic/036 generic/037 generic/069 generic/070 generic/071 generic/074 generic/080 generic/084 generic/086 generic/095 generic/098 generic/100 generic/103 generic/109 generic/113 generic/117 generic/124 generic/125 generic/129 generic/130 generic/132 generic/133 generic/135 generic/141 generic/169 generic/198 generic/207 generic/208 generic/210 generic/211 generic/212 generic/214 generic/215 generic/221 generic/225 generic/228 generic/236 generic/239 generic/241 generic/245 generic/246 generic/247 generic/248 generic/249 generic/257 generic/258 generic/286 generic/308 generic/309 generic/310 generic/313 generic/315 generic/316 generic/337 generic/339 generic/340 generic/344 generic/345 generic/346 generic/349 generic/350 generic/354 generic/360 generic/377 generic/391 generic/393 generic/394 generic/406 generic/412 generic/420 generic/422 generic/428 generic/432 generic/433 generic/436 generic/437 generic/438 generic/439 generic/443 generic/445 generic/446 generic/448 generic/451 generic/452 generic/454 generic/460 generic/464 generic/465 generic/476 generic/490 generic/504 generic/523 generic/524 generic/533 generic/539 generic/551 generic/567 generic/568 generic/590 generic/591 Passed all 118 tests ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-14 1:20 updated ksmbd (cifsd) Steve French 2020-12-14 12:46 ` Namjae Jeon @ 2020-12-14 17:45 ` Stefan Metzmacher 2020-12-14 18:48 ` Jeremy Allison 2020-12-15 2:28 ` Namjae Jeon 1 sibling, 2 replies; 13+ messages in thread From: Stefan Metzmacher @ 2020-12-14 17:45 UTC (permalink / raw) To: Steve French, Namjae Jeon; +Cc: CIFS, samba-technical [-- Attachment #1.1: Type: text/plain, Size: 1478 bytes --] Am 14.12.20 um 02:20 schrieb Steve French via samba-technical: > I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next > ontop of 5.10 kernel. Let me know if you see any problems. xfstest > results (and recent improvements) running Linux cifs.ko->ksmbd look > very promising. I just looked briefly, but I'm wondering about a few things: 1. The xattr's to store additional meta data are not compatible with Samba's way of storing things: https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl In order to make it possible to use the same filesystem with both servers it would be great if the well established way used in Samba would be used as well. 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ? 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info is kept unchanged, I guess at least the groups array should be set to be empty. 4. What is work->saved_cred_level used for in ksmbd_override_fsids()? It seems to be unused and adds a lot of complexity. 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools instead of https://github.com/cifsd-team/ksmbd-tools 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? I think the behavior should be enforced without a switch. These are just some initial thoughts. metze [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-14 17:45 ` Stefan Metzmacher @ 2020-12-14 18:48 ` Jeremy Allison 2020-12-15 2:29 ` Namjae Jeon 2020-12-15 2:28 ` Namjae Jeon 1 sibling, 1 reply; 13+ messages in thread From: Jeremy Allison @ 2020-12-14 18:48 UTC (permalink / raw) To: Stefan Metzmacher; +Cc: Steve French, Namjae Jeon, CIFS, samba-technical On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote: >Am 14.12.20 um 02:20 schrieb Steve French via samba-technical: >> I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next >> ontop of 5.10 kernel. Let me know if you see any problems. xfstest >> results (and recent improvements) running Linux cifs.ko->ksmbd look >> very promising. > >I just looked briefly, but I'm wondering about a few things: > >1. The xattr's to store additional meta data are not compatible with > Samba's way of storing things: > https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl > > In order to make it possible to use the same filesystem with both servers > it would be great if the well established way used in Samba would be used > as well. A thousand times this ! If cifs.ko->ksmbd adds a differnt way of storing the extra meta-data that is incompatible with Samba this would be a disaster for users. Please fix this before proposing any merge. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: updated ksmbd (cifsd) 2020-12-14 18:48 ` Jeremy Allison @ 2020-12-15 2:29 ` Namjae Jeon 2020-12-15 4:13 ` Jeremy Allison 0 siblings, 1 reply; 13+ messages in thread From: Namjae Jeon @ 2020-12-15 2:29 UTC (permalink / raw) To: 'Jeremy Allison', 'Stefan Metzmacher' Cc: 'Steve French', 'CIFS', 'samba-technical' > On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote: > >Am 14.12.20 um 02:20 schrieb Steve French via samba-technical: > >> I just rebased https://protect2.fireeye.com/v1/url?k=e100f21c-be9bcb17-e1017953-002590f5b904- > f00629b46b3afee4&q=1&e=6fc8b980-0fd2-4e4d-a9dc- > 9ea15e482833&u=https%3A%2F%2Fgithub.com%2Fsmfrench%2Fsmb3-kernel%2Ftree%2Fcifsd-for-next > >> ontop of 5.10 kernel. Let me know if you see any problems. xfstest > >> results (and recent improvements) running Linux cifs.ko->ksmbd look > >> very promising. > > > >I just looked briefly, but I'm wondering about a few things: > > > >1. The xattr's to store additional meta data are not compatible with > > Samba's way of storing things: > > > >https://protect2.fireeye.com/v1/url?k=fbb13e03-a42a0708-fbb0b54c-002590 > >f5b904-f4288e37b0eb9ae8&q=1&e=6fc8b980-0fd2-4e4d-a9dc-9ea15e482833&u=ht > >tps%3A%2F%2Fgit.samba.org%2F%3Fp%3Dsamba.git%3Ba%3Dblob%3Bf%3Dlibrpc%2F > >idl%2Fxattr.idl > > > > In order to make it possible to use the same filesystem with both servers > > it would be great if the well established way used in Samba would be used > > as well. > > A thousand times this ! If cifs.ko->ksmbd adds a differnt way of storing the extra meta-data that is > incompatible with Samba this would be a disaster for users. > > Please fix this before proposing any merge. You said that samba can handle it even if ksmbd has own extra metadata format. I didn't think it was necessary to what you said. If we have to do this, I think it is not too late to work after sending ksmbd to linux-next first. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-15 2:29 ` Namjae Jeon @ 2020-12-15 4:13 ` Jeremy Allison 0 siblings, 0 replies; 13+ messages in thread From: Jeremy Allison @ 2020-12-15 4:13 UTC (permalink / raw) To: Namjae Jeon Cc: 'Stefan Metzmacher', 'Steve French', 'samba-technical', 'CIFS' On Tue, Dec 15, 2020 at 11:29:01AM +0900, Namjae Jeon via samba-technical wrote: > >> On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote: >> >Am 14.12.20 um 02:20 schrieb Steve French via samba-technical: >> >> I just rebased https://protect2.fireeye.com/v1/url?k=e100f21c-be9bcb17-e1017953-002590f5b904- >> f00629b46b3afee4&q=1&e=6fc8b980-0fd2-4e4d-a9dc- >> 9ea15e482833&u=https%3A%2F%2Fgithub.com%2Fsmfrench%2Fsmb3-kernel%2Ftree%2Fcifsd-for-next >> >> ontop of 5.10 kernel. Let me know if you see any problems. xfstest >> >> results (and recent improvements) running Linux cifs.ko->ksmbd look >> >> very promising. >> > >> >I just looked briefly, but I'm wondering about a few things: >> > >> >1. The xattr's to store additional meta data are not compatible with >> > Samba's way of storing things: >> > >> >https://protect2.fireeye.com/v1/url?k=fbb13e03-a42a0708-fbb0b54c-002590 >> >f5b904-f4288e37b0eb9ae8&q=1&e=6fc8b980-0fd2-4e4d-a9dc-9ea15e482833&u=ht >> >tps%3A%2F%2Fgit.samba.org%2F%3Fp%3Dsamba.git%3Ba%3Dblob%3Bf%3Dlibrpc%2F >> >idl%2Fxattr.idl >> > >> > In order to make it possible to use the same filesystem with both servers >> > it would be great if the well established way used in Samba would be used >> > as well. >> >> A thousand times this ! If cifs.ko->ksmbd adds a differnt way of storing the extra meta-data that is >> incompatible with Samba this would be a disaster for users. >> >> Please fix this before proposing any merge. >You said that samba can handle it even if ksmbd has own extra metadata format. I didn't think it was >necessary to what you said. If we have to do this, I think it is not too late to work after sending >ksmbd to linux-next first. Yes, Samba could add an adaptor to read the ksmbd metadata format, but then files written by Samba would have metadata unreadable by ksmbd. As the Samba metadata format is widely used then I don't see the purpose of creating a new format for this. It's needless incompatibility. Please fix this before submitting anywhere. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: updated ksmbd (cifsd) 2020-12-14 17:45 ` Stefan Metzmacher 2020-12-14 18:48 ` Jeremy Allison @ 2020-12-15 2:28 ` Namjae Jeon 2020-12-15 14:29 ` Stefan Metzmacher 1 sibling, 1 reply; 13+ messages in thread From: Namjae Jeon @ 2020-12-15 2:28 UTC (permalink / raw) To: 'Stefan Metzmacher' Cc: 'CIFS', 'samba-technical', 'Sergey Senozhatsky', 'Hyunchul Lee', 'Steve French' > I just looked briefly, but I'm wondering about a few things: > > 1. The xattr's to store additional meta data are not compatible with > Samba's way of storing things: > https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl > > In order to make it possible to use the same filesystem with both servers > it would be great if the well established way used in Samba would be used > as well. Yes, Maybe... I will check it after sending ksmbd to linux-next. > 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ? Because of smb2.acls.GENERIC failure. TESTING FILE GENERIC BITS get the original sd Testing generic bits 0x00000000 time: 2020-12-15 00:00:37.940992 failure: GENERIC [ (../../source4/torture/smb2/acls.c:439) Incorrect status NT_STATUS_SHARING_VIOLATION - should be NT_STATUS_OK I really don't understand this test. This testcase expect that FILE_DELETE is set in response if desired access of smb2 open is FILE_MAXIMAL_ACCESS. I don't know why smb2 open should be allowed although FILE_SHARE_DELETE is not set in previous open in this test. Can you give me a hint ? > 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info > is kept unchanged, I guess at least the groups array should be set to be empty. Yes, We need to handle the groups list. Will fix it. > 4. What is work->saved_cred_level used for in ksmbd_override_fsids()? > It seems to be unused and adds a lot of complexity. ksmbd_override_fsids could be called recursively. work->saved_cred_level prevents ksmbd from overriding fs[g|u]id again. > 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools > instead of https://github.com/cifsd-team/ksmbd-tools Okay. Will update it. > 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? > I think the behavior should be enforced without a switch. I can make it default yes. Can you explain more why it should be enforced ? > These are just some initial thoughts. Thanks for sharing your review! > > metze ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-15 2:28 ` Namjae Jeon @ 2020-12-15 14:29 ` Stefan Metzmacher 2020-12-16 3:24 ` Sergey Senozhatsky 2020-12-16 8:50 ` Namjae Jeon 0 siblings, 2 replies; 13+ messages in thread From: Stefan Metzmacher @ 2020-12-15 14:29 UTC (permalink / raw) To: Namjae Jeon Cc: 'CIFS', 'Steve French', 'samba-technical', 'Hyunchul Lee', 'Sergey Senozhatsky' [-- Attachment #1.1: Type: text/plain, Size: 2761 bytes --] Am 15.12.20 um 03:28 schrieb Namjae Jeon via samba-technical: > >> I just looked briefly, but I'm wondering about a few things: >> >> 1. The xattr's to store additional meta data are not compatible with >> Samba's way of storing things: >> https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl >> >> In order to make it possible to use the same filesystem with both servers >> it would be great if the well established way used in Samba would be used >> as well. > Yes, Maybe... I will check it after sending ksmbd to linux-next. > >> 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ? > Because of smb2.acls.GENERIC failure. > > TESTING FILE GENERIC BITS > get the original sd > Testing generic bits 0x00000000 > time: 2020-12-15 00:00:37.940992 > failure: GENERIC [ > (../../source4/torture/smb2/acls.c:439) Incorrect status NT_STATUS_SHARING_VIOLATION - should be NT_STATUS_OK > > I really don't understand this test. This testcase expect that FILE_DELETE is set in response if desired access > of smb2 open is FILE_MAXIMAL_ACCESS. > I don't know why smb2 open should be allowed although FILE_SHARE_DELETE is not set in previous open in this test. > Can you give me a hint ? As far as I can see the test assumes the user has SeRestorePrivilege, with that SEC_FLAG_MAXIMUM_ALLOWED will add FILE_DELETE, see https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/8ada5fbe-db4e-49fd-aef6-20d54b748e40 >> 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info >> is kept unchanged, I guess at least the groups array should be set to be empty. > Yes, We need to handle the groups list. Will fix it. > >> 4. What is work->saved_cred_level used for in ksmbd_override_fsids()? >> It seems to be unused and adds a lot of complexity. > ksmbd_override_fsids could be called recursively. > work->saved_cred_level prevents ksmbd from overriding fs[g|u]id again. But that will always be on the same session/share combination? >> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools >> instead of https://github.com/cifsd-team/ksmbd-tools > Okay. Will update it. Thanks! >> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? >> I think the behavior should be enforced without a switch. > I can make it default yes. Can you explain more why it should be enforced ? Why should an unprivileged user ever be able to start the server? Wouldn't that be a massive security problem as that user would provide the share definitions and users and controls what ksmbd_override_fsids() will use? metze [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-15 14:29 ` Stefan Metzmacher @ 2020-12-16 3:24 ` Sergey Senozhatsky 2020-12-16 4:21 ` Sergey Senozhatsky 2020-12-16 8:50 ` Namjae Jeon 1 sibling, 1 reply; 13+ messages in thread From: Sergey Senozhatsky @ 2020-12-16 3:24 UTC (permalink / raw) To: Stefan Metzmacher Cc: Namjae Jeon, 'CIFS', 'Steve French', 'samba-technical', 'Hyunchul Lee', 'Sergey Senozhatsky' On (20/12/15 15:29), Stefan Metzmacher wrote: > >> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? > >> I think the behavior should be enforced without a switch. > > I can make it default yes. Can you explain more why it should be enforced ? > > Why should an unprivileged user ever be able to start the server? > Wouldn't that be a massive security problem as that user would provide > the share definitions and users and controls what ksmbd_override_fsids() will use? The idea was that user-space needs to have its own user:group (e.g. CIFSD:CIFSD). And smb.conf and password file should not be readable by anyone who's not from CIFSD:CIFSD - similar to how .ssh/config is 0700 on any reasonably configured system. The massive security problem here is that the server runs in the kernel. So I don't always see why people want to also run user-space (which serves RPC calls, and technically can be tricked to do something that it was not intended to do) under root - wouldn't this just increases the attack surface? -ss ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-16 3:24 ` Sergey Senozhatsky @ 2020-12-16 4:21 ` Sergey Senozhatsky 2020-12-17 3:29 ` Sergey Senozhatsky 0 siblings, 1 reply; 13+ messages in thread From: Sergey Senozhatsky @ 2020-12-16 4:21 UTC (permalink / raw) To: Stefan Metzmacher, Namjae Jeon Cc: 'CIFS', 'Steve French', 'samba-technical', 'Hyunchul Lee', Sergey Senozhatsky On (20/12/16 12:24), Sergey Senozhatsky wrote: > On (20/12/15 15:29), Stefan Metzmacher wrote: > > >> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? > > >> I think the behavior should be enforced without a switch. > > > I can make it default yes. Can you explain more why it should be enforced ? > > > > Why should an unprivileged user ever be able to start the server? > > Wouldn't that be a massive security problem as that user would provide > > the share definitions and users and controls what ksmbd_override_fsids() will use? > > The idea was that user-space needs to have its own user:group > (e.g. CIFSD:CIFSD). And smb.conf and password file should not > be readable by anyone who's not from CIFSD:CIFSD - similar to > how .ssh/config is 0700 on any reasonably configured system. > > The massive security problem here is that the server runs in > the kernel. So I don't always see why people want to also run > user-space (which serves RPC calls, and technically can be > tricked to do something that it was not intended to do) under > root - wouldn't this just increases the attack surface? So SMB_SERVER_CHECK_CAP_NET_ADMIN enforces the "user-space must be a privileged process" policy. Even CAP_NET_ADMIN is too huge, not to mention that _probably_ this CAP requirement means that people will just "sudo cifsd". One way or another a malformed RPC request can do quite a bit of damage to the system, because user-space runs with the CAPs it doesn't really need. It would be better to enforce a different policy, IMHO. Something like: groupadd ... CIFSD_GROUP useradd -g CIFSD_GID -p CIFSD_PASSWORD CIFSD_LOGIN chmod 0700 smb.conf and password db chown CIFSD_LOGIN:CIFSD_GROUP smb.conf and password db And perhaps we need to add some checks to the user-space cifsd: make sure that smb.conf and password db are 0700 + some more. -ss ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: updated ksmbd (cifsd) 2020-12-16 4:21 ` Sergey Senozhatsky @ 2020-12-17 3:29 ` Sergey Senozhatsky 0 siblings, 0 replies; 13+ messages in thread From: Sergey Senozhatsky @ 2020-12-17 3:29 UTC (permalink / raw) To: Stefan Metzmacher, Namjae Jeon Cc: 'CIFS', 'Steve French', 'samba-technical', 'Hyunchul Lee', Sergey Senozhatsky On (20/12/16 13:21), Sergey Senozhatsky wrote: > So SMB_SERVER_CHECK_CAP_NET_ADMIN enforces the "user-space must > be a privileged process" policy. Even CAP_NET_ADMIN is too huge, > not to mention that _probably_ this CAP requirement means that > people will just "sudo cifsd". One way or another a malformed > RPC request can do quite a bit of damage to the system, because > user-space runs with the CAPs it doesn't really need. > > It would be better to enforce a different policy, IMHO. > Something like: > > groupadd ... CIFSD_GROUP > useradd -g CIFSD_GID -p CIFSD_PASSWORD CIFSD_LOGIN > chmod 0700 smb.conf and password db > chown CIFSD_LOGIN:CIFSD_GROUP smb.conf and password db Just a sketch. Completely untested (wasn't even compile tested). Merely an idea. This removes the requirement for user-space to run as a privileged process. Because "let's grant some random user-space daemon administrative privileges" most likely doesn't improve security of the system. So the idea is to provide CIFSD administrator UID and GID during kcifsd modprobe and then in IPC handlers check if the app issuing a netlink syscall has the same UID and GID as the CIFSD administrator or not. This untested, sorry. Just checking the idea. --- diff --git a/fs/cifsd/Kconfig b/fs/cifsd/Kconfig index e12616cf8477..9f221a37223d 100644 --- a/fs/cifsd/Kconfig +++ b/fs/cifsd/Kconfig @@ -50,14 +50,6 @@ config SMB_SERVER_SMBDIRECT SMB Direct allows transferring SMB packets over RDMA. If unsure, say N. -config SMB_SERVER_CHECK_CAP_NET_ADMIN - bool "Enable check network administration capability" - depends on SMB_SERVER - default n - - help - Prevent unprivileged processes to start the cifsd kernel server. - config SMB_SERVER_KERBEROS5 bool "Support for Kerberos 5" depends on SMB_SERVER diff --git a/fs/cifsd/server.c b/fs/cifsd/server.c index b9e114f8a5d2..dbbdb3503b0d 100644 --- a/fs/cifsd/server.c +++ b/fs/cifsd/server.c @@ -25,6 +25,7 @@ int ksmbd_debug_types; +static int admin_uid = -1, admin_gid = -1; struct ksmbd_server_config server_conf; enum SERVER_CTRL_TYPE { @@ -335,6 +336,26 @@ static void server_conf_free(void) } } +static int server_admin_cred_init(void) +{ + if (admin_uid == -1 || admin_gid == -1) + return 0; + + server_conf.admin_cred = kmalloc(sizeof(struct admin_cred), GFP_KERNEL); + if (!server_conf.admin_cred) + return -ENOMEM; + + server_conf.admin_cred.uid = make_kuid(&init_user_ns, admin_uid); + server_conf.admin_cred.gid = make_kgid(&init_user_ns, admin_gid); + if (!(uid_validserver_conf.admin_cred.uid() && + gid_valid(server_conf.admin_cred.gid))) { + kfree(server_conf.admin_cred); + server_conf.admin_cred = NULL; + return -EINVAL; + } + return 0; +} + static int server_conf_init(void) { WRITE_ONCE(server_conf.state, SERVER_STATE_STARTING_UP); @@ -343,10 +364,9 @@ static int server_conf_init(void) server_conf.max_protocol = ksmbd_max_protocol(); server_conf.auth_mechs = KSMBD_AUTH_NTLMSSP; #ifdef CONFIG_SMB_SERVER_KERBEROS5 - server_conf.auth_mechs |= KSMBD_AUTH_KRB5 | - KSMBD_AUTH_MSKRB5; + server_conf.auth_mechs |= KSMBD_AUTH_KRB5 | KSMBD_AUTH_MSKRB5; #endif - return 0; + return server_admin_cred_init(); } static void server_ctrl_handle_init(struct server_ctrl_struct *ctrl) @@ -418,6 +438,21 @@ int server_queue_ctrl_reset_work(void) return __queue_ctrl_work(SERVER_CTRL_TYPE_RESET); } +int server_validate_admin_cred(void) +{ + if (admin_uid == -1 || admin_gid == -1) + return 0; + + /* We couldn't init server admin UID/GID */ + if (!server_conf.admin_cred) + return -EINVAL; + + if (uid_eq(task_uid(current), server_conf.admin_cred.uid) && + gid_eq(task_gid(current), server_conf.admin_cred.gid)) + return 0; + return -EINVAL; +} + static ssize_t stats_show(struct class *class, struct class_attribute *attr, char *buf) @@ -614,6 +649,11 @@ static void __exit ksmbd_server_exit(void) ksmbd_release_inode_hash(); } +module_param(admin_uid, int, 0); +MODULE_PARM_DESC(admin_uid, "ksmb administrator user id"); +module_param(admin_gid, int, 0); +MODULE_PARM_DESC(admin_gid, "ksmb administrator group id"); + MODULE_AUTHOR("Namjae Jeon <linkinjeon@kernel.org>"); MODULE_VERSION(KSMBD_VERSION); MODULE_DESCRIPTION("Linux kernel CIFS/SMB SERVER"); diff --git a/fs/cifsd/server.h b/fs/cifsd/server.h index 7b2f6318fcff..ed0249061470 100644 --- a/fs/cifsd/server.h +++ b/fs/cifsd/server.h @@ -19,6 +19,11 @@ extern int ksmbd_debugging; +struct admin_cred { + kuid_t uid; + kgid_t gid; +}; + struct ksmbd_server_config { unsigned int flags; unsigned int state; @@ -34,6 +39,7 @@ struct ksmbd_server_config { struct smb_sid domain_sid; unsigned int auth_mechs; + struct admin_cred *admin_cred; char *conf[SERVER_CONF_WORK_GROUP + 1]; }; @@ -57,6 +63,7 @@ static inline int ksmbd_server_configurable(void) return READ_ONCE(server_conf.state) < SERVER_STATE_RESETTING; } +int server_verify_admin_cred(void); int server_queue_ctrl_init_work(void); int server_queue_ctrl_reset_work(void); #endif /* __SERVER_H__ */ diff --git a/fs/cifsd/transport_ipc.c b/fs/cifsd/transport_ipc.c index 5f24a1ed5c34..b2a42f6ce6e3 100644 --- a/fs/cifsd/transport_ipc.c +++ b/fs/cifsd/transport_ipc.c @@ -345,10 +345,8 @@ static int handle_startup_event(struct sk_buff *skb, struct genl_info *info) { int ret = 0; -#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN - if (!netlink_capable(skb, CAP_NET_ADMIN)) + if (server_validate_admin_cred()) return -EPERM; -#endif if (!ksmbd_ipc_validate_version(info)) return -EINVAL; @@ -401,10 +399,8 @@ static int handle_generic_event(struct sk_buff *skb, struct genl_info *info) int sz; int type = info->genlhdr->cmd; -#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN - if (!netlink_capable(skb, CAP_NET_ADMIN)) + if (server_validate_admin_cred()) return -EPERM; -#endif if (type >= KSMBD_EVENT_MAX) { WARN_ON(1); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: updated ksmbd (cifsd) 2020-12-15 14:29 ` Stefan Metzmacher 2020-12-16 3:24 ` Sergey Senozhatsky @ 2020-12-16 8:50 ` Namjae Jeon 1 sibling, 0 replies; 13+ messages in thread From: Namjae Jeon @ 2020-12-16 8:50 UTC (permalink / raw) To: 'Stefan Metzmacher' Cc: 'CIFS', 'Steve French', 'samba-technical', 'Hyunchul Lee', 'Sergey Senozhatsky' > >> 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ? > > Because of smb2.acls.GENERIC failure. > > > > TESTING FILE GENERIC BITS > > get the original sd > > Testing generic bits 0x00000000 > > time: 2020-12-15 00:00:37.940992 > > failure: GENERIC [ > > (../../source4/torture/smb2/acls.c:439) Incorrect status > > NT_STATUS_SHARING_VIOLATION - should be NT_STATUS_OK > > > > I really don't understand this test. This testcase expect that > > FILE_DELETE is set in response if desired access of smb2 open is FILE_MAXIMAL_ACCESS. > > I don't know why smb2 open should be allowed although FILE_SHARE_DELETE is not set in previous open > in this test. > > Can you give me a hint ? > > As far as I can see the test assumes the user has SeRestorePrivilege, with that > SEC_FLAG_MAXIMUM_ALLOWED will add FILE_DELETE, see https://protect2.fireeye.com/v1/url?k=3a9ae45d- > 6501dd47-3a9b6f12-000babff24ad-8398dba5a818cd4a&q=1&e=bdf5897b-3ecc-49dc-9105- > 2d6782854fcc&u=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fopenspecs%2Fwindows_protocols%2Fms- > fsa%2F8ada5fbe-db4e-49fd-aef6-20d54b748e40 The question I'm asking is how it can be opened with FILE DELETE that adding by SEC_FLAG_MAXIMUM_ALLOWED without FILE_SHARE_DELETE in 1st open. NT_STATUS_SHARING_VIOLATION error should be returned? but this test should be allowed to open. It test in the following sequences. - 1st smb2 open with NTCREATEX_SHARE_ACCESS_READ | NTCREATEX_SHARE_ACCESS_WRITE - SMB2 set info security(). - 2nd open with SEC_FLAG_MAXIMUM_ALLOWED(adding FILE DELETE) => NT_STATUS_SHARING_VIOLATION or NT_STATUS_OK ? > > >> 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info > >> is kept unchanged, I guess at least the groups array should be set to be empty. > > Yes, We need to handle the groups list. Will fix it. > > > >> 4. What is work->saved_cred_level used for in ksmbd_override_fsids()? > >> It seems to be unused and adds a lot of complexity. > > ksmbd_override_fsids could be called recursively. > > work->saved_cred_level prevents ksmbd from overriding fs[g|u]id again. > > But that will always be on the same session/share combination? Ah, ksmbd_override_fsids() has been recursively called to handle SMB1 requests. At present, SMB1 codes was removed in smb3_kernel tree, So we can remove work->saved_cred_level. Thanks for your review! > > >> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references > https://protect2.fireeye.com/v1/url?k=6f3cad54-30a7944e-6f3d261b-000babff24ad- > 32002aad36f8cca9&q=1&e=bdf5897b-3ecc-49dc-9105-2d6782854fcc&u=https%3A%2F%2Fgithub.com%2Fcifsd- > team%2Fcifsd-tools > >> instead of > >> https://protect2.fireeye.com/v1/url?k=cf0932a6-90920bbc-cf08b9e9-000b > >> abff24ad-ea69fcf05590fae2&q=1&e=bdf5897b-3ecc-49dc-9105-2d6782854fcc& > >> u=https%3A%2F%2Fgithub.com%2Fcifsd-team%2Fksmbd-tools > > Okay. Will update it. > > Thanks! > > >> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default? > >> I think the behavior should be enforced without a switch. > > I can make it default yes. Can you explain more why it should be enforced ? > > Why should an unprivileged user ever be able to start the server? > Wouldn't that be a massive security problem as that user would provide the share definitions and users > and controls what ksmbd_override_fsids() will use? > > metze ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-12-17 3:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-14 1:20 updated ksmbd (cifsd) Steve French 2020-12-14 12:46 ` Namjae Jeon 2020-12-15 2:28 ` Namjae Jeon 2020-12-14 17:45 ` Stefan Metzmacher 2020-12-14 18:48 ` Jeremy Allison 2020-12-15 2:29 ` Namjae Jeon 2020-12-15 4:13 ` Jeremy Allison 2020-12-15 2:28 ` Namjae Jeon 2020-12-15 14:29 ` Stefan Metzmacher 2020-12-16 3:24 ` Sergey Senozhatsky 2020-12-16 4:21 ` Sergey Senozhatsky 2020-12-17 3:29 ` Sergey Senozhatsky 2020-12-16 8:50 ` Namjae Jeon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.