On Tue, 21 Sep 2010 22:20:21 -0500 Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > On Tue, Sep 21, 2010 at 10:07 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > > On Tue, 21 Sep 2010 21:57:44 -0500 > > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > > > >> On Tue, Sep 21, 2010 at 6:42 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > >> > On Tue, 21 Sep 2010 14:04:24 -0500 > >> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > >> > > >> >> > > >> >> > Right. I'm just not sure why we need a separate flag attached to the > >> >> > server struct for this. Why was the "first_time" mechanism not good > >> >> > enough here? I see no reason why that wouldn't have worked for NTLMSSP > >> >> > too. > >> >> > >> >> Jeff, I will investigate but at the first glance, it looks like > >> >> first_ses is per smb session > >> >> and not smb connection, not sure if that would be good enough for ntlmssp. > >> >> > >> > > >> > first_time is set by is_first_ses_reconnect(). The comment on that > >> > function says: > >> > > >> > * Checks if this is the first smb session to be reconnected after > >> > * the socket has been reestablished (so we know whether to use vc 0). > >> > * Called while holding the cifs_tcp_ses_lock, so do not block > >> > > >> > ...which isn't entirely true, since this works even when there hasn't > >> > been a reconnect. It just walks the list of sessions on a socket and > >> > sees if any of them are already established (that is, need_reconnect > >> > is false). > >> > > >> > So there is nominally a bug here -- sesInfoAlloc probably should set > >> > needs_reconnect to true. But since cifs_get_smb_ses doesn't stick the > >> > session on the server's list until after the session setup succeeds the > >> > first time, it doesn't really cause any problems. > >> > > >> > -- > >> > Jeff Layton <jlayton@xxxxxxxxx> > >> > > >> > >> We probably ought to keep first_time or cphready per type of auth mech > >> since it is possible to have multiple smb sessions on the same connection using > >> various kinds of authentication mechanisms. > >> > > > > Ok, but only one of them gets to set the actual session_key. The socket > > is a "shared" resource of sorts and we need something that indicates > > which session has the "right of way" to set the session_key. The thing > > with traffic lights is that they only work if everyone agrees on which > > ones to use. ;) > > > > I am not sure traffic light analogy will work. I think signing for session > with sec=ntlmsspi will fail if sec=ntlmv2i happend to be very first session > on that smb connect and has set up the session key. > The keys are different, stored in different locations, and the scheme is > different i.e. the key sizes that go in the signing are different sizes. > Do you know this for a fact? My understanding is different than this. AFAIU, the session_key is simply set by the first SESSION_SETUP performed on the socket. If the first one uses plain NTLM and then a later one uses krb5, then the socket uses the key from the NTLM setup for signing even if the krb5 session had a bigger key. Now, in point of fact, the client doesn't currently mix sectypes on a socket, so the argument is somewhat moot. If there are races with the first_time flag however (and I think that you're correct that there are), then they should be fixed. > > Your previous email sounded convincing that there is a potential race > > there. I think you should work through the implications of that and > > come up with a race-free scheme that fixes this. Don't just do it for > > NTLMSSP though -- fix it for all the sectypes. > > > > The scope of this patch was deemed ntlmv2 within NTLMSSP > authentication and signing. > I do not have a setup for kerberos testing on Samba as well as Windows > server as of now. > That's fine, a separate patch will do. My suggestion would be to do an initial cleanup patch that eliminates the first_time flag and switches it to use your new scheme or something similarly non-racy, and then to respin patches 3 and 4 in your set as a new set on top of that. -- Jeff Layton <jlayton@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
