[PATCH] SMB2: Enforce sec= mount option

[PATCH] SMB2: Enforce sec= mount option

[Sachin Prabhu]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

Re: [PATCH] SMB2: Enforce sec= mount option

[Scott Lovenberg]

On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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?"

-- 
Peace and Blessings,
-Scott.

Re: [PATCH] SMB2: Enforce sec= mount option

[Sachin Prabhu]

On Thu, 2016-12-08 at 02:06 -0600, Scott Lovenberg wrote:
> On Thu, Dec 8, 2016 at 12:46 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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

Re: [PATCH] SMB2: Enforce sec= mount option

[Scott Lovenberg]

On Thu, Dec 8, 2016 at 3:03 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  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.

-- 
Peace and Blessings,
-Scott.

Re: [PATCH] SMB2: Enforce sec= mount option

[Germano Percossi]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] SMB2: Enforce sec= mount option

[Sachin Prabhu]

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-H+wXaHxf7aJhl2p70BpVqQ@public.gmane.org
> > > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH] SMB2: Enforce sec= mount option

[Scott Lovenberg]

On Wed, Jan 11, 2017 at 6:17 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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.

Fwd: [PATCH] SMB2: Enforce sec= mount option

[Steve French]

---------- Forwarded message ----------
From: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Wed, Jan 11, 2017 at 11:58 AM
Subject: Re: [PATCH] SMB2: Enforce sec= mount option
To: Scott Lovenberg <scott.lovenberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Germano Percossi
<germano.percossi-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>, linux-cifs <linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Wed, Jan 11, 2017 at 6:17 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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.




-- 
Thanks,

Steve



-- 
Thanks,

Steve

Re: SMB2: Enforce sec= mount option

[L A Walsh]

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'...

Re: SMB2: Enforce sec= mount option

[Aurélien Aptel]

L A Walsh <law-gT3AUAsYRbTYtjvyW6yDsg@public.gmane.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).

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: SMB2: Enforce sec= mount option

[Scott Lovenberg]

On Tue, Jan 10, 2017 at 5:11 PM, L A Walsh <law-gT3AUAsYRbTYtjvyW6yDsg@public.gmane.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.

-- 
Peace and Blessings,
-Scott.

Re: SMB2: Enforce sec= mount option

[Steve French]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jan 10, 2017 at 5:11 PM, L A Walsh <law-gT3AUAsYRbTYtjvyW6yDsg@public.gmane.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.




-- 
Thanks,

Steve

Re: SMB2: Enforce sec= mount option

[L A Walsh]

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... ;-)

Re: SMB2: Enforce sec= mount option

[Sachin Prabhu]

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-gT3AUAsYRbTYtjvyW6yDsg@public.gmane.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.

Re: SMB2: Enforce sec= mount option

[L A Walsh]

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'...

Fwd: [PATCH] SMB2: Enforce sec= mount option

[Steve French]

---------- Forwarded message ----------
From: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Tue, Jan 10, 2017 at 2:21 PM
Subject: Re: [PATCH] SMB2: Enforce sec= mount option
To: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: linux-cifs <linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




-- 
Thanks,

Steve



-- 
Thanks,

Steve