| Message ID | CAH2r5mtPAwURZxTeRBfn+c=hNyeaDbmKbYXt=w3VkUu5i1cxNQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 23 Nov 2012 17:36:45 -0600 Steve French <smfrench@gmail.com> wrote: > This patch to upgrade the default security mechanism to ntlmv2/ntlmssp > (which is broadly supported for years now, and a reasonable minimum, > far better than ntlm) is overdue, but I had to rework it to simplify > it. > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 5c670b9..3bca289 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -1103,6 +1103,7 @@ cifs_parse_mount_options(const char *mountdata, > const char *devname, > bool uid_specified = false; > bool gid_specified = false; > bool sloppy = false; > + bool sec_explicitly_set = false; > char *invalid = NULL; > char *nodename = utsname()->nodename; > char *string = NULL; > @@ -1763,6 +1764,7 @@ cifs_parse_mount_options(const char *mountdata, > const char *devname, > > if (cifs_parse_security_flavors(string, vol) != 0) > goto cifs_parse_mount_err; > + sec_explicitly_set = true; > break; > case Opt_cache: > string = match_strdup(args); > @@ -1799,6 +1801,8 @@ cifs_parse_mount_options(const char *mountdata, > const char *devname, > goto cifs_parse_mount_err; > } > #endif > + if (sec_explicitly_set == false) > + vol->secFlg |= CIFSSEC_MAY_NTLMSSP; > > if (vol->UNCip == NULL) > vol->UNCip = &vol->UNC[2]; > @@ -2397,8 +2401,6 @@ cifs_set_cifscreds(struct smb_vol *vol > __attribute__((unused)), > } > #endif /* CONFIG_KEYS */ > > -static bool warned_on_ntlm; /* globals init to false automatically */ > - > static struct cifs_ses * > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) > { > @@ -2475,14 +2477,6 @@ cifs_get_smb_ses(struct TCP_Server_Info > *server, struct smb_vol *volume_info) > ses->cred_uid = volume_info->cred_uid; > ses->linux_uid = volume_info->linux_uid; > > - /* ntlmv2 is much stronger than ntlm security, and has been broadly > - supported for many years, time to update default security mechanism */ > - if ((volume_info->secFlg == 0) && warned_on_ntlm == false) { > - warned_on_ntlm = true; > - cERROR(1, "default security mechanism requested. The default " > - "security mechanism will be upgraded from ntlm to " > - "ntlmv2 in kernel release 3.3"); > - } > ses->overrideSecFlg = volume_info->secFlg; > > mutex_lock(&ses->session_mutex); > How does this change the SecurityFlags interface?
it doesn't change security flags - but it seemed the smallest and safest since it basically says: 1) if you pass in "sec=" then use that 2) otherwise use ntlmssp (with ntlmv2) so shouldn't have any unintended consequences (and the sign mount option should work as expected as well) On Fri, Nov 23, 2012 at 7:41 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Fri, 23 Nov 2012 17:36:45 -0600 > Steve French <smfrench@gmail.com> wrote: > >> This patch to upgrade the default security mechanism to ntlmv2/ntlmssp >> (which is broadly supported for years now, and a reasonable minimum, >> far better than ntlm) is overdue, but I had to rework it to simplify >> it. >> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index 5c670b9..3bca289 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -1103,6 +1103,7 @@ cifs_parse_mount_options(const char *mountdata, >> const char *devname, >> bool uid_specified = false; >> bool gid_specified = false; >> bool sloppy = false; >> + bool sec_explicitly_set = false; >> char *invalid = NULL; >> char *nodename = utsname()->nodename; >> char *string = NULL; >> @@ -1763,6 +1764,7 @@ cifs_parse_mount_options(const char *mountdata, >> const char *devname, >> >> if (cifs_parse_security_flavors(string, vol) != 0) >> goto cifs_parse_mount_err; >> + sec_explicitly_set = true; >> break; >> case Opt_cache: >> string = match_strdup(args); >> @@ -1799,6 +1801,8 @@ cifs_parse_mount_options(const char *mountdata, >> const char *devname, >> goto cifs_parse_mount_err; >> } >> #endif >> + if (sec_explicitly_set == false) >> + vol->secFlg |= CIFSSEC_MAY_NTLMSSP; >> >> if (vol->UNCip == NULL) >> vol->UNCip = &vol->UNC[2]; >> @@ -2397,8 +2401,6 @@ cifs_set_cifscreds(struct smb_vol *vol >> __attribute__((unused)), >> } >> #endif /* CONFIG_KEYS */ >> >> -static bool warned_on_ntlm; /* globals init to false automatically */ >> - >> static struct cifs_ses * >> cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) >> { >> @@ -2475,14 +2477,6 @@ cifs_get_smb_ses(struct TCP_Server_Info >> *server, struct smb_vol *volume_info) >> ses->cred_uid = volume_info->cred_uid; >> ses->linux_uid = volume_info->linux_uid; >> >> - /* ntlmv2 is much stronger than ntlm security, and has been broadly >> - supported for many years, time to update default security mechanism */ >> - if ((volume_info->secFlg == 0) && warned_on_ntlm == false) { >> - warned_on_ntlm = true; >> - cERROR(1, "default security mechanism requested. The default " >> - "security mechanism will be upgraded from ntlm to " >> - "ntlmv2 in kernel release 3.3"); >> - } >> ses->overrideSecFlg = volume_info->secFlg; >> >> mutex_lock(&ses->session_mutex); >> > > How does this change the SecurityFlags interface? > > -- > Jeff Layton <jlayton@redhat.com>
changing #define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLM | CIFSSEC_MAY_NTLMV2 | CIFSSEC_MAY_NTLMSSP) to #define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLMSSP) affects more code On Fri, Nov 23, 2012 at 8:48 PM, Steve French <smfrench@gmail.com> wrote: > it doesn't change security flags - but it seemed the smallest and > safest since it basically says: > 1) if you pass in "sec=" then use that > 2) otherwise use ntlmssp (with ntlmv2) > > so shouldn't have any unintended consequences (and the sign mount > option should work as expected as well) > > On Fri, Nov 23, 2012 at 7:41 PM, Jeff Layton <jlayton@redhat.com> wrote: >> On Fri, 23 Nov 2012 17:36:45 -0600 >> Steve French <smfrench@gmail.com> wrote: >> >>> This patch to upgrade the default security mechanism to ntlmv2/ntlmssp >>> (which is broadly supported for years now, and a reasonable minimum, >>> far better than ntlm) is overdue, but I had to rework it to simplify >>> it. >>> >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >>> index 5c670b9..3bca289 100644 >>> --- a/fs/cifs/connect.c >>> +++ b/fs/cifs/connect.c >>> @@ -1103,6 +1103,7 @@ cifs_parse_mount_options(const char *mountdata, >>> const char *devname, >>> bool uid_specified = false; >>> bool gid_specified = false; >>> bool sloppy = false; >>> + bool sec_explicitly_set = false; >>> char *invalid = NULL; >>> char *nodename = utsname()->nodename; >>> char *string = NULL; >>> @@ -1763,6 +1764,7 @@ cifs_parse_mount_options(const char *mountdata, >>> const char *devname, >>> >>> if (cifs_parse_security_flavors(string, vol) != 0) >>> goto cifs_parse_mount_err; >>> + sec_explicitly_set = true; >>> break; >>> case Opt_cache: >>> string = match_strdup(args); >>> @@ -1799,6 +1801,8 @@ cifs_parse_mount_options(const char *mountdata, >>> const char *devname, >>> goto cifs_parse_mount_err; >>> } >>> #endif >>> + if (sec_explicitly_set == false) >>> + vol->secFlg |= CIFSSEC_MAY_NTLMSSP; >>> >>> if (vol->UNCip == NULL) >>> vol->UNCip = &vol->UNC[2]; >>> @@ -2397,8 +2401,6 @@ cifs_set_cifscreds(struct smb_vol *vol >>> __attribute__((unused)), >>> } >>> #endif /* CONFIG_KEYS */ >>> >>> -static bool warned_on_ntlm; /* globals init to false automatically */ >>> - >>> static struct cifs_ses * >>> cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) >>> { >>> @@ -2475,14 +2477,6 @@ cifs_get_smb_ses(struct TCP_Server_Info >>> *server, struct smb_vol *volume_info) >>> ses->cred_uid = volume_info->cred_uid; >>> ses->linux_uid = volume_info->linux_uid; >>> >>> - /* ntlmv2 is much stronger than ntlm security, and has been broadly >>> - supported for many years, time to update default security mechanism */ >>> - if ((volume_info->secFlg == 0) && warned_on_ntlm == false) { >>> - warned_on_ntlm = true; >>> - cERROR(1, "default security mechanism requested. The default " >>> - "security mechanism will be upgraded from ntlm to " >>> - "ntlmv2 in kernel release 3.3"); >>> - } >>> ses->overrideSecFlg = volume_info->secFlg; >>> >>> mutex_lock(&ses->session_mutex); >>> >> >> How does this change the SecurityFlags interface? >> >> -- >> Jeff Layton <jlayton@redhat.com> > > > > -- > Thanks, > > Steve
On Fri, Nov 23, 2012 at 8:52 PM, Steve French <smfrench@gmail.com> wrote: > changing > #define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLM | > CIFSSEC_MAY_NTLMV2 | CIFSSEC_MAY_NTLMSSP) > > to > > #define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLMSSP) > > affects more code It does seem to work - global_secflag is touched in more places, but looks safe enough as an alternative. Do you prefer the other change? > On Fri, Nov 23, 2012 at 8:48 PM, Steve French <smfrench@gmail.com> wrote: >> it doesn't change security flags - but it seemed the smallest and >> safest since it basically says: >> 1) if you pass in "sec=" then use that >> 2) otherwise use ntlmssp (with ntlmv2) >> >> so shouldn't have any unintended consequences (and the sign mount >> option should work as expected as well) >> >> On Fri, Nov 23, 2012 at 7:41 PM, Jeff Layton <jlayton@redhat.com> wrote: >>> On Fri, 23 Nov 2012 17:36:45 -0600 >>> Steve French <smfrench@gmail.com> wrote: >>> >>>> This patch to upgrade the default security mechanism to ntlmv2/ntlmssp >>>> (which is broadly supported for years now, and a reasonable minimum, >>>> far better than ntlm) is overdue, but I had to rework it to simplify >>>> it. >>>> >>>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >>>> index 5c670b9..3bca289 100644 >>>> --- a/fs/cifs/connect.c >>>> +++ b/fs/cifs/connect.c >>>> @@ -1103,6 +1103,7 @@ cifs_parse_mount_options(const char *mountdata, >>>> const char *devname, >>>> bool uid_specified = false; >>>> bool gid_specified = false; >>>> bool sloppy = false; >>>> + bool sec_explicitly_set = false; >>>> char *invalid = NULL; >>>> char *nodename = utsname()->nodename; >>>> char *string = NULL; >>>> @@ -1763,6 +1764,7 @@ cifs_parse_mount_options(const char *mountdata, >>>> const char *devname, >>>> >>>> if (cifs_parse_security_flavors(string, vol) != 0) >>>> goto cifs_parse_mount_err; >>>> + sec_explicitly_set = true; >>>> break; >>>> case Opt_cache: >>>> string = match_strdup(args); >>>> @@ -1799,6 +1801,8 @@ cifs_parse_mount_options(const char *mountdata, >>>> const char *devname, >>>> goto cifs_parse_mount_err; >>>> } >>>> #endif >>>> + if (sec_explicitly_set == false) >>>> + vol->secFlg |= CIFSSEC_MAY_NTLMSSP; >>>> >>>> if (vol->UNCip == NULL) >>>> vol->UNCip = &vol->UNC[2]; >>>> @@ -2397,8 +2401,6 @@ cifs_set_cifscreds(struct smb_vol *vol >>>> __attribute__((unused)), >>>> } >>>> #endif /* CONFIG_KEYS */ >>>> >>>> -static bool warned_on_ntlm; /* globals init to false automatically */ >>>> - >>>> static struct cifs_ses * >>>> cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) >>>> { >>>> @@ -2475,14 +2477,6 @@ cifs_get_smb_ses(struct TCP_Server_Info >>>> *server, struct smb_vol *volume_info) >>>> ses->cred_uid = volume_info->cred_uid; >>>> ses->linux_uid = volume_info->linux_uid; >>>> >>>> - /* ntlmv2 is much stronger than ntlm security, and has been broadly >>>> - supported for many years, time to update default security mechanism */ >>>> - if ((volume_info->secFlg == 0) && warned_on_ntlm == false) { >>>> - warned_on_ntlm = true; >>>> - cERROR(1, "default security mechanism requested. The default " >>>> - "security mechanism will be upgraded from ntlm to " >>>> - "ntlmv2 in kernel release 3.3"); >>>> - } >>>> ses->overrideSecFlg = volume_info->secFlg; >>>> >>>> mutex_lock(&ses->session_mutex); >>>> >>> >>> How does this change the SecurityFlags interface? >>> >>> -- >>> Jeff Layton <jlayton@redhat.com> >> >> >> >> -- >> Thanks, >> >> Steve > > > > -- > Thanks, > > Steve
On Fri, 23 Nov 2012 20:48:40 -0600 Steve French <smfrench@gmail.com> wrote: > it doesn't change security flags - but it seemed the smallest and > safest since it basically says: > 1) if you pass in "sec=" then use that > 2) otherwise use ntlmssp (with ntlmv2) > > so shouldn't have any unintended consequences (and the sign mount > option should work as expected as well) > Umm...I think it would. The story for people who need to mount using cleartext passwords has always been "Set SecurityFlags to a magic value and mount without a sec= option". With your original patch, that would have broken them, AFAICT. > To be more specific: do you prefer this > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index f5af252..2cd5ea2 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1362,7 +1362,7 @@ require use of the stronger protocol */ > #define CIFSSEC_MUST_SEAL 0x40040 /* not supported yet */ > #define CIFSSEC_MUST_NTLMSSP 0x80080 /* raw ntlmssp with ntlmv2 */ > > -#define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLM | > CIFSSEC_MAY_NTLMV2 | CIFSSEC_MAY_NTLMSSP) > +#define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLMSSP) > #define CIFSSEC_MAX (CIFSSEC_MUST_SIGN | CIFSSEC_MUST_NTLMV2) > #define CIFSSEC_AUTH_MASK (CIFSSEC_MAY_NTLM | CIFSSEC_MAY_NTLMV2 | > CIFSSEC_MAY_LANMAN | CIFSSEC_MAY_PLNTXT | CIFSSEC_MAY_ > /* > I think so -- that looks like it won't break existing users who need to set SecurityFlags to mount particular servers. You should also have this patch remove the now-bogus warning at mount time though. As non-sensical as the SecurityFlags interface is, we're stuck with it for now. For the longer term, I'd like to start deprecating the SecurityFlags interface altogether. I'll plan to write up a comprehensive proposal for doing that soon.
On Sat, Nov 24, 2012 at 5:11 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Fri, 23 Nov 2012 20:48:40 -0600 > Steve French <smfrench@gmail.com> wrote: > >> it doesn't change security flags - but it seemed the smallest and >> safest since it basically says: >> 1) if you pass in "sec=" then use that >> 2) otherwise use ntlmssp (with ntlmv2) >> >> so shouldn't have any unintended consequences (and the sign mount >> option should work as expected as well) >> > > Umm...I think it would. The story for people who need to mount using > cleartext passwords has always been "Set SecurityFlags to a magic value > and mount without a sec= option". With your original patch, that would > have broken them, AFAICT. originally it was supposed to be mount with sec=none (after the administrator configured the system to allow weak passwords via the global configuration flag). We clearly do not want to allow plaintext passwords to be sent over the network unless the user/admin really knows what they are doing. >> To be more specific: do you prefer this >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index f5af252..2cd5ea2 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -1362,7 +1362,7 @@ require use of the stronger protocol */ >> #define CIFSSEC_MUST_SEAL 0x40040 /* not supported yet */ >> #define CIFSSEC_MUST_NTLMSSP 0x80080 /* raw ntlmssp with ntlmv2 */ >> >> -#define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLM | >> CIFSSEC_MAY_NTLMV2 | CIFSSEC_MAY_NTLMSSP) >> +#define CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLMSSP) >> #define CIFSSEC_MAX (CIFSSEC_MUST_SIGN | CIFSSEC_MUST_NTLMV2) >> #define CIFSSEC_AUTH_MASK (CIFSSEC_MAY_NTLM | CIFSSEC_MAY_NTLMV2 | >> CIFSSEC_MAY_LANMAN | CIFSSEC_MAY_PLNTXT | CIFSSEC_MAY_ >> /* >> > > I think so -- that looks like it won't break existing users who need to > set SecurityFlags to mount particular servers. You should also have > this patch remove the now-bogus warning at mount time though. Yes - that is the same in both patches. > As non-sensical as the SecurityFlags interface is, we're stuck with it > for now. For the longer term, I'd like to start deprecating the > SecurityFlags interface altogether. I'll plan to write up a > comprehensive proposal for doing that soon. I don't mind removing security flags - but we need a way for cifs at module load time (or some after) to read a set of (administrator configurable) system wide security defaults (similar to registry configuration, or smb.conf for Samba) - I don't know the best way to do this (or way consistent with how other modules do it).
On Sat, Nov 24, 2012 at 10:55 AM, Steve French <smfrench@gmail.com> wrote: > On Sat, Nov 24, 2012 at 5:11 AM, Jeff Layton <jlayton@redhat.com> wrote: >> On Fri, 23 Nov 2012 20:48:40 -0600 >> Steve French <smfrench@gmail.com> wrote: >> >>> it doesn't change security flags - but it seemed the smallest and >>> safest since it basically says: >>> 1) if you pass in "sec=" then use that >>> 2) otherwise use ntlmssp (with ntlmv2) >>> >>> so shouldn't have any unintended consequences (and the sign mount >>> option should work as expected as well) >>> >> >> Umm...I think it would. The story for people who need to mount using >> cleartext passwords has always been "Set SecurityFlags to a magic value >> and mount without a sec= option". With your original patch, that would >> have broken them, AFAICT. you are probably right (sec=none was obviously for null user not plain text password)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 5c670b9..3bca289 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1103,6 +1103,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, bool uid_specified = false; bool gid_specified = false; bool sloppy = false; + bool sec_explicitly_set = false; char *invalid = NULL; char *nodename = utsname()->nodename;
This patch to upgrade the default security mechanism to ntlmv2/ntlmssp (which is broadly supported for years now, and a reasonable minimum, far better than ntlm) is overdue, but I had to rework it to simplify it. char *string = NULL; @@ -1763,6 +1764,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, if (cifs_parse_security_flavors(string, vol) != 0) goto cifs_parse_mount_err; + sec_explicitly_set = true; break; case Opt_cache: string = match_strdup(args); @@ -1799,6 +1801,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, goto cifs_parse_mount_err; } #endif + if (sec_explicitly_set == false) + vol->secFlg |= CIFSSEC_MAY_NTLMSSP; if (vol->UNCip == NULL) vol->UNCip = &vol->UNC[2]; @@ -2397,8 +2401,6 @@ cifs_set_cifscreds(struct smb_vol *vol __attribute__((unused)), } #endif /* CONFIG_KEYS */ -static bool warned_on_ntlm; /* globals init to false automatically */ - static struct cifs_ses * cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) { @@ -2475,14 +2477,6 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) ses->cred_uid = volume_info->cred_uid; ses->linux_uid = volume_info->linux_uid; - /* ntlmv2 is much stronger than ntlm security, and has been broadly - supported for many years, time to update default security mechanism */ - if ((volume_info->secFlg == 0) && warned_on_ntlm == false) { - warned_on_ntlm = true; - cERROR(1, "default security mechanism requested. The default " - "security mechanism will be upgraded from ntlm to " - "ntlmv2 in kernel release 3.3"); - } ses->overrideSecFlg = volume_info->secFlg; mutex_lock(&ses->session_mutex);