On Wed, 22 Sep 2010 15:42:16 -0500 Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > 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). > Yep, that's correct. Note that the secType is a per-server parameter right now. We could mix sectypes on a socket. I sent a patchset that did that this past spring, but Steve basically NAK'ed it. IIRC, he wanted to allow the sec= option to take an ordered list of sectypes and I didn't see any way to do that sanely. After that I backed off and left that more or less as is, but fixed it so that socket selection takes the secType into account (hence the match_security function). -- 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
