diff mbox

RFC: Revert move default dialect from CIFS to to SMB3"

Message ID 1504213298-27431-1-git-send-email-linux@leemhuis.info (mailing list archive)
State New, archived
Headers show

Commit Message

Thorsten Leemhuis Aug. 31, 2017, 9:01 p.m. UTC
This reverts commit eef914a9eb5eb83e60eb498315a491cd1edc13a1 (
[SMB3] Improve security, move default dialect to SMB3 from old CIFS), 
as it confuses users: https://bugzilla.kernel.org/show_bug.cgi?id=196599

It was a patch to improve security by switching to SMB3 by default and
support SMB1 (aka CIFS) only when explicitly requested, as the latter
is not considered secure anymore (see below for details). This is one of
the rare cases where regressions are unavoidable and accepted in Linux.
But that's bad enough already, so we at least should make it easy for
people to get an idea why something suddenly stopped working with a
newer kernel version. That's not the case, because due to eef914a9eb5e
a mount of a server that only supports CIFS/SMB1 with mount.cifs fails
with a misleading message:

> mount error(112): Host is down > Refer to the mount.cifs(8) manual
> page (e.g. man mount.cifs)

The corresponding message in the kernel log is just as unhelpful:

> CIFS VFS: cifs_mount failed w/return code = -112

This needs to be improved. Hence remove this for now, as the world won't
end suddenly if this gets delayed one or two cycles and resubmitted in
a way that leads to a more helpful error message.

For completeness, here are parts from the original patch description:

