| Message ID | 1481179577-15995-1-git-send-email-sprabhu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.com> wrote: > If the security type specified using a mount option is not supported, > the SMB2 session setup code changes the security type to RawNTLMSSP. We > should instead fail the mount and return an error. > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > --- > fs/cifs/smb2pdu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 5ca5ea46..e66fad6 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct SMB2_sess_data *sess_data) > static int > SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data *sess_data) > { > - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) > + /* Default sec type is set to RawNTLMSSP */ > + if (ses->sectype == Unspecified) > ses->sectype = RawNTLMSSP; > > switch (ses->sectype) { > -- > 2.7.4 > > -- My initial reaction was "allow the SSP to do its thing". However, after some consideration, this is a much better way to handle this exceptional case when a user gives an explicit security type and it cannot be honored. +1, FWIW My only concern is, "will this be considered a regression by users that have (unknowingly) relied upon the previous behavior?"
On Thu, 2016-12-08 at 02:06 -0600, Scott Lovenberg wrote: > On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.com> > wrote: > > > > If the security type specified using a mount option is not > > supported, > > the SMB2 session setup code changes the security type to > > RawNTLMSSP. We > > should instead fail the mount and return an error. > > > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > > --- > > fs/cifs/smb2pdu.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > index 5ca5ea46..e66fad6 100644 > > --- a/fs/cifs/smb2pdu.c > > +++ b/fs/cifs/smb2pdu.c > > @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct > > SMB2_sess_data *sess_data) > > static int > > SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data > > *sess_data) > > { > > - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) > > + /* Default sec type is set to RawNTLMSSP */ > > + if (ses->sectype == Unspecified) > > ses->sectype = RawNTLMSSP; > > > > switch (ses->sectype) { > > -- > > 2.7.4 > > > > -- > > My initial reaction was "allow the SSP to do its thing". However, > after some consideration, this is a much better way to handle this > exceptional case when a user gives an explicit security type and it > cannot be honored. +1, FWIW > My only concern is, "will this be considered a regression by users > that have (unknowingly) relied upon the previous behavior?" > That is a valid concern which could trip up users who have been using an incorrect mount option. However in my opinion it would be better to reject mounts in case where the mount options requested are not available instead of silently switching the mount options and ending up with behaviour which wasn't expected by the user. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 8, 2016 at 3:03 AM, Sachin Prabhu <sprabhu@redhat.com> wrote: > On Thu, 2016-12-08 at 02:06 -0600, Scott Lovenberg wrote: >> On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.com> >> wrote: >> > >> > If the security type specified using a mount option is not >> > supported, >> > the SMB2 session setup code changes the security type to >> > RawNTLMSSP. We >> > should instead fail the mount and return an error. >> > >> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> >> > --- >> > fs/cifs/smb2pdu.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >> > index 5ca5ea46..e66fad6 100644 >> > --- a/fs/cifs/smb2pdu.c >> > +++ b/fs/cifs/smb2pdu.c >> > @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct >> > SMB2_sess_data *sess_data) >> > static int >> > SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data >> > *sess_data) >> > { >> > - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) >> > + /* Default sec type is set to RawNTLMSSP */ >> > + if (ses->sectype == Unspecified) >> > ses->sectype = RawNTLMSSP; >> > >> > switch (ses->sectype) { >> > -- >> > 2.7.4 >> > >> > -- >> >> My initial reaction was "allow the SSP to do its thing". However, >> after some consideration, this is a much better way to handle this >> exceptional case when a user gives an explicit security type and it >> cannot be honored. +1, FWIW >> My only concern is, "will this be considered a regression by users >> that have (unknowingly) relied upon the previous behavior?" >> > That is a valid concern which could trip up users who have been using > an incorrect mount option. However in my opinion it would be better to > reject mounts in case where the mount options requested are not > available instead of silently switching the mount options and ending up > with behaviour which wasn't expected by the user. > > Sachin Prabhu Perhaps a reasonable middle ground would be logging so it fails less silently? I'm not sure that average users would think to check the kernel log though, so it might be a moot point. Either way, the ends justify the means here IMHO.
---------- Forwarded message ---------- From: Steve French <smfrench@gmail.com> Date: Tue, Jan 10, 2017 at 2:21 PM Subject: Re: [PATCH] SMB2: Enforce sec= mount option To: Sachin Prabhu <sprabhu@redhat.com> Cc: linux-cifs <linux-cifs@vger.kernel.org> Thinking about this a little more - the only minor correction I would suggest is that NTLMv2 might as well continue to map to RawNTLMSSP (since they have similar meanings, ie ntlmv2 hash) - but we could error out on the others (other than krb5) On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.com> wrote: > > If the security type specified using a mount option is not supported, > the SMB2 session setup code changes the security type to RawNTLMSSP. We > should instead fail the mount and return an error. > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > --- > fs/cifs/smb2pdu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 5ca5ea46..e66fad6 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct SMB2_sess_data *sess_data) > static int > SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data *sess_data) > { > - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) > + /* Default sec type is set to RawNTLMSSP */ > + if (ses->sectype == Unspecified) > ses->sectype = RawNTLMSSP; > > switch (ses->sectype) { > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Sachin Prabhu wrote: > If the security type specified using a mount option is not supported, > the SMB2 session setup code changes the security type to RawNTLMSSP. We > should instead fail the mount and return an error. > --- Saw the comment by Steve F, and it got me to thinking. Please take this as a suggestion or idea... I'm not heavily committed to a single solution, at this point, as haven't really thought through all of the ramifications. Is it possible to add a 'prefix' or 'suffix', like an "=" sign or a '+' -- to mean: '=' = exactly this 'sec' level '+' = this 'sec'-level or greater '<' = less than or equal to this sec-level --- Using the symbols is a similar idea to some fields in 'find' where +/- are used to indicate greater or less than the stated number. I'm not sure about the symbols, exactly, but I know in samba I ask for smb2 for the protocol and more often than not, only get smb1, but I'd rather have it work than fail. Since I'm on a closed net, I'd have to say the same for security options, but I'd like to have a choice to force it if I wanted to... Anyway -- just an idea that might offer more flexibility than just 'fail'... -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sachin Prabhu wrote: > If the security type specified using a mount option is not supported, > the SMB2 session setup code changes the security type to RawNTLMSSP. We > should instead fail the mount and return an error. > --- Saw the comment by Steve F, and it got me to thinking. Please take this as a suggestion or idea... I'm not heavily committed to a single solution, at this point, as haven't really thought through all of the ramifications. Is it possible to add a 'prefix' or 'suffix', like an "=" sign or a '+' -- to mean: '=' = exactly this 'sec' level '+' = this 'sec'-level or greater '<' = less than or equal to this sec-level --- Using the symbols is a similar idea to some fields in 'find' where +/- are used to indicate greater or less than the stated number. I'm not sure about the symbols, exactly, but I know in samba I ask for smb2 for the protocol and more often than not, only get smb1, but I'd rather have it work than fail. Since I'm on a closed net, I'd have to say the same for security options, but I'd like to have a choice to force it if I wanted to... Anyway -- just an idea that might offer more flexibility than just 'fail'... -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
L A Walsh <law@tlinx.org> writes: > I'm not sure about the symbols, exactly, but I know in samba I > ask for smb2 for the protocol and more often than not, only > get smb1, but I'd rather have it work than fail. About this specific thing: I believe smbclient negotiate the highest protocol version it can (according to the min/max protocol version set in its conf) whereas cifs.ko always uses smb1 by default (which is probably against the spec).
Hi, My 2 cents. Everything depends on the interpretation given to the sec option. If it is interpreted like "this is what I want" then, yes, this patch does the right thing. But if it is interpreted as "this is what I prefer" then, rejecting the mount is not the right thing and will be seen as a regression. The latter interpretation is not so uncommon, given there is not an easy way to ask the kernel what is the default negotiation protocol and there is not a way to blacklist protocols. I planned to implement a blacklist mechanism (along with a preference list) but it is overkill for the present purpose. My suggestion is to add an additional boolean to make this option strict, so it would be possible to let old code use sec as a suggestion while others can start adding the boolean if they prefer failures over silent changes. Obviously, this requires changes in doc and mount.cifs but could save some headaches. Regards, Germano On 12/08/2016 09:03 AM, Sachin Prabhu wrote: > On Thu, 2016-12-08 at 02:06 -0600, Scott Lovenberg wrote: >> On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.com> >> wrote: >>> >>> If the security type specified using a mount option is not >>> supported, >>> the SMB2 session setup code changes the security type to >>> RawNTLMSSP. We >>> should instead fail the mount and return an error. >>> >>> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> >>> --- >>> fs/cifs/smb2pdu.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index 5ca5ea46..e66fad6 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct >>> SMB2_sess_data *sess_data) >>> static int >>> SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data >>> *sess_data) >>> { >>> - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) >>> + /* Default sec type is set to RawNTLMSSP */ >>> + if (ses->sectype == Unspecified) >>> ses->sectype = RawNTLMSSP; >>> >>> switch (ses->sectype) { >>> -- >>> 2.7.4 >>> >>> -- >> >> My initial reaction was "allow the SSP to do its thing". However, >> after some consideration, this is a much better way to handle this >> exceptional case when a user gives an explicit security type and it >> cannot be honored. +1, FWIW >> My only concern is, "will this be considered a regression by users >> that have (unknowingly) relied upon the previous behavior?" >> > That is a valid concern which could trip up users who have been using > an incorrect mount option. However in my opinion it would be better to > reject mounts in case where the mount options requested are not > available instead of silently switching the mount options and ending up > with behaviour which wasn't expected by the user. > > Sachin Prabhu > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-01-11 at 11:45 +0000, Germano Percossi wrote: > Hi, > > My 2 cents. > > Everything depends on the interpretation given to the sec option. > If it is interpreted like "this is what I want" then, yes, this patch > does the right thing. > > But if it is interpreted as "this is what I prefer" then, rejecting > the > mount is not the right thing and will be seen as a regression. > > The latter interpretation is not so uncommon, given there is not an > easy > way to ask the kernel what is the default negotiation protocol and > there > is not a way to blacklist protocols. My own opinion is that when a mount option is explicitly passed, it is expected the option provided be used and fail if it cannot be used. I think replacing a explicitly requested mount option with another can lead to unexpected results and may also affect things activities like testing. There are other mount options like "vers=" where cifs doesn't attempt to replace an invalid protocol version with another. This in my opinion is the correct behaviour. Steve, What is your opinion on this? Sachin Prabhu > > I planned to implement a blacklist mechanism (along with a preference > list) but it is overkill for the present purpose. > > My suggestion is to add an additional boolean to make this option > strict, so it would be possible to let old code use sec as a > suggestion while others can start adding the boolean if they prefer > failures over silent changes. > > Obviously, this requires changes in doc and mount.cifs but could save > some headaches. > > Regards, > Germano > > On 12/08/2016 09:03 AM, Sachin Prabhu wrote: > > On Thu, 2016-12-08 at 02:06 -0600, Scott Lovenberg wrote: > > > On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu@redhat.co > > > m> > > > wrote: > > > > > > > > If the security type specified using a mount option is not > > > > supported, > > > > the SMB2 session setup code changes the security type to > > > > RawNTLMSSP. We > > > > should instead fail the mount and return an error. > > > > > > > > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > > > > --- > > > > fs/cifs/smb2pdu.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > > > index 5ca5ea46..e66fad6 100644 > > > > --- a/fs/cifs/smb2pdu.c > > > > +++ b/fs/cifs/smb2pdu.c > > > > @@ -955,7 +955,8 @@ > > > > SMB2_sess_auth_rawntlmssp_authenticate(struct > > > > SMB2_sess_data *sess_data) > > > > static int > > > > SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data > > > > *sess_data) > > > > { > > > > - if (ses->sectype != Kerberos && ses->sectype != > > > > RawNTLMSSP) > > > > + /* Default sec type is set to RawNTLMSSP */ > > > > + if (ses->sectype == Unspecified) > > > > ses->sectype = RawNTLMSSP; > > > > > > > > switch (ses->sectype) { > > > > -- > > > > 2.7.4 > > > > > > > > -- > > > > > > My initial reaction was "allow the SSP to do its > > > thing". However, > > > after some consideration, this is a much better way to handle > > > this > > > exceptional case when a user gives an explicit security type and > > > it > > > cannot be honored. +1, FWIW > > > My only concern is, "will this be considered a regression by > > > users > > > that have (unknowingly) relied upon the previous behavior?" > > > > > > > That is a valid concern which could trip up users who have been > > using > > an incorrect mount option. However in my opinion it would be better > > to > > reject mounts in case where the mount options requested are not > > available instead of silently switching the mount options and > > ending up > > with behaviour which wasn't expected by the user. > > > > Sachin Prabhu > > -- > > To unsubscribe from this list: send the line "unsubscribe linux- > > cifs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 6:17 AM, Sachin Prabhu <sprabhu@redhat.com> wrote: [snip] > My own opinion is that when a mount option is explicitly passed, it is > expected the option provided be used and fail if it cannot be used. I > think replacing a explicitly requested mount option with another can > lead to unexpected results and may also affect things activities like > testing. > > There are other mount options like "vers=" where cifs doesn't attempt > to replace an invalid protocol version with another. This in my opinion > is the correct behaviour. Allow me to play Devil's advocate, but with a bridge we'll have to cross if we enact this behavior... Quoth The (current-ish) Fine Manual page for mount.cifs (quoted in full for historical mail list reference) : " sec= Security mode. Allowed values are: · none - attempt to connection as a null user (no name) · krb5 - Use Kerberos version 5 authentication · krb5i - Use Kerberos authentication and forcibly enable packet signing · ntlm - Use NTLM password hashing · ntlmi - Use NTLM password hashing and force packet signing · ntlmv2 - Use NTLMv2 password hashing · ntlmv2i - Use NTLMv2 password hashing and force packet signing · ntlmssp - Use NTLMv2 password hashing encapsulated in Raw NTLMSSP message · ntlmsspi - Use NTLMv2 password hashing encapsulated in Raw NTLMSSP message, and force packet signing The default in mainline kernel versions prior to v3.8 was sec=ntlm. In v3.8, the default was changed to sec=ntlmssp. If the server requires signing during protocol negotiation, then it may be enabled automatically. Packet signing may also be enabled automatically if it's enabled in /proc/fs/cifs/SecurityFlags. " What is the defined behavior if I specify that I'd like any of the more current NTLM variants kerberos-5, explicitly, without packet signing but the server requires packet signing? Currently, we silently turn it on. Is the defined behavior to now loudly fail rather than enable signing? Especially since the default value changed in Linux-3.8. I'd argue that we already somewhat treat the "sec" option as a preference rather than a requirement. I would further argue, although not strongly, that if we want to go down this road, that we should accept multiple options and if NONE of them can be used, then we fail. Does anyone have a better idea, or is the consensus that this isn't as large of an issue (both technologically and from a user perspective) as I perceive it to be? Steve, I'll defer to you as well with my objection above noted. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
---------- Forwarded message ---------- From: Steve French <smfrench@gmail.com> Date: Wed, Jan 11, 2017 at 11:58 AM Subject: Re: [PATCH] SMB2: Enforce sec= mount option To: Scott Lovenberg <scott.lovenberg@gmail.com> Cc: Sachin Prabhu <sprabhu@redhat.com>, Germano Percossi <germano.percossi@citrix.com>, linux-cifs <linux-cifs@vger.kernel.org> There is a broader question here about signing (making sure we enforce it properly if signing is requested but not supported) but I think that ntlmv2 can map to ntlmssp/ntlmv2. Given that ntlmv2 is the level of password hash used in our implementation of ntlmssp - I am fine with allowing ntlmv2 as a synonym (we wouldn't distinguish gssapi encapsulation of ntlmssp as a distinct sec version either - what matters is the level of security). On the question of signing: 1) if the server requires signing we turn it on (without signing required on the client mount, we allow the server to choose) 2) if the client requires signing and the server doesn't support it we should fail since the client expects signing I wouldn't mind adding mount option that forces signing to be off no matter what the server requested but doubt it would be needed. I do agree about accepting a list of sec= options and would take a kernel patch for that, but our earlier decision was that this could be done easier in user space (mount.cifs) - ie retry with different sec= options if the user specified more than. On Wed, Jan 11, 2017 at 11:48 AM, Scott Lovenberg <scott.lovenberg@gmail.com> wrote: > > On Wed, Jan 11, 2017 at 6:17 AM, Sachin Prabhu <sprabhu@redhat.com> wrote: > [snip] > > My own opinion is that when a mount option is explicitly passed, it is > > expected the option provided be used and fail if it cannot be used. I > > think replacing a explicitly requested mount option with another can > > lead to unexpected results and may also affect things activities like > > testing. > > > > There are other mount options like "vers=" where cifs doesn't attempt > > to replace an invalid protocol version with another. This in my opinion > > is the correct behaviour. > > Allow me to play Devil's advocate, but with a bridge we'll have to > cross if we enact this behavior... Quoth The (current-ish) Fine Manual > page for mount.cifs (quoted in full for historical mail list > reference) : > " > sec= > Security mode. Allowed values are: > > · none - attempt to connection as a null user (no name) > · krb5 - Use Kerberos version 5 authentication > · krb5i - Use Kerberos authentication and forcibly enable > packet signing > · ntlm - Use NTLM password hashing > · ntlmi - Use NTLM password hashing and force packet signing > · ntlmv2 - Use NTLMv2 password hashing > · ntlmv2i - Use NTLMv2 password hashing and force packet signing > · ntlmssp - Use NTLMv2 password hashing encapsulated in > Raw NTLMSSP message > · ntlmsspi - Use NTLMv2 password hashing encapsulated in > Raw NTLMSSP message, and force packet signing > The default in mainline kernel versions prior to v3.8 was > sec=ntlm. In v3.8, the default was changed to sec=ntlmssp. > > If the server requires signing during protocol negotiation, > then it may be enabled automatically. Packet signing may also be > enabled > automatically if it's enabled in /proc/fs/cifs/SecurityFlags. > " > > What is the defined behavior if I specify that I'd like any of the > more current NTLM variants kerberos-5, explicitly, without packet > signing but the server requires packet signing? Currently, we > silently turn it on. Is the defined behavior to now loudly fail > rather than enable signing? Especially since the default value > changed in Linux-3.8. I'd argue that we already somewhat treat the > "sec" option as a preference rather than a requirement. I would > further argue, although not strongly, that if we want to go down this > road, that we should accept multiple options and if NONE of them can > be used, then we fail. Does anyone have a better idea, or is the > consensus that this isn't as large of an issue (both technologically > and from a user perspective) as I perceive it to be? > > Steve, I'll defer to you as well with my objection above noted.
On Tue, Jan 10, 2017 at 5:11 PM, L A Walsh <law@tlinx.org> wrote: > Sachin Prabhu wrote: >> >> If the security type specified using a mount option is not supported, >> the SMB2 session setup code changes the security type to RawNTLMSSP. We >> should instead fail the mount and return an error. >> > > --- > Saw the comment by Steve F, and it got me to thinking. > Please take this as a suggestion or idea... I'm not > heavily committed to a single solution, at this point, as > haven't really thought through all of the ramifications. > > Is it possible to add a 'prefix' or 'suffix', like an > "=" sign or a '+' -- to mean: > > '=' = exactly this 'sec' level > '+' = this 'sec'-level or greater > '<' = less than or equal to this sec-level > --- > Using the symbols is a similar idea to some fields in > 'find' where +/- are used to indicate greater or less than > the stated number. > > I'm not sure about the symbols, exactly, but I know in samba I > ask for smb2 for the protocol and more often than not, only > get smb1, but I'd rather have it work than fail. > > Since I'm on a closed net, I'd have to say the same for > security options, but I'd like to have a choice to force it > if I wanted to... > > Anyway -- just an idea that might offer more flexibility than just > 'fail'... > It'd take a tiny bit of messing with the command line parser, but I'd be for that. As a gesture of good faith, since I raised the issue, I'd be willing to submit the patch set for mount.cifs to support this if everyone is on board. I'd suggest staying away from '<' and '>' as they're shell redirects though. This would be a reasonable shorthand for a comma separated list (which also might take a bit of messing with the parser since we split on ',') - it could reasonably loop in the userland mount helper, mount.cifs, in much the same way Steve suggested that it should be handled in userland.
Some general thoughts:
1) users shouldn't have to know about the details of protocol
encapsulation, just security features that might matter to them
2) focus should be on SMB2.1 or later (SMB3 or later ideally), we can
leave cifs semantics alone.
3) warning messages are best printed in user space by mount.cifs
(since users don't often read kernel debug message logs)
4) there are few choices relevant for SMB2 and later
a) krb5 vs. ntlmv2
b) signing optional vs mandatory
- and for SMB3.1.1 AES-CCM vs. AES-GCM (see
https://jira.corp.primarydata.com/browse/PD-22499)
5) sane defaults that make it easier for the user with the most common
case (SMB3 or later supported by server) are preferred
My preference would be:
a) change smb2 and later behavior only (even limiting changes to
SMB3 or later is fine). No reason to change cifs and risk older
RHEL/SLES/Ubuntu users (and servers where support for LANMAN or NTLM
may have mattered)
b) allowing multiple dialects and/or multiple "sec=" is fine, but
probably easier to limit changes to mount.cifs and this is pretty
simple if we only have two real choices today.
c) changing default to allow no "sec=" to mean try ntlmv2 and krb5
(not sure which should be the default) is fine, but may be better to
do the retry in userspace
d) allow SMB3.1.1 choice for CCM vs. GCM when we have support for
these in cifs.ko
e) On invalid sec= choices for SMB2 and later e.g. "sec=lanman" or
"sec=ntlm" warn on these in userspace (in mount.cifs) - we can simply
strip them out before calling the kernel.
f) "sec=ntlmv2" is fine to leave in since I don't see any harm in
treating ntlmv2 and ntlmv2 in ntlmssp and ntlmv2 in ntlmssp in gssapi
as synonyms for SMB2/SMB3 (the user shouldn't have to know how
encapsulation of passwords is done) - and if we ever would have to
retry in kernel to attempt the two encapsulation methods (ntlmv2 in
ntlmssp vs. ntlmv2 in ntlmssp in gssapi) that is fine.
On Wed, Jan 11, 2017 at 3:02 PM, Scott Lovenberg
<scott.lovenberg@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 5:11 PM, L A Walsh <law@tlinx.org> wrote:
>> Sachin Prabhu wrote:
>>>
>>> If the security type specified using a mount option is not supported,
>>> the SMB2 session setup code changes the security type to RawNTLMSSP. We
>>> should instead fail the mount and return an error.
>>>
>>
>> ---
>> Saw the comment by Steve F, and it got me to thinking.
>> Please take this as a suggestion or idea... I'm not
>> heavily committed to a single solution, at this point, as
>> haven't really thought through all of the ramifications.
>>
>> Is it possible to add a 'prefix' or 'suffix', like an
>> "=" sign or a '+' -- to mean:
>>
>> '=' = exactly this 'sec' level
>> '+' = this 'sec'-level or greater
>> '<' = less than or equal to this sec-level
>> ---
>> Using the symbols is a similar idea to some fields in
>> 'find' where +/- are used to indicate greater or less than
>> the stated number.
>>
>> I'm not sure about the symbols, exactly, but I know in samba I
>> ask for smb2 for the protocol and more often than not, only
>> get smb1, but I'd rather have it work than fail.
>>
>> Since I'm on a closed net, I'd have to say the same for
>> security options, but I'd like to have a choice to force it
>> if I wanted to...
>>
>> Anyway -- just an idea that might offer more flexibility than just
>> 'fail'...
>>
>
> It'd take a tiny bit of messing with the command line parser, but I'd
> be for that. As a gesture of good faith, since I raised the issue,
> I'd be willing to submit the patch set for mount.cifs to support this
> if everyone is on board. I'd suggest staying away from '<' and '>' as
> they're shell redirects though. This would be a reasonable shorthand
> for a comma separated list (which also might take a bit of messing
> with the parser since we split on ',') - it could reasonably loop in
> the userland mount helper, mount.cifs, in much the same way Steve
> suggested that it should be handled in userland.
Scott Lovenberg wrote: > It'd take a tiny bit of messing with the command line parser, but I'd > be for that. As a gesture of good faith, since I raised the issue, > I'd be willing to submit the patch set for mount.cifs to support this > if everyone is on board. I'd suggest staying away from '<' and '>' --- Yeah, the '<' sent a shiver as I wrote it. find uses "-5" means '<=5', but the minus bothered me a bit, but perhaps less than the '<'. Was looking at samba's switches they allow specifying "min" and "max" for protocols, that could be written compactly with "proto=+smb1,-smb2.1", or any number of more verbose options. The options for auth+security seem a bit less regular than always specifying a range, like some options automatically disable lower security options -- I suppose specifying a 'max' isn't needed, since if once side offers a higher level that is too high for the other, the other can just NACK during the negotiation. Better get back to lurking now... ;-) -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-01-11 at 15:02 -0600, Scott Lovenberg wrote: > On Tue, Jan 10, 2017 at 5:11 PM, L A Walsh <law@tlinx.org> wrote: > > Sachin Prabhu wrote: > > > > > > If the security type specified using a mount option is not > > > supported, > > > the SMB2 session setup code changes the security type to > > > RawNTLMSSP. We > > > should instead fail the mount and return an error. > > > > > > > --- > > Saw the comment by Steve F, and it got me to thinking. > > Please take this as a suggestion or idea... I'm not > > heavily committed to a single solution, at this point, as > > haven't really thought through all of the ramifications. > > > > Is it possible to add a 'prefix' or 'suffix', like an > > "=" sign or a '+' -- to mean: > > > > '=' = exactly this 'sec' level > > '+' = this 'sec'-level or greater > > '<' = less than or equal to this sec-level > > --- > > Using the symbols is a similar idea to some fields in > > 'find' where +/- are used to indicate greater or less than > > the stated number. > > > > I'm not sure about the symbols, exactly, but I know in samba I > > ask for smb2 for the protocol and more often than not, only > > get smb1, but I'd rather have it work than fail. > > > > Since I'm on a closed net, I'd have to say the same for > > security options, but I'd like to have a choice to force it > > if I wanted to... > > > > Anyway -- just an idea that might offer more flexibility than just > > 'fail'... > > > > It'd take a tiny bit of messing with the command line parser, but I'd > be for that. As a gesture of good faith, since I raised the issue, > I'd be willing to submit the patch set for mount.cifs to support this > if everyone is on board. I'd suggest staying away from '<' and '>' > as > they're shell redirects though. This would be a reasonable shorthand > for a comma separated list (which also might take a bit of messing > with the parser since we split on ',') - it could reasonably loop in > the userland mount helper, mount.cifs, in much the same way Steve > suggested that it should be handled in userland. > I think the userland would be a good option to handle this as I suspect it may be much easier to recover from mount failures and to attempt a remount from userland. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 5ca5ea46..e66fad6 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -955,7 +955,8 @@ SMB2_sess_auth_rawntlmssp_authenticate(struct SMB2_sess_data *sess_data) static int SMB2_select_sec(struct cifs_ses *ses, struct SMB2_sess_data *sess_data) { - if (ses->sectype != Kerberos && ses->sectype != RawNTLMSSP) + /* Default sec type is set to RawNTLMSSP */ + if (ses->sectype == Unspecified) ses->sectype = RawNTLMSSP; switch (ses->sectype) {
If the security type specified using a mount option is not supported, the SMB2 session setup code changes the security type to RawNTLMSSP. We should instead fail the mount and return an error. Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> --- fs/cifs/smb2pdu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)