Message-ID: <202109231109.0AD3D5A@keescook>
Date:   Thu, 23 Sep 2021 11:21:11 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Steve French <smfrench@...il.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        CIFS <linux-cifs@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Namjae Jeon <linkinjeon@...nel.org>
Subject: Re: [GIT PULL] ksmbd server security fixes

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

Powered by blists - more mailing lists