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
