Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAH2r5mvu5wTcgoR-EeXLcoZOvhEiMR0Lfmwt6gd1J1wvtTLDHA@mail.gmail.com>
 <202109221850.003A16EC1@keescook>
In-Reply-To: <202109221850.003A16EC1@keescook>
From:   ronnie sahlberg <ronniesahlberg@gmail.com>
Date:   Thu, 23 Sep 2021 21:13:28 +1000
Message-ID: <CAN05THQ6xT0dWyev+c-PhJ+LZ6ABNpCxCzBK7PQHJM_yaE+wWQ@mail.gmail.com>
Subject: Re: [GIT PULL] ksmbd server security fixes
To:     Kees Cook <keescook@chromium.org>
Cc:     Steve French <smfrench@gmail.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        CIFS <linux-cifs@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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