2013-09-07 23:18:17

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 0/6] Fix NFSv4 client-side security autonegotiation

So, it turns out that security autonegotition on the NFSv4 client has been
broken since Linux 3.10. Nobody has noticed so far, and that is probably
because it is broken on the Linux server too...

With this set of patches, the NFS client will track 'sec=', and will
ensure that we do _not_ allow the crossing of mountpoint boundaries that
violate the 'sec=' mount option.
If the user does not set a 'sec=' mount option, the client will use
autonegotiation and try to select the first supported flavour that is
returned by the SECINFO/SECINFO_NO_NAME functions. The problem here is
that the Linux server appears to sort the list of flavours that it
supports, which makes a mockery of export lines of the form:

/export -sync,no_wdelay,no_subtree_check 192.168.1.10/24 \
(sec=krb5p:krb5i,rw,sec=krb5:sys,ro)

Normally, I'd expect an export like the above to order krb5p and
krb5i _before_ the krb5 and sys in the response to SECINFO so that
the client will prefer those over the flavours that only allow read
access to the data...

Anyhow, the client should now be ready for the server-side fix...

Cheers
Trond


Trond Myklebust (6):
NFS: Clean up the auth flavour array mess
NFS: Clean up nfs_parse_security_flavors()
NFSv4: Fix security auto-negotiation
NFSv4: Disallow security negotiation for lookups when 'sec=' is
specified
NFSv4: Allow security autonegotiation for submounts
NFS: nfs_compare_super shouldn't check the auth flavour unless 'sec='
was set

fs/nfs/internal.h | 2 +-
fs/nfs/nfs4_fs.h | 2 +-
fs/nfs/nfs4client.c | 20 +++++++++----
fs/nfs/nfs4getroot.c | 4 +--
fs/nfs/nfs4namespace.c | 21 +++++++++++---
fs/nfs/nfs4proc.c | 21 ++++++++++----
fs/nfs/nfs4super.c | 2 --
fs/nfs/super.c | 79 +++++++++++++++++++++++++++++++++-----------------
8 files changed, 104 insertions(+), 47 deletions(-)

--
1.8.3.1



2013-09-08 14:00:33

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 0/6] Fix NFSv4 client-side security autonegotiation

T24gU3VuLCAyMDEzLTA5LTA4IGF0IDE1OjU1ICswMjAwLCBXaWxsaWFtIERhdWNoeSB3cm90ZToN
Cj4gSGkgVHJvbmQsDQo+IA0KPiBPbiBTdW4sIFNlcCA4LCAyMDEzIGF0IDE6MTggQU0sIFRyb25k
IE15a2xlYnVzdA0KPiA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+IHdyb3RlOg0KPiA+IFNv
LCBpdCB0dXJucyBvdXQgdGhhdCBzZWN1cml0eSBhdXRvbmVnb3RpdGlvbiBvbiB0aGUgTkZTdjQg
Y2xpZW50IGhhcyBiZWVuDQo+ID4gYnJva2VuIHNpbmNlIExpbnV4IDMuMTAuIE5vYm9keSBoYXMg
bm90aWNlZCBzbyBmYXIsIGFuZCB0aGF0IGlzIHByb2JhYmx5DQo+ID4gYmVjYXVzZSBpdCBpcyBi
cm9rZW4gb24gdGhlIExpbnV4IHNlcnZlciB0b28uLi4NCj4gDQo+IFNpbmNlIDMuMTAueCBrZXJu
ZWwgd2lsbCBiZSBhIGxvbmcgdGVybSBzdXBwb3J0LCBhcmUgdGhlc2UgcGF0Y2hlcw0KPiBnb2lu
ZyB0byBiZSBhcHBsaWVkIGluIHN0YWJsZSB0cmVlPw0KDQpJIHdhc24ndCBwbGFubmluZyBvbiBk
b2luZyB0aGF0IGFzIGl0IGlzIGVhc3kgdG8gd29yayBhcm91bmQgdGhlIHByb2JsZW0NCmJ5IHVz
aW5nIGFuIGV4cGxpY2l0ICdzZWM9JyBvcHRpb24uIFRoYXQgc2FpZCwgaWYgdGhlIGRpc3Ryb3Mg
ZG8gd2FudCBpdA0KaW4gMy4xMCwgdGhlbiB0aGV5IHNob3VsZCBiZSBhYmxlIHRvIHNwb25zb3Ig
aXRzIGluY2x1c2lvbiBpbiB0aGUgc3RhYmxlDQpzZXJpZXMuDQoNCkNoZWVycw0KICBUcm9uZA0K
DQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5l
dEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

2013-09-08 13:56:10

by William Dauchy

[permalink] [raw]
Subject: Re: [PATCH 0/6] Fix NFSv4 client-side security autonegotiation

Hi Trond,

On Sun, Sep 8, 2013 at 1:18 AM, Trond Myklebust
<[email protected]> wrote:
> So, it turns out that security autonegotition on the NFSv4 client has been
> broken since Linux 3.10. Nobody has noticed so far, and that is probably
> because it is broken on the Linux server too...

Since 3.10.x kernel will be a long term support, are these patches
going to be applied in stable tree?

Regards,

--
William

2013-09-07 23:18:20

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 5/6] NFSv4: Allow security autonegotiation for submounts

