On Tue, Sep 21, 2010 at 10:40 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote: > 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 > Is this correct? Every auth type gets its own smb connection (in match security)? We probably do not need that check (and functionality). -- 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
