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
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
On Sun, Sep 19, 2021 at 7:22 AM Steve French <[email protected]> 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
On Mon, Sep 20, 2021 at 5:46 PM Linus Torvalds
<[email protected]> wrote:
>
> On Sun, Sep 19, 2021 at 7:22 AM Steve French <[email protected]> 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
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/[email protected]/
[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
On Wed, Sep 22, 2021 at 9:47 PM Kees Cook <[email protected]> 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
On Thu, Sep 23, 2021 at 12:48 PM Kees Cook <[email protected]> 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/[email protected]/
> [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
On Wed, Sep 22, 2021 at 10:20:01PM -0500, Steve French wrote:
>On Wed, Sep 22, 2021 at 9:47 PM Kees Cook <[email protected]> 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.
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