In cases where the parent super block was not mounted with a 'sec=' line,
allow autonegotiation of security for the submounts.

Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4client.c | 3 ++-
fs/nfs/nfs4namespace.c | 21 +++++++++++++++++----
2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index cc80085..a860ab5 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1078,7 +1078,8 @@ struct nfs_server *nfs4_create_referral_server(struct nfs_clone_mount *data,
if (error < 0)
goto error;

- error = nfs4_server_common_setup(server, mntfh, false);
+ error = nfs4_server_common_setup(server, mntfh,
+ !(parent_server->flags & NFS_MOUNT_SECFLAVOUR));
if (error < 0)
goto error;

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index cdb0b41..2288cd3 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -11,6 +11,7 @@
#include <linux/mount.h>
#include <linux/namei.h>
#include <linux/nfs_fs.h>
+#include <linux/nfs_mount.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/sunrpc/clnt.h>
@@ -369,21 +370,33 @@ out:
struct vfsmount *nfs4_submount(struct nfs_server *server, struct dentry *dentry,
struct nfs_fh *fh, struct nfs_fattr *fattr)
{
+ rpc_authflavor_t flavor = server->client->cl_auth->au_flavor;
struct dentry *parent = dget_parent(dentry);
+ struct inode *dir = parent->d_inode;
+ struct qstr *name = &dentry->d_name;
struct rpc_clnt *client;
struct vfsmount *mnt;

/* Look it up again to get its attributes and sec flavor */
- client = nfs4_proc_lookup_mountpoint(parent->d_inode, &dentry->d_name, fh, fattr);
+ client = nfs4_proc_lookup_mountpoint(dir, name, fh, fattr);
dput(parent);
if (IS_ERR(client))
return ERR_CAST(client);

- if (fattr->valid & NFS_ATTR_FATTR_V4_REFERRAL)
+ if (fattr->valid & NFS_ATTR_FATTR_V4_REFERRAL) {
mnt = nfs_do_refmount(client, dentry);
- else
- mnt = nfs_do_submount(dentry, fh, fattr, client->cl_auth->au_flavor);
+ goto out;
+ }

+ if (client->cl_auth->au_flavor != flavor)
+ flavor = client->cl_auth->au_flavor;
+ else if (!(server->flags & NFS_MOUNT_SECFLAVOUR)) {
+ rpc_authflavor_t new = nfs4_negotiate_security(dir, name);
+ if ((int)new >= 0)
+ flavor = new;
+ }
+ mnt = nfs_do_submount(dentry, fh, fattr, flavor);
+out:
rpc_shutdown_client(client);
return mnt;
}
--
1.8.3.1


2013-09-08 14:04:43

by William Dauchy

[permalink] [raw]
Subject: Re: [PATCH 0/6] Fix NFSv4 client-side security autonegotiation

On Sun, Sep 8, 2013 at 4:00 PM, Myklebust, Trond
<[email protected]> wrote:
> I wasn't planning on doing that as it is easy to work around the problem
> by using an explicit 'sec=' option. That said, if the distros do want it
> in 3.10, then they should be able to sponsor its inclusion in the stable
> series.

ack, understood.

Regards,
--
William

2013-09-07 23:18:18

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 2/6] NFS: Clean up nfs_parse_security_flavors()

Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/super.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5d16ee3..b2dd6da 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1032,49 +1032,50 @@ static int nfs_parse_security_flavors(char *value,
struct nfs_parsed_mount_data *mnt)
{
substring_t args[MAX_OPT_ARGS];
+ rpc_authflavor_t pseudoflavor;

dfprintk(MOUNT, "NFS: parsing sec=%s option\n", value);

switch (match_token(value, nfs_secflavor_tokens, args)) {
case Opt_sec_none:
- mnt->auth_flavors[0] = RPC_AUTH_NULL;
+ pseudoflavor = RPC_AUTH_NULL;
break;
case Opt_sec_sys:
- mnt->auth_flavors[0] = RPC_AUTH_UNIX;
+ pseudoflavor = RPC_AUTH_UNIX;
break;
case Opt_sec_krb5:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_KRB5;
+ pseudoflavor = RPC_AUTH_GSS_KRB5;
break;
case Opt_sec_krb5i:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_KRB5I;
+ pseudoflavor = RPC_AUTH_GSS_KRB5I;
break;
case Opt_sec_krb5p:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_KRB5P;
+ pseudoflavor = RPC_AUTH_GSS_KRB5P;
break;
case Opt_sec_lkey:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_LKEY;
+ pseudoflavor = RPC_AUTH_GSS_LKEY;
break;
case Opt_sec_lkeyi:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_LKEYI;
+ pseudoflavor = RPC_AUTH_GSS_LKEYI;
break;
case Opt_sec_lkeyp:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_LKEYP;
+ pseudoflavor = RPC_AUTH_GSS_LKEYP;
break;
case Opt_sec_spkm:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_SPKM;
+ pseudoflavor = RPC_AUTH_GSS_SPKM;
break;
case Opt_sec_spkmi:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_SPKMI;
+ pseudoflavor = RPC_AUTH_GSS_SPKMI;
break;
case Opt_sec_spkmp:
- mnt->auth_flavors[0] = RPC_AUTH_GSS_SPKMP;
+ pseudoflavor = RPC_AUTH_GSS_SPKMP;
break;
default:
return 0;
}

mnt->flags |= NFS_MOUNT_SECFLAVOUR;
- mnt->auth_flavor_len = 1;
+ nfs_set_auth_parsed_mount_data(mnt, pseudoflavor);
return 1;
}

