Disable SMB2 for 3.6?

[Jeremy Allison]

On Thu, Jul 07, 2011 at 07:32:54PM +0200, Volker Lendecke wrote:
> 
> On Thu, Jul 07, 2011 at 10:12:26AM -0700, Jeremy Allison wrote:
> > So I've been thinking about this. The reason it went undetected
> > for so long is that mostly it's not an issue for correctness for
> > the client. Most clients are single credential connections for
> > most of the time.
> > 
> > Sure, it's a mistake and has to be fixed at the top level (in
> > the same way as we have the AS_USER flag in SMB1), but it isn't
> > a "the sky is falling" bug as you're saying here.
> 
> It is, sorry. The problem is that the architecture is so
> opaque that this was not detected for many, many months. And
> this is not fixable with a simple patch. We need to re-think
> the security model of the SMB2 server, and this takes much
> more time than we have before releasing 3.6. This has been
> postponed too many times already, we just can't deny our
> users the other features anymore.

Ok, I've spent some time looking at the error, and metze's
fix.

IMHO (and I hope metze's too) our security architecture is
actually in good shape for SMB2.

Every incoming request with a session id and tree id associated
with it goes through the functions smbd_smb2_request_check_session()
which ensures that the session id is a valid one, and then it goes through
smbd_smb2_request_check_tcon(), which ensures that the tree id given is a
valid one for that session id to access.

It is in this function (smbd_smb2_request_check_tcon()) that change_to_user()
to set the credentials for a valid SMB2 call is called.

As in SMB1, we don't go back to root in between every incoming
calls as most commonly calls will be coming in with the same
session and tree id's which allows us to skip changing UNIX
credentials between calls, which we know can be costly performance-wise.

The error in the SMB2 server logic, and which metze's patch
corrects, is to not uniquely identify the SMB2 requests that
have a valid session-id but don't have an associated tree-id,
and so should be processed as root.

The current design "fails-safe" as it were, in that it keeps
the non-root user credentials from the previous call when processing
one of these tid-less requests. Processing tid-less requests as the
previous user is bad, but not nearly as bad as the reverse, in which
where we would sess/tid requests would be processed as root instead
of the correct user (which at first is what I thought you said was
happening).

Metze's fix is correct, in that adding become_root() calls into
the switch statement for tid-less calls will fix the problem,
but it's different from the way we handle it in SMB1 where we
have a lookup table with a flag that specifies whether we process
as root or not (the AS_USER flag).

The central question - and hopefully Volker will weigh in on
this - is should we add such a lookup table in the SMB2
dispatch function to flag the opcodes that require processing
as root and do a lookup, or is it clearer to use metze's patch
as-is and insert them inline into each switch ?

Metze - do you have an opinion as to whether your patch produces
the clearest way to get maintainable code ? I'm assuming you
want the become_root() calls inline ?

Volker - would it help if the function smbd_smb2_request_check_tcon()
was moved out of smbd/smb2_tcon.c and into smbd/smb2_server.c
and the same for smbd_smb2_request_check_session() (move out of
smbd/smb2_sessetup.c into smbd/smb2_server.c) as it would make
the validation functions locally located to the dispatch function
that has to call them to check security ? Right now they are
located in the files that are named after the code that processes
the request, which makes sense from a naming point of view, but
not from a logic point of view - these functions are validation
code, not request processing code. If we moved them both into
smbd/smb2_server.c to just above smbd_smb2_request_dispatch()
they could both be make static to that file.

Volker, your opinion is most valuable, as you're the one who
found this bug - which is most definitely a show-stopper for
release (the very definition in fact :-).

I'm just eternally grateful that I have Volker watching my
back for bugs like this :-) :-). We wouldn't produce code
of any quality at all without this kind of review.

Thanks,

Jeremy.