Disable SMB2 for 3.6?

[Volker Lendecke]

On Thu, Jul 07, 2011 at 12:07:40PM -0700, Jeremy Allison wrote:
> 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.

Reviewed metze's patch: I think it is just correct and does
fix the problem. Yesterday I was just kind of shocked to
find this and did overreact. Apologies for that.

Regarding the code restructurings that you mention: In SMB2
we have few enough calls that these functions can be
distributed among that switch statement. In SMB1 we have a
lot more calls, so I think the table approach is appropriate
for that protocol. For clarity though I would really like to
see that code together in one place right next to the big
switch statement. This might mean just moving code around,
but for my taste it would make it more obvious.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen