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;
Date:   Wed, 22 Sep 2021 19:47:44 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Steve French <smfrench@gmail.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        CIFS <linux-cifs@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] ksmbd server security fixes
Message-ID: <202109221850.003A16EC1@keescook>
References: <CAH2r5mvu5wTcgoR-EeXLcoZOvhEiMR0Lfmwt6gd1J1wvtTLDHA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAH2r5mvu5wTcgoR-EeXLcoZOvhEiMR0Lfmwt6gd1J1wvtTLDHA@mail.gmail.com>
Precedence: bulk

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