> Due to recent publicity about security vulnerabilities in the much
> older CIFS dialect, move the default dialect to the widely accepted
> (and quite secure) SMB3.0 dialect from the old default of the CIFS
> dialect.
>
> We do not want to be encouraging use of less secure dialects, and
> both Microsoft and CERT now strongly recommend not using the older
> CIFS dialect (SMB Security Best Practices "recommends disabling
> SMBv1").
>
> SMB3 is both secure and widely available: in Windows 8 and later,
> Samba and Macs.
>
> Users can still choose to explicitly mount with the less secure
> dialect (for old servers) by choosing "vers=1.0" on the cifs mount

Signed-off-by: Thorsten Leemhuis <linux@leemhuis.info>
CC: Steve French <smfrench@gmail.com>
CC: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/connect.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thorsten Leemhuis Aug. 31, 2017, 9:36 p.m. UTC | #1
Lo! To give a bit more background to this (the mail I reply to was the
first I sent with git send-email and I missed some details): Maybe I'm
over stretching my abilities/position as regression tracker with this
RFC for a revert, but I hope it at least triggers a discussion if such a
revert should be done or not.

The problem described in below patch description is known for a few
weeks already, but the bugzilla entry about it got ignored by the CIFS
developers; the same happened to a mail one affected user sent to
cifs-devel.

I should have send this revert RFC earlier, because I think more users
will be confused by the switch to SMB3, because the error messages they
get when trying to mount a SMB1/CIFS only share are not very helpful. I
myself had one system which regressed and made me look closer. And I
heard at least some widespread NAS boxes only support CIFS/SMB1 by
default :-/

Note: I only light-tested this revert and it worked fine for me (I still
could mount a CIFS/SMB1 only samba share and was still able to mount a
samba share on a different host with SMB3). But I'm not familiar with
the code I modify here. Hence someone who is (Steven?) IMHO should ACK
this before it gets applied.

Ciao, Thorsten


On 31.08.2017 23:01, Thorsten Leemhuis wrote:
> This reverts commit eef914a9eb5eb83e60eb498315a491cd1edc13a1 (
> [SMB3] Improve security, move default dialect to SMB3 from old CIFS), 
> as it confuses users: https://bugzilla.kernel.org/show_bug.cgi?id=196599
> 
> It was a patch to improve security by switching to SMB3 by default and
> support SMB1 (aka CIFS) only when explicitly requested, as the latter
> is not considered secure anymore (see below for details). This is one of
> the rare cases where regressions are unavoidable and accepted in Linux.
> But that's bad enough already, so we at least should make it easy for
> people to get an idea why something suddenly stopped working with a
> newer kernel version. That's not the case, because due to eef914a9eb5e
> a mount of a server that only supports CIFS/SMB1 with mount.cifs fails
> with a misleading message:
> 
>> mount error(112): Host is down > Refer to the mount.cifs(8) manual
>> page (e.g. man mount.cifs)
> 
> The corresponding message in the kernel log is just as unhelpful:
> 
>> CIFS VFS: cifs_mount failed w/return code = -112
> 
> This needs to be improved. Hence remove this for now, as the world won't
> end suddenly if this gets delayed one or two cycles and resubmitted in
> a way that leads to a more helpful error message.
> 
> For completeness, here are parts from the original patch description:
> 
>> Due to recent publicity about security vulnerabilities in the much
>> older CIFS dialect, move the default dialect to the widely accepted
>> (and quite secure) SMB3.0 dialect from the old default of the CIFS
>> dialect.
>>
>> We do not want to be encouraging use of less secure dialects, and
>> both Microsoft and CERT now strongly recommend not using the older
>> CIFS dialect (SMB Security Best Practices "recommends disabling
>> SMBv1").
>>
>> SMB3 is both secure and widely available: in Windows 8 and later,
>> Samba and Macs.
>>
>> Users can still choose to explicitly mount with the less secure
>> dialect (for old servers) by choosing "vers=1.0" on the cifs mount
> 
> Signed-off-by: Thorsten Leemhuis <linux@leemhuis.info>
> CC: Steve French <smfrench@gmail.com>
> CC: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>  fs/cifs/connect.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 59647eb..6ab261cd 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1272,9 +1272,9 @@ static int cifs_parse_security_flavors(char *value,
>  
>  	vol->actimeo = CIFS_DEF_ACTIMEO;
>  
> -	/* FIXME: add autonegotiation for SMB3 or later rather than just SMB3 */
> -	vol->ops = &smb30_operations; /* both secure and accepted widely */
> -	vol->vals = &smb30_values;
> +	/* FIXME: add autonegotiation -- for now, SMB1 is default */
> +	vol->ops = &smb1_operations;
> +	vol->vals = &smb1_values;
>  
>  	vol->echo_interval = SMB_ECHO_INTERVAL_DEFAULT;
>  
> 
--
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 Sept. 1, 2017, 12:03 a.m. UTC | #2
Thorsten Leemhuis wrote:
> This reverts commit eef914a9eb5eb83e60eb498315a491cd1edc13a1 (
> [SMB3] Improve security, move default dialect to SMB3 from old CIFS), 
> as it confuses users: https://bugzilla.kernel.org/show_bug.cgi?id=196599
>
> It was a patch to improve security by switching to SMB3 by default and
> support SMB1 (aka CIFS) only when explicitly requested, as the latter
> is not considered secure anymore (see below for details). This is one of
> the rare cases where regressions are unavoidable and accepted in Linux.
>   
----
    Why not SMB2.1?  Win7 is still in support and getting security updates.
MS has not issued any updates for Win7 upgrading it to SMB3.0 for any
reason (that I'm aware of) -- including security. 

    If there were security problems in Win7 w/SMB2.1, wouldn't MS
issue patches -- as they did for WinXP just recently for a severe
SMB1 bug?

    Seems like if they are willing to patch "out of support" XP, for
a serious problem, then they would be more likely to patch Win7 for
lesser problems.

    Seems like jumping the default to MS's latest and greatest puts
linux on MS's OS-release schedule -- especially when they haven't declared
SMB2.1 as "bad"...  From what I understand, most of the new security
features in 3.0 when into SMB2.1 or 2.0.

>> SMB3 is both secure and widely available: in Windows 8 and later,
>> Samba and Macs.
>>     
----
    I can't find more recent stats than last Dec, but Win7 had between
2-3X the number of Win8 users AND Win7 had between 40-100% more uses
than Win10.  Win 8 was pretty much a non-starter.
(http://www.zdnet.com/article/windows-10-versus-windows-7-whose-numbers-do-you-trust/)

As of March 2017, another article showed Win7 growing w/r/t Win10:
(https://www.theinquirer.net/inquirer/news/3005602/windows-7-market-share-rises-at-the-expense-of-windows-10)

    I can't say moving the default away from SMB1 seems like a bad thing 
-- especially if the error messages can be improved.  Besides security, its
notably slower, but many home devices still use SMB1 -- which is *fine*,
if they are not exposed to the outside net.  Then again, I've never
put a Windows machine facing the internet -- don't think they are security
enough -- use linux for that.


>
>   
--
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
Linus Torvalds Sept. 1, 2017, 12:12 a.m. UTC | #3
On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
> Lo! To give a bit more background to this (the mail I reply to was the
> first I sent with git send-email and I missed some details): Maybe I'm
> over stretching my abilities/position as regression tracker with this
> RFC for a revert, but I hope it at least triggers a discussion if such a
> revert should be done or not.

I don't think that a revert is appropriate.

But perhaps just a single printk() or something if the user does *not*
specify the version explicitly? Just saying something like

  We used to default to 1.0, we now default to 3.0, if you want old
defaults, use "vers=1.0"

Oh, looking at that version parsing code, I think we also need to fix
that legacy "ver=1" thing (ver without the 's') which now silently
ignores "ver=1" as being the "default", even though it's not.

I do *not* believe that "default to version 1" is acceptable.

                Linus
--
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
Steve French Sept. 1, 2017, 12:29 a.m. UTC | #4
Yes - updating the parsing slightly and printks as suggested makes sense

Some additional warning messages in the userspace helper (adding Jeff
Layton), mount.cifs can also help.

I also have an experimental set of patches to allow multi-dialect
negotiation with at least three of the acceptable dialects
(smb2.1/smb3/smb3.02) which will help, but complicate secure dialect
validation ("validate negotiate") but that will have to wait till next
release.

On Thu, Aug 31, 2017 at 7:12 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>> Lo! To give a bit more background to this (the mail I reply to was the
>> first I sent with git send-email and I missed some details): Maybe I'm
>> over stretching my abilities/position as regression tracker with this
>> RFC for a revert, but I hope it at least triggers a discussion if such a
>> revert should be done or not.
>
> I don't think that a revert is appropriate.
>
> But perhaps just a single printk() or something if the user does *not*
> specify the version explicitly? Just saying something like
>
>   We used to default to 1.0, we now default to 3.0, if you want old
> defaults, use "vers=1.0"
>
> Oh, looking at that version parsing code, I think we also need to fix
> that legacy "ver=1" thing (ver without the 's') which now silently
> ignores "ver=1" as being the "default", even though it's not.
>
> I do *not* believe that "default to version 1" is acceptable.
>
>                 Linus
Steve French Sept. 1, 2017, 2:42 a.m. UTC | #5
Any thoughts on this patch to add additional warnings for the user -
logging when using default dialects (or when server returns dialect
not supported), and noting the default dialect change?

See https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=bb86f22eeddbb5879675b55168b8fa8990d74a21

On Thu, Aug 31, 2017 at 7:29 PM, Steve French <smfrench@gmail.com> wrote:
> Yes - updating the parsing slightly and printks as suggested makes sense
>
> Some additional warning messages in the userspace helper (adding Jeff
> Layton), mount.cifs can also help.
>
> I also have an experimental set of patches to allow multi-dialect
> negotiation with at least three of the acceptable dialects
> (smb2.1/smb3/smb3.02) which will help, but complicate secure dialect
> validation ("validate negotiate") but that will have to wait till next
> release.
>
> On Thu, Aug 31, 2017 at 7:12 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>> Lo! To give a bit more background to this (the mail I reply to was the
>>> first I sent with git send-email and I missed some details): Maybe I'm
>>> over stretching my abilities/position as regression tracker with this
>>> RFC for a revert, but I hope it at least triggers a discussion if such a
>>> revert should be done or not.
>>
>> I don't think that a revert is appropriate.
>>
>> But perhaps just a single printk() or something if the user does *not*
>> specify the version explicitly? Just saying something like
>>
>>   We used to default to 1.0, we now default to 3.0, if you want old
>> defaults, use "vers=1.0"
>>
>> Oh, looking at that version parsing code, I think we also need to fix
>> that legacy "ver=1" thing (ver without the 's') which now silently
>> ignores "ver=1" as being the "default", even though it's not.
>>
>> I do *not* believe that "default to version 1" is acceptable.
>>
>>                 Linus
>
>
>
> --
> Thanks,
>
> Steve
ronnie sahlberg Sept. 1, 2017, 3:06 a.m. UTC | #6
Two missing \n in connect.c

Fix those and you can add a
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

On Fri, Sep 1, 2017 at 12:42 PM, Steve French <smfrench@gmail.com> wrote:
> Any thoughts on this patch to add additional warnings for the user -
> logging when using default dialects (or when server returns dialect
> not supported), and noting the default dialect change?
>
> See https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=bb86f22eeddbb5879675b55168b8fa8990d74a21
>
> On Thu, Aug 31, 2017 at 7:29 PM, Steve French <smfrench@gmail.com> wrote:
>> Yes - updating the parsing slightly and printks as suggested makes sense
>>
>> Some additional warning messages in the userspace helper (adding Jeff
>> Layton), mount.cifs can also help.
>>
>> I also have an experimental set of patches to allow multi-dialect
>> negotiation with at least three of the acceptable dialects
>> (smb2.1/smb3/smb3.02) which will help, but complicate secure dialect
>> validation ("validate negotiate") but that will have to wait till next
>> release.
>>
>> On Thu, Aug 31, 2017 at 7:12 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>>> Lo! To give a bit more background to this (the mail I reply to was the
>>>> first I sent with git send-email and I missed some details): Maybe I'm
>>>> over stretching my abilities/position as regression tracker with this
>>>> RFC for a revert, but I hope it at least triggers a discussion if such a
>>>> revert should be done or not.
>>>
>>> I don't think that a revert is appropriate.
>>>
>>> But perhaps just a single printk() or something if the user does *not*
>>> specify the version explicitly? Just saying something like
>>>
>>>   We used to default to 1.0, we now default to 3.0, if you want old
>>> defaults, use "vers=1.0"
>>>
>>> Oh, looking at that version parsing code, I think we also need to fix
>>> that legacy "ver=1" thing (ver without the 's') which now silently
>>> ignores "ver=1" as being the "default", even though it's not.
>>>
>>> I do *not* believe that "default to version 1" is acceptable.
>>>
>>>                 Linus
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>
>
>
> --
> Thanks,
>
> Steve
--
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
Andrew Bartlett Sept. 1, 2017, 3:11 a.m. UTC | #7
On Thu, 2017-08-31 at 17:03 -0700, L. A. Walsh wrote:
> Thorsten Leemhuis wrote:
> > This reverts commit eef914a9eb5eb83e60eb498315a491cd1edc13a1 (
> > [SMB3] Improve security, move default dialect to SMB3 from old
> > CIFS), 
> > as it confuses users: https://bugzilla.kernel.org/show_bug.cgi?id=1
> > 96599
> > 
> > It was a patch to improve security by switching to SMB3 by default
> > and
> > support SMB1 (aka CIFS) only when explicitly requested, as the
> > latter
> > is not considered secure anymore (see below for details). This is
> > one of
> > the rare cases where regressions are unavoidable and accepted in
> > Linux.
> >   
> 
> ----
>     Why not SMB2.1?  Win7 is still in support and getting security
> updates.
> MS has not issued any updates for Win7 upgrading it to SMB3.0 for any
> reason (that I'm aware of) -- including security. 
> 
>     If there were security problems in Win7 w/SMB2.1, wouldn't MS
> issue patches -- as they did for WinXP just recently for a severe
> SMB1 bug?

To be clear, the issue with SMB1 is lack of integrity protection, in
particular on the negotiation, where a client may come to think it had
to use NTLM(v2) to talk to the server. 

The only protection available on SMB1 is 'smb signing', which is after
the negotiation of the authentication protocols etc.

>     Seems like if they are willing to patch "out of support" XP, for
> a serious problem, then they would be more likely to patch Win7 for
> lesser problems.
> 
>     Seems like jumping the default to MS's latest and greatest puts
> linux on MS's OS-release schedule -- especially when they haven't
> declared
> SMB2.1 as "bad"...  From what I understand, most of the new security
> features in 3.0 when into SMB2.1 or 2.0.

Sadly 'most' turned out not to be good enough to actually secure the
negotiation, which is the main weakness.   If a change is to be made, I
think it should move up to SMB3. 

In terms of Windows versions, Windows 2012R2 is quite popular these
days, even if Windows 8.1 still wasn't a great hit. 

Finally, I do agree that the move from SMB1 is partly on the basis of a
false premise.  It appears that Microsoft's recent declaration of SMB1
as 'bad' is as much on the basis of coding flaws in their own SMB1
implementation as the lack of protection.  It is hoped that more modern
code might have less buffer-overflow style security flaws compared with
the 25 year old stuff.

Both however are good reasons to move off the SMB1 protocol towards
SMB3 on the server, and this is easiest and safest to do once the
clients also move to SMB3 by default.

Andrew Bartlett
Jeff Layton Sept. 1, 2017, 11:07 a.m. UTC | #8
On Thu, 2017-08-31 at 21:42 -0500, Steve French wrote:
> Any thoughts on this patch to add additional warnings for the user -
> logging when using default dialects (or when server returns dialect
> not supported), and noting the default dialect change?
> 
> See https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=bb86f22eeddbb5879675b55168b8fa8990d74a21
> 

Breaking backward compatability sucks, but I agree that there's no real
alternative here. SMB1 is just not a safe default these days.

> On Thu, Aug 31, 2017 at 7:29 PM, Steve French <smfrench@gmail.com> wrote:
> > Yes - updating the parsing slightly and printks as suggested makes sense
> > 
> > Some additional warning messages in the userspace helper (adding Jeff
> > Layton), mount.cifs can also help.
> > 

What do you suggest here?

> > I also have an experimental set of patches to allow multi-dialect
> > negotiation with at least three of the acceptable dialects
> > (smb2.1/smb3/smb3.02) which will help, but complicate secure dialect
> > validation ("validate negotiate") but that will have to wait till next
> > release.
> > 

That seems like the best way to fix this. If you fail to negotiate any
dialect, throw a warning then.


> > On Thu, Aug 31, 2017 at 7:12 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
> > > > Lo! To give a bit more background to this (the mail I reply to was the
> > > > first I sent with git send-email and I missed some details): Maybe I'm
> > > > over stretching my abilities/position as regression tracker with this
> > > > RFC for a revert, but I hope it at least triggers a discussion if such a
> > > > revert should be done or not.
> > > 
> > > I don't think that a revert is appropriate.
> > > 
> > > But perhaps just a single printk() or something if the user does *not*
> > > specify the version explicitly? Just saying something like
> > > 
> > >   We used to default to 1.0, we now default to 3.0, if you want old
> > > defaults, use "vers=1.0"
> > > 
> > > Oh, looking at that version parsing code, I think we also need to fix
> > > that legacy "ver=1" thing (ver without the 's') which now silently
> > > ignores "ver=1" as being the "default", even though it's not.
> > > 
> > > I do *not* believe that "default to version 1" is acceptable.
> > > 
> > >                 Linus
> > 
> > 
> > 
> > --
> > Thanks,
> > 
> > Steve
> 
> 
>
L. A. Walsh Sept. 1, 2017, 6:23 p.m. UTC | #9
Linus Torvalds wrote:
> On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>   
>> Lo! To give a bit more background to this (the mail I reply to was the
>> first I sent with git send-email and I missed some details): Maybe I'm
>> over stretching my abilities/position as regression tracker with this
>> RFC for a revert, but I hope it at least triggers a discussion if such a
>> revert should be done or not.
>>     
>
> I don't think that a revert is appropriate.
>
> But perhaps just a single printk() or something if the user does *not*
> specify the version explicitly? Just saying something like
>
>   We used to default to 1.0, we now default to 3.0, if you want old
> defaults, use "vers=1.0"
>   
----
    Why be incompatible with the majority of Windows installations?
I.e.  If you really want to up security from 1.0 (not adverse to that),
then why not go to 2.1 as used by Win7?  Win7 is still in support
from MS -- and they haven't indicated a need to upgrade to 3.x for
security reasons.  3.x may have new security features, no argument, but
that doesn't mean 2.1, is insecure.


> I do *not* believe that "default to version 1" is acceptable.
>   
---
    But does it have to jump to 3?  I.e. Why not go a more middle
route of 2.1 -- as it is still security-supported by MS.  Ideally
MS would find some bug in 2.1 and allow 3.x to be an upgrade to Win7,
but until then...

Linda
--
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
Linus Torvalds Sept. 1, 2017, 7:45 p.m. UTC | #10
On Fri, Sep 1, 2017 at 11:23 AM, L. A. Walsh <linux-cifs@tlinx.org> wrote:
>    Why be incompatible with the majority of Windows installations?
> I.e.  If you really want to up security from 1.0 (not adverse to that),
> then why not go to 2.1 as used by Win7?  Win7 is still in support
> from MS -- and they haven't indicated a need to upgrade to 3.x for
> security reasons.  3.x may have new security features, no argument, but
> that doesn't mean 2.1, is insecure.

I'm certainly ok with changing the default to 2.1 if that helps people.

Is that actually likely to help the people who now see problems with
the existing 3.0 default?

I don't know the exact security issue details with cifs, but I _think_
it was explicitly _only_ smb-1.0, right?

                   Linus
--
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
Steve French Sept. 2, 2017, 2:16 a.m. UTC | #11
On Fri, Sep 1, 2017 at 2:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Sep 1, 2017 at 11:23 AM, L. A. Walsh <linux-cifs@tlinx.org> wrote:
>>    Why be incompatible with the majority of Windows installations?
>> I.e.  If you really want to up security from 1.0 (not adverse to that),
>> then why not go to 2.1 as used by Win7?  Win7 is still in support
>> from MS -- and they haven't indicated a need to upgrade to 3.x for
>> security reasons.  3.x may have new security features, no argument, but
>> that doesn't mean 2.1, is insecure.
>
> I'm certainly ok with changing the default to 2.1 if that helps people.
>
> Is that actually likely to help the people who now see problems with
> the existing 3.0 default?
>
> I don't know the exact security issue details with cifs, but I _think_
> it was explicitly _only_ smb-1.0, right?

The default was SMB1 (CIFS) and was recently changed to SMB3.
The dialect still can be overridden by specifying "vers=1.0" or "vers=2.1"
etc. on mount.

We just put together a patch to better explain the default changes
(with additional warning messages) as suggested.

SMB3 is significantly better than SMB2.1 (supporting encrypted shares
and sessions for example, and requiring support for "secure negotiate")
and some servers require SMB3 minimum as a result, but it was agreed
at the last test event to eventually support multi-dialect negotiation (for
which SMB2.1 e.g. Windows 7, would be the oldest and least secure
dialect we would support) but in this interim stage we had to pick one,
and the improvements in SMB3 (over SMB2.1) tipped the balance.

In 4.14 we will likely have the ability to more securely do multi-dialect
negotiation, and this issue of SMB2.1 vs. SMB3 will be moot as the
server will choose its most recent dialect.
Linus Torvalds Sept. 2, 2017, 3:56 a.m. UTC | #12
On Fri, Sep 1, 2017 at 7:16 PM, Steve French <smfrench@gmail.com> wrote:
>
> The default was SMB1 (CIFS) and was recently changed to SMB3.
> The dialect still can be overridden by specifying "vers=1.0" or "vers=2.1"
> etc. on mount.
>
> We just put together a patch to better explain the default changes
> (with additional warning messages) as suggested.
>
> SMB3 is significantly better than SMB2.1 (supporting encrypted shares
> and sessions for example, and requiring support for "secure negotiate")
> and some servers require SMB3 minimum as a result,

The default shouldn't be about "best and most secure", but "most
convenient, while still not actively *IN*secure"

So "some servers require 3.0" may be true, but if it's also the case
that "most servers still don't do 3.0 at all", then it's a "some" vs
"most".

Which is the most common one? That should be the default.

I realize that eventually we'll have auto-negotiation, but that's
clearly not for 4.13. So in the meantime the only issue is what the
right default should be without auto-negotiation.

So it should be about what the failure rate is. If trying for smb3 has
a high failure rate because people simply don't have that yet, then
making that the default was clearly the wrong choice.

Because being "better" is immaterial if it doesn't work.

              Linus
--
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
Andrew Bartlett Sept. 2, 2017, 5:22 a.m. UTC | #13
On Fri, 2017-09-01 at 20:56 -0700, Linus Torvalds wrote:
> On Fri, Sep 1, 2017 at 7:16 PM, Steve French <smfrench@gmail.com> wrote:
> > 
> > The default was SMB1 (CIFS) and was recently changed to SMB3.
> > The dialect still can be overridden by specifying "vers=1.0" or "vers=2.1"
> > etc. on mount.
> > 
> > We just put together a patch to better explain the default changes
> > (with additional warning messages) as suggested.
> > 
> > SMB3 is significantly better than SMB2.1 (supporting encrypted shares
> > and sessions for example, and requiring support for "secure negotiate")
> > and some servers require SMB3 minimum as a result,
> 
> The default shouldn't be about "best and most secure", but "most
> convenient, while still not actively *IN*secure"
> 
> So "some servers require 3.0" may be true, but if it's also the case
> that "most servers still don't do 3.0 at all", then it's a "some" vs
> "most".
> 
> Which is the most common one? That should be the default.
> 
> I realize that eventually we'll have auto-negotiation, but that's
> clearly not for 4.13. So in the meantime the only issue is what the
> right default should be without auto-negotiation.
> 
> So it should be about what the failure rate is. If trying for smb3 has
> a high failure rate because people simply don't have that yet, then
> making that the default was clearly the wrong choice.
> 
> Because being "better" is immaterial if it doesn't work.

My quick research shows:

SMB 2.1 but not SMB3 is on:
 Windows 7
 Windows 8
 Windows 2008
 Windows 2012
 Samba 3.6 and earlier (SMB1 only by default)

SMB3 is on:
 Windows 8.1
 Windows 2012 R2
 Windows 10
 Windows 2016 
 Samba 4.0 and above (released 2012)

I'm not sure exactly which Mac versions.  

Some breakage will be old (and quite recent) NAS and routers that use
SMB1 and often very old Samba, but most of those only do SMB1. 

In terms of 'actively *IN*secure', it really depends what you mean by
that.  

That is, like all big changes, the movement against SMB1 has had
multiple motivations:
 - a desire no longer to expose really old code in Windows (recently
exploited)
 - a desire to retire an old and complex protocol
 - SMB3 having proper secure negotiation (I'll leave it to Steve to
explain the difference between SMB2 and 3 in that respect, I'm not so
current on the details). 

I hope this help,

Andrew Bartlett
Thorsten Leemhuis Sept. 2, 2017, 2:25 p.m. UTC | #14
Hi! Just a quick feedback from my side.

After reading Andrew explanation in this thread about the "movement
against SMB1" I kind of think "maybe the proposed revert for 4.13 and
doing it properly in 4.14 would really have been a good fit". But
whatever, doesn't bother me much any more:

Steve French wrote on 01.09.2017 04:42:
> Any thoughts on this patch to add additional warnings for the user -
> logging when using default dialects (or when server returns dialect
> not supported), and noting the default dialect change?
> See https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=bb86f22eeddbb5879675b55168b8fa8990d74a21

I noticed Linus committed a sightly updated variant earlier today
(https://git.kernel.org/torvalds/c/7e682f766f28 ). I just gave it a
quick try and it worked well. I can still mount smb3 shares. For
cifs/smb1 shares mount.cifs obviously still fails with the confusing
error message. But at least one gets a better explanation in dmesg now.

Many thx for this! Ciao, Thorsten.

P.S.: For the curious reader (and search engines!), this is the
confusing mount message you'll get when trying to access a CIFS/SMB1
only share with Linux master currently:

mount error(112): Host is down
Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)"

And this is the warning you see in dmesg now when not specifying
"vers=1.0" as option (-o) to mount/mount.cifs:

No dialect specified on mount. Default has changed to a
more secure dialect, SMB3 (vers=3.0), from CIFS (SMB1). To use the less
secure SMB1 dialect to access old servers which do not support SMB3
specify vers=1.0 on mount. For somewhat newer servers such as Windows 7
try vers=2.1.
CIFS VFS: cifs_mount failed w/return code = -112


> On Thu, Aug 31, 2017 at 7:29 PM, Steve French <smfrench@gmail.com> wrote:
>> Yes - updating the parsing slightly and printks as suggested makes sense
>>
>> Some additional warning messages in the userspace helper (adding Jeff
>> Layton), mount.cifs can also help.
>>
>> I also have an experimental set of patches to allow multi-dialect
>> negotiation with at least three of the acceptable dialects
>> (smb2.1/smb3/smb3.02) which will help, but complicate secure dialect
>> validation ("validate negotiate") but that will have to wait till next
>> release.
>>
>> On Thu, Aug 31, 2017 at 7:12 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> On Thu, Aug 31, 2017 at 2:36 PM, Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>>> Lo! To give a bit more background to this (the mail I reply to was the
>>>> first I sent with git send-email and I missed some details): Maybe I'm
>>>> over stretching my abilities/position as regression tracker with this
>>>> RFC for a revert, but I hope it at least triggers a discussion if such a
>>>> revert should be done or not.
>>>
>>> I don't think that a revert is appropriate.
>>>
>>> But perhaps just a single printk() or something if the user does *not*
>>> specify the version explicitly? Just saying something like
>>>
>>>   We used to default to 1.0, we now default to 3.0, if you want old
>>> defaults, use "vers=1.0"
>>>
>>> Oh, looking at that version parsing code, I think we also need to fix
>>> that legacy "ver=1" thing (ver without the 's') which now silently
>>> ignores "ver=1" as being the "default", even though it's not.
>>>
>>> I do *not* believe that "default to version 1" is acceptable.
>>>
>>>                 Linus
--
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
Linus Torvalds Sept. 2, 2017, 5:09 p.m. UTC | #15
On Fri, Sep 1, 2017 at 10:22 PM, Andrew Bartlett <abartlet@samba.org> wrote:
>
> My quick research shows:
>
> SMB 2.1 but not SMB3 is on:
>  Windows 7
>  Windows 8
>  Windows 2008
>  Windows 2012
>  Samba 3.6 and earlier (SMB1 only by default)
>
> SMB3 is on:
>  Windows 8.1
>  Windows 2012 R2
>  Windows 10
>  Windows 2016
>  Samba 4.0 and above (released 2012)

But most, if not all, of those SMB3 cases _also_ support SMB2.1,
right?  So the "3.0 _only_" case ends up being a fairly rare case
where things have been explicitly limited, and any previous Linux use
must have had that explicit "vers=3.0" flag anyway?

No?

Anyway, we can't avoid *some* breakage (ie the places that literally
only support 1.0 will have to add the explicit "vers=1.0" to get the
mount).

And I merged the code to add better error reporting yesterday, so
hopefully regardless of the default we choose the breakage is not
nearly as confusing to people any more.

                      Linus
--
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 mbox

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 59647eb..6ab261cd 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1272,9 +1272,9 @@  static int cifs_parse_security_flavors(char *value,
 
 	vol->actimeo = CIFS_DEF_ACTIMEO;
 
-	/* FIXME: add autonegotiation for SMB3 or later rather than just SMB3 */
-	vol->ops = &smb30_operations; /* both secure and accepted widely */
-	vol->vals = &smb30_values;
+	/* FIXME: add autonegotiation -- for now, SMB1 is default */
+	vol->ops = &smb1_operations;
+	vol->vals = &smb1_values;
 
 	vol->echo_interval = SMB_ECHO_INTERVAL_DEFAULT;