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@xxxxxxxxxxx/ [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