--
1.8.3.1


2013-09-07 23:18:18

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 1/6] NFS: Clean up the auth flavour array mess

What is the point of having a 'auth_flavor_len' field, if it is
always set to 1, and can't be used to determine if the user has
selected an auth flavour?
This cleanup goes back to using auth_flavor_len for its original
intended purpose, and gets rid of the ad-hoc replacements.

Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4super.c | 4 +++-
fs/nfs/super.c | 37 +++++++++++++++++++++++++------------
2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 5dbe2d2..4ad837c 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -253,8 +253,10 @@ struct dentry *nfs4_try_mount(int flags, const char *dev_name,

dfprintk(MOUNT, "--> nfs4_try_mount()\n");

- if (data->auth_flavors[0] == RPC_AUTH_MAXFLAVOR)
+ if (data->auth_flavor_len < 1) {
data->auth_flavors[0] = RPC_AUTH_UNIX;
+ data->auth_flavor_len = 1;
+ }
export_path = data->nfs_server.export_path;
data->nfs_server.export_path = "/";
root_mnt = nfs_do_root_mount(&nfs4_remote_fs_type, flags, mount_info,
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 6ad9053..5d16ee3 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -923,7 +923,7 @@ static struct nfs_parsed_mount_data *nfs_alloc_parsed_mount_data(void)
data->nfs_server.port = NFS_UNSPEC_PORT;
data->nfs_server.protocol = XPRT_TRANSPORT_TCP;
data->auth_flavors[0] = RPC_AUTH_MAXFLAVOR;
- data->auth_flavor_len = 1;
+ data->auth_flavor_len = 0;
data->minorversion = 0;
data->need_mount = true;
data->net = current->nsproxy->net_ns;
@@ -1018,6 +1018,13 @@ static void nfs_set_mount_transport_protocol(struct nfs_parsed_mount_data *mnt)
}
}

