* [GIT PULL] ksmbd server security fixes @ 2021-10-02 4:14 Steve French 2021-10-03 0:52 ` pr-tracker-bot 0 siblings, 1 reply; 11+ messages in thread From: Steve French @ 2021-10-02 4:14 UTC (permalink / raw) To: Linus Torvalds; +Cc: LKML, CIFS Please pull the following changes since commit 5816b3e6577eaa676ceb00a848f0fd65fe2adc29: Linux 5.15-rc3 (2021-09-26 14:08:19 -0700) are available in the Git repository at: git://git.samba.org/ksmbd.git tags/5.15-rc3-ksmbd-fixes for you to fetch changes up to 87ffb310d5e8a441721a9d04dfa7c90cd9da3916: ksmbd: missing check for NULL in convert_to_nt_pathname() (2021-09-30 20:00:05 -0500) ---------------------------------------------------------------- Eleven fixes for the ksmbd kernel server, mostly security related: - an important fix for disabling weak NTLMv1 authentication - seven security (improved buffer overflow checks) fixes - fix for wrong infolevel struct used in some getattr/setattr paths - two small documentation fixes Regression test results from Linux client to current ksmbd: http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/76 ---------------------------------------------------------------- Dan Carpenter (1): ksmbd: missing check for NULL in convert_to_nt_pathname() Enzo Matsumiya (1): ksmbd: fix documentation for 2 functions Hyunchul Lee (1): ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Namjae Jeon (7): ksmbd: fix invalid request buffer access in compound MAINTAINERS: rename cifs_common to smbfs_common in cifs and ksmbd entry ksmbd: remove NTLMv1 authentication ksmbd: use correct basic info level in set_file_basic_info() ksmbd: add request buffer validation in smb2_set_info ksmbd: add validation in smb2 negotiate ksmbd: fix transform header validation Ronnie Sahlberg (1): ksmbd: remove RFC1002 check in smb2 request MAINTAINERS | 4 +- fs/ksmbd/auth.c | 205 ------------------------------------- fs/ksmbd/crypto_ctx.c | 16 --- fs/ksmbd/crypto_ctx.h | 8 -- fs/ksmbd/misc.c | 17 ++-- fs/ksmbd/oplock.c | 41 ++++++-- fs/ksmbd/smb2pdu.c | 256 ++++++++++++++++++++++++++++++++++++----------- fs/ksmbd/smb2pdu.h | 9 ++ fs/ksmbd/smb_common.c | 47 +++++---- fs/ksmbd/smb_common.h | 8 -- fs/ksmbd/smbacl.c | 21 +++- fs/ksmbd/transport_tcp.c | 4 +- 12 files changed, 294 insertions(+), 342 deletions(-) -- Thanks, Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-10-02 4:14 [GIT PULL] ksmbd server security fixes Steve French @ 2021-10-03 0:52 ` pr-tracker-bot 0 siblings, 0 replies; 11+ messages in thread From: pr-tracker-bot @ 2021-10-03 0:52 UTC (permalink / raw) To: Steve French; +Cc: Linus Torvalds, LKML, CIFS The pull request you sent on Fri, 1 Oct 2021 23:14:41 -0500: > git://git.samba.org/ksmbd.git tags/5.15-rc3-ksmbd-fixes has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/e25ca045c32a0d787b143fef0acc5a43cc9ccc66 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* [GIT PULL] ksmbd server security fixes @ 2021-09-19 14:22 Steve French 2021-09-20 22:45 ` Linus Torvalds ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Steve French @ 2021-09-19 14:22 UTC (permalink / raw) To: Linus Torvalds; +Cc: CIFS, LKML Please pull the following changes since commit bf9f243f23e6623f310ba03fbb14e10ec3a61290: Merge tag '5.15-rc-ksmbd-part2' of git://git.samba.org/ksmbd (2021-09-09 16:17:14 -0700) are available in the Git repository at: git://git.samba.org/ksmbd.git tags/5.15-rc1-ksmbd for you to fetch changes up to 6d56262c3d224699b29b9bb6b4ace8bab7d692c2: ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info (2021-09-18 10:51:38 -0500) ---------------------------------------------------------------- 3 ksmbd fixes: including an important security fix for path processing, and a missing buffer overflow check, and a trivial fix for incorrect header inclusion There are three additional patches (and also a patch to improve symlink checks) for other buffer overflow cases that are being reviewed and tested. Regression test results: http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/67 and https://app.travis-ci.com/github/namjaejeon/ksmbd/builds/237919800 ---------------------------------------------------------------- Hyunchul Lee (1): ksmbd: prevent out of share access Mike Galbraith (1): ksmbd: transport_rdma: Don't include rwlock.h directly Namjae Jeon (1): ksmbd: add validation for FILE_FULL_EA_INFORMATION of smb2_get_info fs/ksmbd/misc.c | 76 +++++++++++++++++++++++++++++++++++++++++------ fs/ksmbd/misc.h | 3 +- fs/ksmbd/smb2pdu.c | 18 +++++++---- fs/ksmbd/transport_rdma.c | 1 - 4 files changed, 81 insertions(+), 17 deletions(-) -- Thanks, Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-19 14:22 Steve French @ 2021-09-20 22:45 ` Linus Torvalds 2021-09-21 2:16 ` Steve French 2021-09-20 23:32 ` pr-tracker-bot 2021-09-23 2:47 ` Kees Cook 2 siblings, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2021-09-20 22:45 UTC (permalink / raw) To: Steve French; +Cc: CIFS, LKML On Sun, Sep 19, 2021 at 7:22 AM Steve French <smfrench@gmail.com> wrote: > > 3 ksmbd fixes: including an important security fix for path > processing, and a missing buffer overflow check, and a trivial fix for > incorrect header inclusion > > There are three additional patches (and also a patch to improve > symlink checks) for other buffer overflow cases that are being > reviewed and tested. Note that if you are working on a path basis, you should really take a look at our vfs lookup_flags, and LOOKUP_BENEATH in particular. The way to deal with '..' and symlinks is not to try to figure it out yourself. You'll get it wrong, partly because the races with rename are quite interesting. The VFS layer knows how to limit pathname lookup to the particular directory you started in these days. Of course, that is only true for the actual path lookup functions. Once you start doing things manually one component at a time yourself, you're on your own. Linus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-20 22:45 ` Linus Torvalds @ 2021-09-21 2:16 ` Steve French 0 siblings, 0 replies; 11+ messages in thread From: Steve French @ 2021-09-21 2:16 UTC (permalink / raw) To: Linus Torvalds; +Cc: CIFS, LKML, Namjae Jeon On Mon, Sep 20, 2021 at 5:46 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sun, Sep 19, 2021 at 7:22 AM Steve French <smfrench@gmail.com> wrote: > > > > 3 ksmbd fixes: including an important security fix for path > > processing, and a missing buffer overflow check, and a trivial fix for > > incorrect header inclusion > > > > There are three additional patches (and also a patch to improve > > symlink checks) for other buffer overflow cases that are being > > reviewed and tested. > > Note that if you are working on a path basis, you should really take a > look at our vfs lookup_flags, and LOOKUP_BENEATH in particular. This was also something that Ralph brought up, and Namjae is looking at now. > The way to deal with '..' and symlinks is not to try to figure it out > yourself. You'll get it wrong, partly because the races with rename > are quite interesting. The VFS layer knows how to limit pathname > lookup to the particular directory you started in these days. > > Of course, that is only true for the actual path lookup functions. > Once you start doing things manually one component at a time yourself, > you're on your own. Agreed. Also FYI I removed the "ksmbd: Use LOOKUP_NO_SYMLINKS" changeset from for-next (I left the first two buffer validation changesets in, since those have been reviewed), since Namjae is working on an updated version following your suggestion (and others' review feedback). -- Thanks, Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-19 14:22 Steve French 2021-09-20 22:45 ` Linus Torvalds @ 2021-09-20 23:32 ` pr-tracker-bot 2021-09-23 2:47 ` Kees Cook 2 siblings, 0 replies; 11+ messages in thread From: pr-tracker-bot @ 2021-09-20 23:32 UTC (permalink / raw) To: Steve French; +Cc: Linus Torvalds, CIFS, LKML The pull request you sent on Sun, 19 Sep 2021 09:22:31 -0500: > git://git.samba.org/ksmbd.git tags/5.15-rc1-ksmbd has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/707a63e9a9dd55432d47bf40457d4a3413888dcc Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-19 14:22 Steve French 2021-09-20 22:45 ` Linus Torvalds 2021-09-20 23:32 ` pr-tracker-bot @ 2021-09-23 2:47 ` Kees Cook 2021-09-23 3:20 ` Steve French 2021-09-23 11:13 ` ronnie sahlberg 2 siblings, 2 replies; 11+ messages in thread From: Kees Cook @ 2021-09-23 2:47 UTC (permalink / raw) To: Steve French; +Cc: Linus Torvalds, CIFS, LKML On Sun, Sep 19, 2021 at 09:22:31AM -0500, Steve French wrote: > 3 ksmbd fixes: including an important security fix for path > processing, and a missing buffer overflow check, and a trivial fix for > incorrect header inclusion > > There are three additional patches (and also a patch to improve > symlink checks) for other buffer overflow cases that are being > reviewed and tested. Hi Steve, I was looking through the history[1] of the ksmbd work, and I'm kind of surprised at some of the flaws being found here. This looks like new code being written, too, I think (I found[0])? Some of these flaws are pretty foundational filesystem security properties[2] that weren't being tested for, besides the upsetting case of having buffer overflows[3] in an in-kernel filesystem server. I'm concerned about code quality here, and I think something needs to change about the review and testing processes. > Regression test results: > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/67 > and > https://app.travis-ci.com/github/namjaejeon/ksmbd/builds/237919800 Can you tell me more about these tests? I'm not immediately filled with confidence, when I see on the second line of the test harness: - wget --no-check-certificate https://mirrors.edge.kernel.org/pub/linux/kernel/v5.x/linux-5.4.109.tar.gz ^^^^^^^^^^^^^^^^^^^^^^ (and why isn't this a sparse clone?) I see xfstests and smbtorture getting run. Were these not catching things like "../../../../../" and the buffer overflows? And if not, where are the new tests that make sure these bugs can never recur? (Also, I see they're being run individually -- why not run the totality?) And looking at the Coverity report[4] under fs/ksmbd/* for linux-next, I see 12 issues dating back to Mar 17, and 1 from 2 days ago: 5 concurrency, 4 memory corruptions, 1 hang, and 2 resource leaks. Coverity is hardly free from false positives, but those seems worth addressing. (Both you and Namjae have accounts already; thank you for doing that a few months back!) Anyway, I think my point is: this doesn't look ready for production use. I understand having bugs, growing new features, etc, but I think more work is needed here to really prove this code is ready to expose the kernel to SMB protocol based attacks. Any binary parsing code needs to be extremely paranoid, and a network file server gets it coming and going: filesystem metadata and protocol handling (and crypto)! :P Anyway, I hope something can change here; if we're going to have an in-kernel SMB server, it should have a distinct advantage over userspace options. -Kees [0] https://lore.kernel.org/lkml/20210322051344.1706-1-namjae.jeon@samsung.com/ [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/fs/ksmbd [2] https://git.kernel.org/linus/f58eae6c5fa882d6d0a6b7587a099602a59d57b5 [3] https://git.kernel.org/linus/6d56262c3d224699b29b9bb6b4ace8bab7d692c2 [4] https://scan.coverity.com/projects/linux-next-weekly-scan View Defects, Settings cog, Filters, File: *ksmbd*, OK -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-23 2:47 ` Kees Cook @ 2021-09-23 3:20 ` Steve French 2021-09-23 15:42 ` Jeremy Allison 2021-09-23 18:21 ` Kees Cook 2021-09-23 11:13 ` ronnie sahlberg 1 sibling, 2 replies; 11+ messages in thread From: Steve French @ 2021-09-23 3:20 UTC (permalink / raw) To: Kees Cook; +Cc: Linus Torvalds, CIFS, LKML, Namjae Jeon On Wed, Sep 22, 2021 at 9:47 PM Kees Cook <keescook@chromium.org> wrote: > > On Sun, Sep 19, 2021 at 09:22:31AM -0500, Steve French wrote: > > 3 ksmbd fixes: including an important security fix for path > > processing, and a missing buffer overflow check, and a trivial fix for > > incorrect header inclusion > > > > There are three additional patches (and also a patch to improve > > symlink checks) for other buffer overflow cases that are being > > reviewed and tested. > > Hi Steve, > > I was looking through the history[1] of the ksmbd work, and I'm kind > of surprised at some of the flaws being found here. I was also surprised that a couple of these weren't found by smbtorture, although to be fair it is more focused on functional testing of the protocol (and is quite detailed). Most of my analysis of the code had been focused on functional coverage, and protocol features (and removing older less secure dialects, which he did). After lots of discussion about areas to review - we created this wiki page to track some of the detailed security review ongoing: https://wiki.samba.org/index.php/Ksmbd-review > I'm concerned about code quality here, and I think something needs to > change about the review and testing processes. Yes - we Namjae, Ronnie, Ralph and others have had multiple recent discussions about the review and testing process, and some useful improvements have been suggested. At a minimum it will include a review of each of the key security areas, and additional tests added (I added a few functional tests yesterday, but more need to be added, perhaps using some of Ronnie's libsmb2 libraries that we used to repro some of the security problems). > > Regression test results: > > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/67 > > and > > https://app.travis-ci.com/github/namjaejeon/ksmbd/builds/237919800 > > Can you tell me more about these tests? I'm not immediately There are basically two types of tests run: - xfstests from Linux to Linux (usually from current mainline cifs.ko mounted to current ksmbd). These are largely functional tests, more at an application level - smbtorture is the Samba functional test suite, and is largely at the protocol level, to show protocol correctness. There are a few security/overflow related tests in this, but more need to be added. The smbtorture runs are automated in github, the xfstest runs are semi-automated, but I have to manually trigger them. The builders for xfstest 'buildbot runs use a Fedora VM on the client, that is spun up with the standard Linux 'buildbot' testing infrastructure, then the kernel is rebbuilt and replaced and then the Fedora client VM rebooted to the current kernel (in the case of the run you are pointing it is was running 5.15-rc2 with one patch applied (see below). The patch was added (from one posted on lkml) to avoid the build break in current mainline Linux. "HEAD is now at 1f07f2e... scripts/sorttable: riscv: fix undelcred identifier 'EM_RISCV' error" > I see xfstests and smbtorture getting run. Were these not catching > things like "../../../../../" and the buffer overflows? And if not, > where are the new tests that make sure these bugs can never recur? That (adding additional functional tests for smb3 overflows, and also it restarts a discussion about creating open source "smb3 fuzzing" tools to help Samba and ksmbd both) ... that is a discussion I have been having with others on the Samba team as well, some of the security bugs could have been found with additions to the "smbtorture" set of functional tests (which are hosted in the Samba server projects). > (Also, I see they're being run individually -- why not run the totality?) You can't run the totality of xfstests (some are not applicable for network fs e.g.) nor of smbtorture (some tests aren't applicable). > And looking at the Coverity report[4] under fs/ksmbd/* for linux-next, I > see 12 issues dating back to Mar 17, and 1 from 2 days ago: 5 concurrency, > 4 memory corruptions, 1 hang, and 2 resource leaks. Coverity is hardly > free from false positives, but those seems worth addressing. (Both you and > Namjae have accounts already; thank you for doing that a few months back!) I completely agree with the importance of Coverity as, even if the majority are 'false positives' or not high priority - there are plenty that Coverity points out that are not. I have focused more on Coverity for cifs.ko than for ksmbd, but after the security patches are merged, it would be good to switch gears to that. > Anyway, I think my point is: this doesn't look ready for production use. > I understand having bugs, growing new features, etc, but I think more > work is needed here to really prove this code is ready to expose the I am pleased with the progress that Namjae et al have been making addressing the problems identified, but agree it is not ready for production use yet, despite good functional test results - and testing events (like the SMB3 plugfest next week) are going to be important, as well as the security reviews. Fortunately the code size is manageable (25KLOC), and without legacy, insecure dialects to worry about (SMB1, LANMAN etc.), unlike most servers, the reviews should proceed reasonably quickly. The current patch list which has been reviewed and tested so far (includes fixes for some of the issues you mention) is: ksmbd: add request buffer validation in smb2_set_info 743d886affeb ksmbd: remove follow symlinks support 3bee78ad0062 ksmbd: fix invalid request buffer access in compound request 18a015bccf9e ksmbd: check protocol id in ksmbd_verify_smb_message() 9f6323311c70 ksmbd: add default data stream name in FILE_STREAM_INFORMATION e44fd5081c50 ksmbd: log that server is experimental at module load But there at least five more under review and testing. Namjae et al have been very responsive. Good news about these test events, which are held multiple times each year for SMB, is that some of the companies participating in the past have run their fuzzers against Samba (and now will be able to do the same against ksmbd). There is some good news (relating to security), once Namjae et al get past these buffer overflow etc. patches. - he has already implemented the strongest encryption supported in SMB3.1.1 - he has implemented the man in the middle attack prevention features of the protocol - strong (Kerberos) authentication is implemented - he has removed support for weak older dialects (including SMB1 and SMB2) of the protocol - he will be removing support for weaker authentication (including NTLMv1) Any feedback you have on the security list identified in the wiki list above, or other things you see in Coverity or the mailing list discussions reviewing the patches would be helpful. -- Thanks, Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-23 3:20 ` Steve French @ 2021-09-23 15:42 ` Jeremy Allison 2021-09-23 18:21 ` Kees Cook 1 sibling, 0 replies; 11+ messages in thread From: Jeremy Allison @ 2021-09-23 15:42 UTC (permalink / raw) To: Steve French; +Cc: Kees Cook, Linus Torvalds, CIFS, LKML, Namjae Jeon On Wed, Sep 22, 2021 at 10:20:01PM -0500, Steve French wrote: >On Wed, Sep 22, 2021 at 9:47 PM Kees Cook <keescook@chromium.org> wrote: >> >> Hi Steve, >> >> I was looking through the history[1] of the ksmbd work, and I'm kind >> of surprised at some of the flaws being found here. > >I was also surprised that a couple of these weren't found by smbtorture, >although to be fair it is more focused on functional testing of the protocol >(and is quite detailed). Most of my analysis of the code had been >focused on functional coverage, and protocol features (and removing Steve, you should have been surprised they weren't caught by smbtorture, especially if your "analysis of the code had been focused on functional coverage". No one has been looking at the logic for this, and IMHO that's a problem. It's good they are looking now, but I think this code needs additional maintainers. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-23 3:20 ` Steve French 2021-09-23 15:42 ` Jeremy Allison @ 2021-09-23 18:21 ` Kees Cook 1 sibling, 0 replies; 11+ messages in thread From: Kees Cook @ 2021-09-23 18:21 UTC (permalink / raw) To: Steve French; +Cc: Linus Torvalds, CIFS, LKML, Namjae Jeon On Wed, Sep 22, 2021 at 10:20:01PM -0500, Steve French wrote: > After lots of discussion about areas to review - we created this wiki > page to track some of the detailed security review ongoing: > > https://wiki.samba.org/index.php/Ksmbd-review Great! > That (adding additional functional tests for smb3 overflows, and > also it restarts a discussion about creating open source "smb3 fuzzing" > tools to help Samba and ksmbd both) ... that is a discussion I have > been having with others on the Samba team as well, some of > the security bugs could have been found with additions > to the "smbtorture" set of functional tests (which are hosted in the Samba > server projects). Yeah, I think this is really important, and especially for bug fixing: if a bug gets fixed in protocol or filesystem handling, there needs to be a test to go with it. Without that, no one can say with a straight face that it is actually fixed. It's just a band-aid unless there is an accompanying test that exercises the flaw to make sure the fix doesn't regress in the future. So, I think each of the recent fixes needs to have an associated test -- especially the path walking and buffer overflows. Is there a "patch requirements" doc for doing reviews? I don't see anything specific to the "on going" review process at the wiki. The wiki just calls out a number of areas that need out-of-band examination (which is great!) in the form of basically a detailed TODO list. But I don't see an actual patch review process. Specifically, what things must a patch author do before the maintainer will be happy to accept a patch? > I am pleased with the progress that Namjae et al have been making > addressing the problems identified, but agree it is not ready for production > use yet, despite good functional test results - and testing events > (like the SMB3 > plugfest next week) are going to be important, as well as the security reviews. > Fortunately the code size is manageable (25KLOC), and without legacy, > insecure dialects to worry about (SMB1, LANMAN etc.), unlike most servers, > the reviews should proceed reasonably quickly. Great! I'm glad to hear it. For those events do you build kernels will full KASAN, KMSAN, KCSAN, etc enabled? There might be a lot of flaws that wouldn't otherwise get noticed. > There is some good news (relating to security), once Namjae et al get past > these buffer overflow etc. patches. > - he has already implemented the strongest encryption supported in SMB3.1.1 > - he has implemented the man in the middle attack prevention features > of the protocol > - strong (Kerberos) authentication is implemented Sounds excellent -- have these received professional crypto review? There are a lot of corner cases in crypto negotiation procotols. > - he has removed support for weak older dialects (including SMB1 and > SMB2) of the protocol > - he will be removing support for weaker authentication (including NTLMv1) Yay attack surface reduction! :) > Any feedback you have on the security list identified in the wiki list > above, or other > things you see in Coverity or the mailing list discussions reviewing the patches > would be helpful. Thanks for making these recent changes; I feel much better about ksmbd's direction. I'll take a look through the Wiki. Thanks! -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] ksmbd server security fixes 2021-09-23 2:47 ` Kees Cook 2021-09-23 3:20 ` Steve French @ 2021-09-23 11:13 ` ronnie sahlberg 1 sibling, 0 replies; 11+ messages in thread From: ronnie sahlberg @ 2021-09-23 11:13 UTC (permalink / raw) To: Kees Cook; +Cc: Steve French, Linus Torvalds, CIFS, LKML On Thu, Sep 23, 2021 at 12:48 PM Kees Cook <keescook@chromium.org> wrote: > > On Sun, Sep 19, 2021 at 09:22:31AM -0500, Steve French wrote: > > 3 ksmbd fixes: including an important security fix for path > > processing, and a missing buffer overflow check, and a trivial fix for > > incorrect header inclusion > > > > There are three additional patches (and also a patch to improve > > symlink checks) for other buffer overflow cases that are being > > reviewed and tested. > > Hi Steve, > > I was looking through the history[1] of the ksmbd work, and I'm kind > of surprised at some of the flaws being found here. This looks like new > code being written, too, I think (I found[0])? Some of these flaws are > pretty foundational filesystem security properties[2] that weren't being > tested for, besides the upsetting case of having buffer overflows[3] > in an in-kernel filesystem server. > > I'm concerned about code quality here, and I think something needs to > change about the review and testing processes. > > > Regression test results: > > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/67 > > and > > https://app.travis-ci.com/github/namjaejeon/ksmbd/builds/237919800 > > Can you tell me more about these tests? I'm not immediately filled with > confidence, when I see on the second line of the test harness: > > - wget --no-check-certificate https://mirrors.edge.kernel.org/pub/linux/kernel/v5.x/linux-5.4.109.tar.gz > ^^^^^^^^^^^^^^^^^^^^^^ > > (and why isn't this a sparse clone?) > > I see xfstests and smbtorture getting run. Were these not catching > things like "../../../../../" and the buffer overflows? And if not, > where are the new tests that make sure these bugs can never recur? > > (Also, I see they're being run individually -- why not run the totality?) I can answer this I set it up this way to unload and reload the module for each test because at the time the module was in really bad shape and doing these reset to known state would make it easier to find easily reproducible faults from running test xyz than chasing shadows when previous test abc or def had left residuals that caused an oops. I.e. Find oopses and failures that are contained in a single test. It should be possible to skip the unload/reload cycle for each test now as the module is a lot more stable. > > And looking at the Coverity report[4] under fs/ksmbd/* for linux-next, I > see 12 issues dating back to Mar 17, and 1 from 2 days ago: 5 concurrency, > 4 memory corruptions, 1 hang, and 2 resource leaks. Coverity is hardly > free from false positives, but those seems worth addressing. (Both you and > Namjae have accounts already; thank you for doing that a few months back!) > > Anyway, I think my point is: this doesn't look ready for production use. > I understand having bugs, growing new features, etc, but I think more > work is needed here to really prove this code is ready to expose the > kernel to SMB protocol based attacks. Any binary parsing code needs to be > extremely paranoid, and a network file server gets it coming and going: > filesystem metadata and protocol handling (and crypto)! :P > > Anyway, I hope something can change here; if we're going to have an > in-kernel SMB server, it should have a distinct advantage over userspace > options. > > -Kees > > [0] https://lore.kernel.org/lkml/20210322051344.1706-1-namjae.jeon@samsung.com/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/fs/ksmbd > [2] https://git.kernel.org/linus/f58eae6c5fa882d6d0a6b7587a099602a59d57b5 > [3] https://git.kernel.org/linus/6d56262c3d224699b29b9bb6b4ace8bab7d692c2 > [4] https://scan.coverity.com/projects/linux-next-weekly-scan > View Defects, Settings cog, Filters, File: *ksmbd*, OK > > -- > Kees Cook ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-03 0:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-02 4:14 [GIT PULL] ksmbd server security fixes Steve French 2021-10-03 0:52 ` pr-tracker-bot -- strict thread matches above, loose matches on Subject: below -- 2021-09-19 14:22 Steve French 2021-09-20 22:45 ` Linus Torvalds 2021-09-21 2:16 ` Steve French 2021-09-20 23:32 ` pr-tracker-bot 2021-09-23 2:47 ` Kees Cook 2021-09-23 3:20 ` Steve French 2021-09-23 15:42 ` Jeremy Allison 2021-09-23 18:21 ` Kees Cook 2021-09-23 11:13 ` ronnie sahlberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).