linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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>,
	Namjae Jeon <linkinjeon@kernel.org>
Subject: Re: [GIT PULL] ksmbd server security fixes
Date: Thu, 23 Sep 2021 11:21:11 -0700	[thread overview]
Message-ID: <202109231109.0AD3D5A@keescook> (raw)
In-Reply-To: <CAH2r5muNG4GvziyMG2unkYNjUiT4V+pz0pWUGkWQNxUZJnBadw@mail.gmail.com>

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

  parent reply	other threads:[~2021-09-23 18:21 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-19 14:22 [GIT PULL] ksmbd server security fixes Steve French
2021-09-20 22:45 ` Linus Torvalds
2021-09-21  2:16   ` Steve French
2021-09-20 23:32 ` pr-tracker-bot
2021-09-23  2:47 ` Kees Cook
2021-09-23  3:20   ` Steve French
2021-09-23 15:42     ` Jeremy Allison
2021-09-23 18:21     ` Kees Cook [this message]
2021-09-23 11:13   ` ronnie sahlberg
2021-10-02  4:14 Steve French
2021-10-03  0:52 ` pr-tracker-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202109231109.0AD3D5A@keescook \
    --to=keescook@chromium.org \
    --cc=linkinjeon@kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=smfrench@gmail.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).