+static void nfs_set_auth_parsed_mount_data(struct nfs_parsed_mount_data *data,
+ rpc_authflavor_t pseudoflavor)
+{
+ data->auth_flavors[0] = pseudoflavor;
+ data->auth_flavor_len = 1;
+}
+
/*
* Parse the value of the 'sec=' option.
*/
@@ -1729,7 +1736,7 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
* Was a sec= authflavor specified in the options? First, verify
* whether the server supports it, and then just try to use it if so.
*/
- if (args->auth_flavors[0] != RPC_AUTH_MAXFLAVOR) {
+ if (args->auth_flavor_len > 0) {
status = nfs_verify_authflavor(args, authlist, authlist_len);
dfprintk(MOUNT, "NFS: using auth flavor %u\n", args->auth_flavors[0]);
if (status)
@@ -1760,7 +1767,7 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf
/* Fallthrough */
}
dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", flavor);
- args->auth_flavors[0] = flavor;
+ nfs_set_auth_parsed_mount_data(args, flavor);
server = nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
if (!IS_ERR(server))
return server;
@@ -1776,7 +1783,7 @@ static struct nfs_server *nfs_try_mount_request(struct nfs_mount_info *mount_inf

/* Last chance! Try AUTH_UNIX */
dfprintk(MOUNT, "NFS: attempting to use auth flavor %u\n", RPC_AUTH_UNIX);
- args->auth_flavors[0] = RPC_AUTH_UNIX;
+ nfs_set_auth_parsed_mount_data(args, RPC_AUTH_UNIX);
return nfs_mod->rpc_ops->create_server(mount_info, nfs_mod);
}

@@ -1893,6 +1900,7 @@ static int nfs23_validate_mount_data(void *options,
{
struct nfs_mount_data *data = (struct nfs_mount_data *)options;
struct sockaddr *sap = (struct sockaddr *)&args->nfs_server.address;
+ int extra_flags = NFS_MOUNT_LEGACY_INTERFACE;

if (data == NULL)
goto out_no_data;
@@ -1908,6 +1916,8 @@ static int nfs23_validate_mount_data(void *options,
goto out_no_v3;
data->root.size = NFS2_FHSIZE;
memcpy(data->root.data, data->old_root.data, NFS2_FHSIZE);
+ /* Turn off security negotiation */
+ extra_flags |= NFS_MOUNT_SECFLAVOUR;
case 4:
if (data->flags & NFS_MOUNT_SECFLAVOUR)
goto out_no_sec;
@@ -1935,7 +1945,7 @@ static int nfs23_validate_mount_data(void *options,
* can deal with.
*/
args->flags = data->flags & NFS_MOUNT_FLAGMASK;
- args->flags |= NFS_MOUNT_LEGACY_INTERFACE;
+ args->flags |= extra_flags;
args->rsize = data->rsize;
args->wsize = data->wsize;
args->timeo = data->timeo;
@@ -1959,9 +1969,10 @@ static int nfs23_validate_mount_data(void *options,
args->namlen = data->namlen;
args->bsize = data->bsize;

- args->auth_flavors[0] = RPC_AUTH_UNIX;
if (data->flags & NFS_MOUNT_SECFLAVOUR)
- args->auth_flavors[0] = data->pseudoflavor;
+ nfs_set_auth_parsed_mount_data(args, data->pseudoflavor);
+ else
+ nfs_set_auth_parsed_mount_data(args, RPC_AUTH_UNIX);
if (!args->nfs_server.hostname)
goto out_nomem;

@@ -2176,7 +2187,7 @@ nfs_remount(struct super_block *sb, int *flags, char *raw_data)
data->rsize = nfss->rsize;
data->wsize = nfss->wsize;
data->retrans = nfss->client->cl_timeout->to_retries;
- data->auth_flavors[0] = nfss->client->cl_auth->au_flavor;
+ nfs_set_auth_parsed_mount_data(data, nfss->client->cl_auth->au_flavor);
data->acregmin = nfss->acregmin / HZ;
data->acregmax = nfss->acregmax / HZ;
data->acdirmin = nfss->acdirmin / HZ;
@@ -2675,15 +2686,17 @@ static int nfs4_validate_mount_data(void *options,
goto out_no_address;
args->nfs_server.port = ntohs(((struct sockaddr_in *)sap)->sin_port);

- args->auth_flavors[0] = RPC_AUTH_UNIX;
if (data->auth_flavourlen) {
+ rpc_authflavor_t pseudoflavor;
if (data->auth_flavourlen > 1)
goto out_inval_auth;
- if (copy_from_user(&args->auth_flavors[0],
+ if (copy_from_user(&pseudoflavor,
data->auth_flavours,
- sizeof(args->auth_flavors[0])))
+ sizeof(pseudoflavor)))
return -EFAULT;
- }
+ nfs_set_auth_parsed_mount_data(args, pseudoflavor);
+ } else
+ nfs_set_auth_parsed_mount_data(args, RPC_AUTH_UNIX);

c = strndup_user(data->hostname.data, NFS4_MAXNAMLEN);
if (IS_ERR(c))
--
1.8.3.1


2013-09-08 20:22:44

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/6] NFSv4: Fix security auto-negotiation


On Sep 7, 2013, at 7:18 PM, Trond Myklebust <[email protected]> wrote:

> NFSv4 security auto-negotiation has been broken since
> commit 4580a92d44e2b21c2254fa5fef0f1bfb43c82318 (NFS:
> Use server-recommended security flavor by default (NFSv3))
> because nfs4_try_mount() will automatically select AUTH_SYS
> if it sees no auth flavours.

nfs(5) says this:

sec=mode The RPCGSS security flavor to use for accessing files on this
mount point. If the sec option is not specified, or if sec=sys
is specified, the NFS client uses the AUTH_SYS security flavor
for all NFS requests on this mount point.

If NFSv4 can negotiate security now, nfs(5) should be updated.


> Signed-off-by: Trond Myklebust <[email protected]>
> Cc: Chuck Lever <[email protected]>
> ---
> fs/nfs/internal.h | 2 +-
> fs/nfs/nfs4_fs.h | 2 +-
> fs/nfs/nfs4client.c | 19 +++++++++++++------
> fs/nfs/nfs4getroot.c | 4 ++--
> fs/nfs/nfs4proc.c | 17 +++++++++++++----
> fs/nfs/nfs4super.c | 4 ----
> 6 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 23ec6e8..d388302c 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -358,7 +358,7 @@ extern struct dentry *nfs_get_root(struct super_block *, struct nfs_fh *,
> extern struct dentry *nfs4_get_root(struct super_block *, struct nfs_fh *,
> const char *);
>
> -extern int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh);
> +extern int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh, bool);
> #endif
>
> struct nfs_pgio_completion_ops;
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index d2db3ce..f520a11 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -221,7 +221,7 @@ struct vfsmount *nfs4_submount(struct nfs_server *, struct dentry *,
> /* nfs4proc.c */
> extern int nfs4_proc_setclientid(struct nfs_client *, u32, unsigned short, struct rpc_cred *, struct nfs4_setclientid_res *);
> extern int nfs4_proc_setclientid_confirm(struct nfs_client *, struct nfs4_setclientid_res *arg, struct rpc_cred *);
> -extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh *, struct nfs_fsinfo *);
> +extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh *, struct nfs_fsinfo *, bool);
> extern int nfs4_proc_bind_conn_to_session(struct nfs_client *, struct rpc_cred *cred);
> extern int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred);
> extern int nfs4_destroy_clientid(struct nfs_client *clp);
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index f798925..cc80085 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -885,7 +885,7 @@ static void nfs4_session_set_rwsize(struct nfs_server *server)
> }
>
> static int nfs4_server_common_setup(struct nfs_server *server,
> - struct nfs_fh *mntfh)
> + struct nfs_fh *mntfh, bool auth_probe)
> {
> struct nfs_fattr *fattr;
> int error;
> @@ -917,7 +917,7 @@ static int nfs4_server_common_setup(struct nfs_server *server,
>
>
> /* Probe the root fh to retrieve its FSID and filehandle */
> - error = nfs4_get_rootfh(server, mntfh);
> + error = nfs4_get_rootfh(server, mntfh, auth_probe);
> if (error < 0)
> goto out;
>
> @@ -949,6 +949,7 @@ out:
> static int nfs4_init_server(struct nfs_server *server,
> const struct nfs_parsed_mount_data *data)
> {
> + rpc_authflavor_t pseudoflavor = RPC_AUTH_UNIX;
> struct rpc_timeout timeparms;
> int error;
>
> @@ -961,13 +962,16 @@ static int nfs4_init_server(struct nfs_server *server,
> server->flags = data->flags;
> server->options = data->options;
>
> + if (data->auth_flavor_len >= 1)
> + pseudoflavor = data->auth_flavors[0];
> +
> /* Get a client record */
> error = nfs4_set_client(server,
> data->nfs_server.hostname,
> (const struct sockaddr *)&data->nfs_server.address,
> data->nfs_server.addrlen,
> data->client_address,
> - data->auth_flavors[0],
> + pseudoflavor,
> data->nfs_server.protocol,
> &timeparms,
> data->minorversion,
> @@ -987,7 +991,7 @@ static int nfs4_init_server(struct nfs_server *server,
>
> server->port = data->nfs_server.port;
>
> - error = nfs_init_server_rpcclient(server, &timeparms, data->auth_flavors[0]);
> + error = nfs_init_server_rpcclient(server, &timeparms, pseudoflavor);
>
> error:
> /* Done */
> @@ -1005,6 +1009,7 @@ struct nfs_server *nfs4_create_server(struct nfs_mount_info *mount_info,
> struct nfs_subversion *nfs_mod)
> {
> struct nfs_server *server;
> + bool auth_probe;
> int error;
>
> dprintk("--> nfs4_create_server()\n");
> @@ -1013,12 +1018,14 @@ struct nfs_server *nfs4_create_server(struct nfs_mount_info *mount_info,
> if (!server)
> return ERR_PTR(-ENOMEM);
>
> + auth_probe = mount_info->parsed->auth_flavor_len < 1;
> +
> /* set up the general RPC client */
> error = nfs4_init_server(server, mount_info->parsed);
> if (error < 0)
> goto error;
>
> - error = nfs4_server_common_setup(server, mount_info->mntfh);
> + error = nfs4_server_common_setup(server, mount_info->mntfh, auth_probe);
> if (error < 0)
> goto error;
>
> @@ -1071,7 +1078,7 @@ struct nfs_server *nfs4_create_referral_server(struct nfs_clone_mount *data,
> if (error < 0)
> goto error;
>
> - error = nfs4_server_common_setup(server, mntfh);
> + error = nfs4_server_common_setup(server, mntfh, false);
> if (error < 0)
> goto error;
>
> diff --git a/fs/nfs/nfs4getroot.c b/fs/nfs/nfs4getroot.c
> index 549462e..c0b3a16 100644
> --- a/fs/nfs/nfs4getroot.c
> +++ b/fs/nfs/nfs4getroot.c
> @@ -9,7 +9,7 @@
>
> #define NFSDBG_FACILITY NFSDBG_CLIENT
>
> -int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh)
> +int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh, bool auth_probe)
> {
> struct nfs_fsinfo fsinfo;
> int ret = -ENOMEM;
> @@ -21,7 +21,7 @@ int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh)
> goto out;
>
> /* Start by getting the root filehandle from the server */
> - ret = nfs4_proc_get_rootfh(server, mntfh, &fsinfo);
> + ret = nfs4_proc_get_rootfh(server, mntfh, &fsinfo, auth_probe);
> if (ret < 0) {
> dprintk("nfs4_get_rootfh: getroot error = %d\n", -ret);
> goto out;
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index cb56102..68551ea 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2884,18 +2884,27 @@ static int nfs4_do_find_root_sec(struct nfs_server *server,
> * @server: initialized nfs_server handle
> * @fhandle: we fill in the pseudo-fs root file handle
> * @info: we fill in an FSINFO struct
> + * @auth_probe: probe the auth flavours
> *
> * Returns zero on success, or a negative errno.
> */
> int nfs4_proc_get_rootfh(struct nfs_server *server, struct nfs_fh *fhandle,
> - struct nfs_fsinfo *info)
> + struct nfs_fsinfo *info,
> + bool auth_probe)
> {
> int status;
>
> - status = nfs4_lookup_root(server, fhandle, info);
> - if ((status == -NFS4ERR_WRONGSEC) &&
> - !(server->flags & NFS_MOUNT_SECFLAVOUR))
> + switch (auth_probe) {
> + case false:
> + status = nfs4_lookup_root(server, fhandle, info);
> + if (status != -NFS4ERR_WRONGSEC)
> + break;
> + /* Did user force a 'sec=' mount option? */
> + if (server->flags & NFS_MOUNT_SECFLAVOUR)
> + break;
> + default:
> status = nfs4_do_find_root_sec(server, fhandle, info);
> + }
>
> if (status == 0)
> status = nfs4_server_capabilities(server, fhandle);
> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> index 4ad837c..e26acdd 100644
> --- a/fs/nfs/nfs4super.c
> +++ b/fs/nfs/nfs4super.c
> @@ -253,10 +253,6 @@ struct dentry *nfs4_try_mount(int flags, const char *dev_name,
>
> dfprintk(MOUNT, "--> nfs4_try_mount()\n");
>
> - if (data->auth_flavor_len < 1) {
> - data->auth_flavors[0] = RPC_AUTH_UNIX;
> - data->auth_flavor_len = 1;
> - }
> export_path = data->nfs_server.export_path;
> data->nfs_server.export_path = "/";
> root_mnt = nfs_do_root_mount(&nfs4_remote_fs_type, flags, mount_info,
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




2013-09-08 20:50:02

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 3/6] NFSv4: Fix security auto-negotiation

T24gU3VuLCAyMDEzLTA5LTA4IGF0IDE2OjIyIC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gU2VwIDcsIDIwMTMsIGF0IDc6MTggUE0sIFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVi
dXN0QG5ldGFwcC5jb20+IHdyb3RlOg0KPiANCj4gPiBORlN2NCBzZWN1cml0eSBhdXRvLW5lZ290
aWF0aW9uIGhhcyBiZWVuIGJyb2tlbiBzaW5jZQ0KPiA+IGNvbW1pdCA0NTgwYTkyZDQ0ZTJiMjFj
MjI1NGZhNWZlZjBmMWJmYjQzYzgyMzE4IChORlM6DQo+ID4gVXNlIHNlcnZlci1yZWNvbW1lbmRl
ZCBzZWN1cml0eSBmbGF2b3IgYnkgZGVmYXVsdCAoTkZTdjMpKQ0KPiA+IGJlY2F1c2UgbmZzNF90
cnlfbW91bnQoKSB3aWxsIGF1dG9tYXRpY2FsbHkgc2VsZWN0IEFVVEhfU1lTDQo+ID4gaWYgaXQg
c2VlcyBubyBhdXRoIGZsYXZvdXJzLg0KPiANCj4gbmZzKDUpIHNheXMgdGhpczoNCj4gDQo+ICAg
ICAgICBzZWM9bW9kZSAgICAgICBUaGUgIFJQQ0dTUyAgc2VjdXJpdHkgZmxhdm9yIHRvIHVzZSBm
b3IgYWNjZXNzaW5nIGZpbGVzIG9uIHRoaXMNCj4gICAgICAgICAgICAgICAgICAgICAgIG1vdW50
IHBvaW50LiAgSWYgdGhlIHNlYyBvcHRpb24gaXMgbm90IHNwZWNpZmllZCwgb3IgaWYgc2VjPXN5
cw0KPiAgICAgICAgICAgICAgICAgICAgICAgaXMgIHNwZWNpZmllZCwgdGhlIE5GUyBjbGllbnQg
dXNlcyB0aGUgQVVUSF9TWVMgc2VjdXJpdHkgZmxhdm9yDQo+ICAgICAgICAgICAgICAgICAgICAg
ICBmb3IgYWxsIE5GUyByZXF1ZXN0cyBvbiB0aGlzIG1vdW50IHBvaW50LiAgDQo+IA0KPiBJZiBO
RlN2NCBjYW4gbmVnb3RpYXRlIHNlY3VyaXR5IG5vdywgbmZzKDUpIHNob3VsZCBiZSB1cGRhdGVk
Lg0KDQpJIHN1Z2dlc3QgdGhhdCB5b3UgcHVsbCBhZ2Fpbi4gTXkgY29weSBvZiBuZnMoNSkgc2F5
cw0KDQogICAgICAgc2VjPWZsYXZvciAgICAgVGhlICBzZWN1cml0eSAgZmxhdm9yIHRvIHVzZSBm
b3IgYWNjZXNzaW5nIGZpbGVzIG9uIHRoaXMNCiAgICAgICAgICAgICAgICAgICAgICBtb3VudCBw
b2ludC4gIElmIHRoZSBzZXJ2ZXIgZG9lcyBub3Qgc3VwcG9ydCAgdGhpcyAgZmxh4oCQDQogICAg
ICAgICAgICAgICAgICAgICAgdm9yLCAgdGhlICBtb3VudCBvcGVyYXRpb24gZmFpbHMuICBJZiBz
ZWM9IGlzIG5vdCBzcGVjaeKAkA0KICAgICAgICAgICAgICAgICAgICAgIGZpZWQsIHRoZSBjbGll
bnQgYXR0ZW1wdHMgdG8gZmluZCBhIHNlY3VyaXR5IGZsYXZvciB0aGF0DQogICAgICAgICAgICAg
ICAgICAgICAgYm90aCAgdGhlIGNsaWVudCBhbmQgdGhlIHNlcnZlciBzdXBwb3J0cy4gIFZhbGlk
IGZsYXZvcnMNCiAgICAgICAgICAgICAgICAgICAgICBhcmUgbm9uZSwgc3lzLCBrcmI1LCBrcmI1
aSwgYW5kICBrcmI1cC4gICBSZWZlciAgdG8gIHRoZQ0KICAgICAgICAgICAgICAgICAgICAgIFNF
Q1VSSVRZIENPTlNJREVSQVRJT05TIHNlY3Rpb24gZm9yIGRldGFpbHMuDQoNCg0KLS0gDQpUcm9u
ZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25k
Lk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0K

2013-09-07 23:18:37

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 4/6] NFSv4: Disallow security negotiation for lookups when 'sec=' is specified

Ensure that nfs4_proc_lookup_common respects the NFS_MOUNT_SECFLAVOUR
flag.

Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 68551ea..122b934 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3154,7 +3154,9 @@ static int nfs4_proc_lookup_common(struct rpc_clnt **clnt, struct inode *dir,
err = -EPERM;
if (client != *clnt)
goto out;
-
+ /* No security negotiation if the user specified 'sec=' */
+ if (NFS_SERVER(dir)->flags & NFS_MOUNT_SECFLAVOUR)
+ goto out;
client = nfs4_create_sec_client(client, dir, name);
if (IS_ERR(client))
return PTR_ERR(client);
--
1.8.3.1


2013-09-07 23:18:37

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 6/6] NFS: nfs_compare_super shouldn't check the auth flavour unless 'sec=' was set

Also don't worry about obsolete mount flags...

Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/super.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index b2dd6da..50bc31d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2295,6 +2295,18 @@ void nfs_clone_super(struct super_block *sb, struct nfs_mount_info *mount_info)
nfs_initialise_sb(sb);
}

+#define NFS_MOUNT_CMP_FLAGMASK ~(NFS_MOUNT_INTR \
+ | NFS_MOUNT_SECURE \
+ | NFS_MOUNT_TCP \
+ | NFS_MOUNT_VER3 \
+ | NFS_MOUNT_KERBEROS \
+ | NFS_MOUNT_NONLM \
+ | NFS_MOUNT_BROKEN_SUID \
+ | NFS_MOUNT_STRICTLOCK \
+ | NFS_MOUNT_UNSHARED \
+ | NFS_MOUNT_NORESVPORT \
+ | NFS_MOUNT_LEGACY_INTERFACE)
+
static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b, int flags)
{
const struct nfs_server *a = s->s_fs_info;
@@ -2305,7 +2317,7 @@ static int nfs_compare_mount_options(const struct super_block *s, const struct n
goto Ebusy;
if (a->nfs_client != b->nfs_client)
goto Ebusy;
- if (a->flags != b->flags)
+ if ((a->flags ^ b->flags) & NFS_MOUNT_CMP_FLAGMASK)
goto Ebusy;
if (a->wsize != b->wsize)
goto Ebusy;
@@ -2319,7 +2331,8 @@ static int nfs_compare_mount_options(const struct super_block *s, const struct n
goto Ebusy;
if (a->acdirmax != b->acdirmax)
goto Ebusy;
- if (clnt_a->cl_auth->au_flavor != clnt_b->cl_auth->au_flavor)
+ if (b->flags & NFS_MOUNT_SECFLAVOUR &&
+ clnt_a->cl_auth->au_flavor != clnt_b->cl_auth->au_flavor)
goto Ebusy;
return 1;
Ebusy:
--
1.8.3.1


2013-09-07 23:18:19

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH 3/6] NFSv4: Fix security auto-negotiation

NFSv4 security auto-negotiation has been broken since
commit 4580a92d44e2b21c2254fa5fef0f1bfb43c82318 (NFS:
Use server-recommended security flavor by default (NFSv3))
because nfs4_try_mount() will automatically select AUTH_SYS
if it sees no auth flavours.

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Chuck Lever <[email protected]>
---
fs/nfs/internal.h | 2 +-
fs/nfs/nfs4_fs.h | 2 +-
fs/nfs/nfs4client.c | 19 +++++++++++++------
fs/nfs/nfs4getroot.c | 4 ++--
fs/nfs/nfs4proc.c | 17 +++++++++++++----
fs/nfs/nfs4super.c | 4 ----
6 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 23ec6e8..d388302c 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -358,7 +358,7 @@ extern struct dentry *nfs_get_root(struct super_block *, struct nfs_fh *,
extern struct dentry *nfs4_get_root(struct super_block *, struct nfs_fh *,
const char *);

-extern int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh);
+extern int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh, bool);
#endif

struct nfs_pgio_completion_ops;
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index d2db3ce..f520a11 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -221,7 +221,7 @@ struct vfsmount *nfs4_submount(struct nfs_server *, struct dentry *,
/* nfs4proc.c */
extern int nfs4_proc_setclientid(struct nfs_client *, u32, unsigned short, struct rpc_cred *, struct nfs4_setclientid_res *);
extern int nfs4_proc_setclientid_confirm(struct nfs_client *, struct nfs4_setclientid_res *arg, struct rpc_cred *);
-extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh *, struct nfs_fsinfo *);
+extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh *, struct nfs_fsinfo *, bool);
extern int nfs4_proc_bind_conn_to_session(struct nfs_client *, struct rpc_cred *cred);
extern int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred);
extern int nfs4_destroy_clientid(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index f798925..cc80085 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -885,7 +885,7 @@ static void nfs4_session_set_rwsize(struct nfs_server *server)
}

static int nfs4_server_common_setup(struct nfs_server *server,
- struct nfs_fh *mntfh)
+ struct nfs_fh *mntfh, bool auth_probe)
{
struct nfs_fattr *fattr;
int error;
@@ -917,7 +917,7 @@ static int nfs4_server_common_setup(struct nfs_server *server,


/* Probe the root fh to retrieve its FSID and filehandle */
- error = nfs4_get_rootfh(server, mntfh);
+ error = nfs4_get_rootfh(server, mntfh, auth_probe);
if (error < 0)
goto out;

@@ -949,6 +949,7 @@ out:
static int nfs4_init_server(struct nfs_server *server,
const struct nfs_parsed_mount_data *data)
{
+ rpc_authflavor_t pseudoflavor = RPC_AUTH_UNIX;
struct rpc_timeout timeparms;
int error;

@@ -961,13 +962,16 @@ static int nfs4_init_server(struct nfs_server *server,
server->flags = data->flags;
server->options = data->options;

+ if (data->auth_flavor_len >= 1)
+ pseudoflavor = data->auth_flavors[0];
+
/* Get a client record */
error = nfs4_set_client(server,
data->nfs_server.hostname,
(const struct sockaddr *)&data->nfs_server.address,
data->nfs_server.addrlen,
data->client_address,
- data->auth_flavors[0],
+ pseudoflavor,
data->nfs_server.protocol,
&timeparms,
data->minorversion,
@@ -987,7 +991,7 @@ static int nfs4_init_server(struct nfs_server *server,

server->port = data->nfs_server.port;

- error = nfs_init_server_rpcclient(server, &timeparms, data->auth_flavors[0]);
+ error = nfs_init_server_rpcclient(server, &timeparms, pseudoflavor);

error:
/* Done */
@@ -1005,6 +1009,7 @@ struct nfs_server *nfs4_create_server(struct nfs_mount_info *mount_info,
struct nfs_subversion *nfs_mod)
{
struct nfs_server *server;
+ bool auth_probe;
int error;

dprintk("--> nfs4_create_server()\n");
@@ -1013,12 +1018,14 @@ struct nfs_server *nfs4_create_server(struct nfs_mount_info *mount_info,
if (!server)
return ERR_PTR(-ENOMEM);

+ auth_probe = mount_info->parsed->auth_flavor_len < 1;
+
/* set up the general RPC client */
error = nfs4_init_server(server, mount_info->parsed);
if (error < 0)
goto error;

- error = nfs4_server_common_setup(server, mount_info->mntfh);
+ error = nfs4_server_common_setup(server, mount_info->mntfh, auth_probe);
if (error < 0)
goto error;

@@ -1071,7 +1078,7 @@ struct nfs_server *nfs4_create_referral_server(struct nfs_clone_mount *data,
if (error < 0)
goto error;

- error = nfs4_server_common_setup(server, mntfh);
+ error = nfs4_server_common_setup(server, mntfh, false);
if (error < 0)
goto error;

diff --git a/fs/nfs/nfs4getroot.c b/fs/nfs/nfs4getroot.c
index 549462e..c0b3a16 100644
--- a/fs/nfs/nfs4getroot.c
+++ b/fs/nfs/nfs4getroot.c
@@ -9,7 +9,7 @@

#define NFSDBG_FACILITY NFSDBG_CLIENT

-int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh)
+int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh, bool auth_probe)
{
struct nfs_fsinfo fsinfo;
int ret = -ENOMEM;
@@ -21,7 +21,7 @@ int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh *mntfh)
goto out;

/* Start by getting the root filehandle from the server */
- ret = nfs4_proc_get_rootfh(server, mntfh, &fsinfo);
+ ret = nfs4_proc_get_rootfh(server, mntfh, &fsinfo, auth_probe);
if (ret < 0) {
dprintk("nfs4_get_rootfh: getroot error = %d\n", -ret);
goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cb56102..68551ea 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2884,18 +2884,27 @@ static int nfs4_do_find_root_sec(struct nfs_server *server,
* @server: initialized nfs_server handle
* @fhandle: we fill in the pseudo-fs root file handle
* @info: we fill in an FSINFO struct
+ * @auth_probe: probe the auth flavours
*
* Returns zero on success, or a negative errno.
*/
int nfs4_proc_get_rootfh(struct nfs_server *server, struct nfs_fh *fhandle,
- struct nfs_fsinfo *info)
+ struct nfs_fsinfo *info,
+ bool auth_probe)
{
int status;

- status = nfs4_lookup_root(server, fhandle, info);
- if ((status == -NFS4ERR_WRONGSEC) &&
- !(server->flags & NFS_MOUNT_SECFLAVOUR))
+ switch (auth_probe) {
+ case false:
+ status = nfs4_lookup_root(server, fhandle, info);
+ if (status != -NFS4ERR_WRONGSEC)
+ break;
+ /* Did user force a 'sec=' mount option? */
+ if (server->flags & NFS_MOUNT_SECFLAVOUR)
+ break;
+ default:
status = nfs4_do_find_root_sec(server, fhandle, info);
+ }

if (status == 0)
status = nfs4_server_capabilities(server, fhandle);
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 4ad837c..e26acdd 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -253,10 +253,6 @@ struct dentry *nfs4_try_mount(int flags, const char *dev_name,

dfprintk(MOUNT, "--> nfs4_try_mount()\n");

- if (data->auth_flavor_len < 1) {
- data->auth_flavors[0] = RPC_AUTH_UNIX;
- data->auth_flavor_len = 1;
- }
export_path = data->nfs_server.export_path;
data->nfs_server.export_path = "/";
root_mnt = nfs_do_root_mount(&nfs4_remote_fs_type, flags, mount_info,
--
1.8.3.1