[PATCH v2] NFS: Fix NFSv2 security settings

[PATCH v2] NFS: Fix NFSv2 security settings

[Chuck Lever]

For a while now any NFSv2 mount where sec= is specified uses
AUTH_NULL. If sec= is not specified, the mount uses AUTH_UNIX.
Commit e68fd7c8071d ("mount: use sec= that was specified on the
command line") attempted to address a very similar problem with
NFSv3, and should have fixed this too, but it has a bug.

The MNTv1 MNT procedure does not return a list of security flavors,
so our client makes up a list containing just AUTH_NULL. This should
enable nfs_verify_authflavors() to assign the sec= specified flavor,
but instead, it incorrectly sets it to AUTH_NULL.

I expect this would also be a problem for any NFSv3 server whose
MNTv3 MNT procedure returned a security flavor list containing only
AUTH_NULL.

Fixes: e68fd7c8071d ("mount: use sec= that was specified on ... ")
BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=310
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/super.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Changes since v1:
- Description edited for accuracy

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d828ef8..6b179af 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1691,8 +1691,8 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
 			rpc_authflavor_t *server_authlist, unsigned int count)
 {
 	rpc_authflavor_t flavor = RPC_AUTH_MAXFLAVOR;
+	bool found_auth_null = false;
 	unsigned int i;
-	int use_auth_null = false;
 
 	/*
 	 * If the sec= mount option is used, the specified flavor or AUTH_NULL
@@ -1701,6 +1701,10 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
 	 * AUTH_NULL has a special meaning when it's in the server list - it
 	 * means that the server will ignore the rpc creds, so any flavor
 	 * can be used but still use the sec= that was specified.
+	 *
+	 * Note also that the MNT procedure in MNTv1 does not return a list
+	 * of supported security flavors. In this case, nfs_mount() fabricates
+	 * a security flavor list containing just AUTH_NULL.
 	 */
 	for (i = 0; i < count; i++) {
 		flavor = server_authlist[i];
@@ -1709,11 +1713,11 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
 			goto out;
 
 		if (flavor == RPC_AUTH_NULL)
-			use_auth_null = true;
+			found_auth_null = true;
 	}
 
-	if (use_auth_null) {
-		flavor = RPC_AUTH_NULL;
+	if (found_auth_null) {
+		flavor = args->auth_info.flavors[0];
 		goto out;
 	}
 

Re: [PATCH v2] NFS: Fix NFSv2 security settings

[Chuck Lever]

Ping...


> On Aug 10, 2017, at 4:41 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> For a while now any NFSv2 mount where sec= is specified uses
> AUTH_NULL. If sec= is not specified, the mount uses AUTH_UNIX.
> Commit e68fd7c8071d ("mount: use sec= that was specified on the
> command line") attempted to address a very similar problem with
> NFSv3, and should have fixed this too, but it has a bug.
> 
> The MNTv1 MNT procedure does not return a list of security flavors,
> so our client makes up a list containing just AUTH_NULL. This should
> enable nfs_verify_authflavors() to assign the sec= specified flavor,
> but instead, it incorrectly sets it to AUTH_NULL.
> 
> I expect this would also be a problem for any NFSv3 server whose
> MNTv3 MNT procedure returned a security flavor list containing only
> AUTH_NULL.
> 
> Fixes: e68fd7c8071d ("mount: use sec= that was specified on ... ")
> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=310
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> fs/nfs/super.c |   12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Changes since v1:
> - Description edited for accuracy
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index d828ef8..6b179af 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1691,8 +1691,8 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
> 			rpc_authflavor_t *server_authlist, unsigned int count)
> {
> 	rpc_authflavor_t flavor = RPC_AUTH_MAXFLAVOR;
> +	bool found_auth_null = false;
> 	unsigned int i;
> -	int use_auth_null = false;
> 
> 	/*
> 	 * If the sec= mount option is used, the specified flavor or AUTH_NULL
> @@ -1701,6 +1701,10 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
> 	 * AUTH_NULL has a special meaning when it's in the server list - it
> 	 * means that the server will ignore the rpc creds, so any flavor
> 	 * can be used but still use the sec= that was specified.
> +	 *
> +	 * Note also that the MNT procedure in MNTv1 does not return a list
> +	 * of supported security flavors. In this case, nfs_mount() fabricates
> +	 * a security flavor list containing just AUTH_NULL.
> 	 */
> 	for (i = 0; i < count; i++) {
> 		flavor = server_authlist[i];
> @@ -1709,11 +1713,11 @@ static int nfs_verify_authflavors(struct nfs_parsed_mount_data *args,
> 			goto out;
> 
> 		if (flavor == RPC_AUTH_NULL)
> -			use_auth_null = true;
> +			found_auth_null = true;
> 	}
> 
> -	if (use_auth_null) {
> -		flavor = RPC_AUTH_NULL;
> +	if (found_auth_null) {
> +		flavor = args->auth_info.flavors[0];
> 		goto out;
> 	}
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

Re: [PATCH v2] NFS: Fix NFSv2 security settings

[Trond Myklebust]

T24gV2VkLCAyMDE3LTA4LTIzIGF0IDE1OjMxIC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
UGluZy4uLg0KDQpBcHBsaWVkIHRvIHRoZSAndGVzdGluZycgYW5kICdsaW51eC1uZXh0JyBicmFu
Y2hlcyBmb3IgdGhlIDQuMTQgbWVyZ2UNCndpbmRvdzoNCiAgaHR0cDovL2dpdC5saW51eC1uZnMu
b3JnLz9wPXRyb25kbXkvbGludXgtbmZzLmdpdDthPXNob3J0bG9nO2g9cmVmcy9oDQplYWRzL2xp
bnV4LW5leHQNCg0KPiANCj4gDQo+ID4gT24gQXVnIDEwLCAyMDE3LCBhdCA0OjQxIFBNLCBDaHVj
ayBMZXZlciA8Y2h1Y2subGV2ZXJAb3JhY2xlLmNvbT4NCj4gPiB3cm90ZToNCj4gPiANCj4gPiBG
b3IgYSB3aGlsZSBub3cgYW55IE5GU3YyIG1vdW50IHdoZXJlIHNlYz0gaXMgc3BlY2lmaWVkIHVz
ZXMNCj4gPiBBVVRIX05VTEwuIElmIHNlYz0gaXMgbm90IHNwZWNpZmllZCwgdGhlIG1vdW50IHVz
ZXMgQVVUSF9VTklYLg0KPiA+IENvbW1pdCBlNjhmZDdjODA3MWQgKCJtb3VudDogdXNlIHNlYz0g
dGhhdCB3YXMgc3BlY2lmaWVkIG9uIHRoZQ0KPiA+IGNvbW1hbmQgbGluZSIpIGF0dGVtcHRlZCB0
byBhZGRyZXNzIGEgdmVyeSBzaW1pbGFyIHByb2JsZW0gd2l0aA0KPiA+IE5GU3YzLCBhbmQgc2hv
dWxkIGhhdmUgZml4ZWQgdGhpcyB0b28sIGJ1dCBpdCBoYXMgYSBidWcuDQo+ID4gDQo+ID4gVGhl
IE1OVHYxIE1OVCBwcm9jZWR1cmUgZG9lcyBub3QgcmV0dXJuIGEgbGlzdCBvZiBzZWN1cml0eSBm
bGF2b3JzLA0KPiA+IHNvIG91ciBjbGllbnQgbWFrZXMgdXAgYSBsaXN0IGNvbnRhaW5pbmcganVz
dCBBVVRIX05VTEwuIFRoaXMNCj4gPiBzaG91bGQNCj4gPiBlbmFibGUgbmZzX3ZlcmlmeV9hdXRo
Zmxhdm9ycygpIHRvIGFzc2lnbiB0aGUgc2VjPSBzcGVjaWZpZWQNCj4gPiBmbGF2b3IsDQo+ID4g
YnV0IGluc3RlYWQsIGl0IGluY29ycmVjdGx5IHNldHMgaXQgdG8gQVVUSF9OVUxMLg0KPiA+IA0K
PiA+IEkgZXhwZWN0IHRoaXMgd291bGQgYWxzbyBiZSBhIHByb2JsZW0gZm9yIGFueSBORlN2MyBz
ZXJ2ZXIgd2hvc2UNCj4gPiBNTlR2MyBNTlQgcHJvY2VkdXJlIHJldHVybmVkIGEgc2VjdXJpdHkg
Zmxhdm9yIGxpc3QgY29udGFpbmluZyBvbmx5DQo+ID4gQVVUSF9OVUxMLg0KPiA+IA0KPiA+IEZp
eGVzOiBlNjhmZDdjODA3MWQgKCJtb3VudDogdXNlIHNlYz0gdGhhdCB3YXMgc3BlY2lmaWVkIG9u
IC4uLiAiKQ0KPiA+IEJ1Z0xpbms6IGh0dHBzOi8vYnVnemlsbGEubGludXgtbmZzLm9yZy9zaG93
X2J1Zy5jZ2k/aWQ9MzEwDQo+ID4gU2lnbmVkLW9mZi1ieTogQ2h1Y2sgTGV2ZXIgPGNodWNrLmxl
dmVyQG9yYWNsZS5jb20+DQo+ID4gLS0tDQo+ID4gZnMvbmZzL3N1cGVyLmMgfCAgIDEyICsrKysr
KysrLS0tLQ0KPiA+IDEgZmlsZSBjaGFuZ2VkLCA4IGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25z
KC0pDQo+ID4gDQo+ID4gQ2hhbmdlcyBzaW5jZSB2MToNCj4gPiAtIERlc2NyaXB0aW9uIGVkaXRl
ZCBmb3IgYWNjdXJhY3kNCj4gPiANCj4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL3N1cGVyLmMgYi9m
cy9uZnMvc3VwZXIuYw0KPiA+IGluZGV4IGQ4MjhlZjguLjZiMTc5YWYgMTAwNjQ0DQo+ID4gLS0t
IGEvZnMvbmZzL3N1cGVyLmMNCj4gPiArKysgYi9mcy9uZnMvc3VwZXIuYw0KPiA+IEBAIC0xNjkx
LDggKzE2OTEsOCBAQCBzdGF0aWMgaW50IG5mc192ZXJpZnlfYXV0aGZsYXZvcnMoc3RydWN0DQo+
ID4gbmZzX3BhcnNlZF9tb3VudF9kYXRhICphcmdzLA0KPiA+IAkJCXJwY19hdXRoZmxhdm9yX3Qg
KnNlcnZlcl9hdXRobGlzdCwgdW5zaWduZWQgaW50DQo+ID4gY291bnQpDQo+ID4gew0KPiA+IAly
cGNfYXV0aGZsYXZvcl90IGZsYXZvciA9IFJQQ19BVVRIX01BWEZMQVZPUjsNCj4gPiArCWJvb2wg
Zm91bmRfYXV0aF9udWxsID0gZmFsc2U7DQo+ID4gCXVuc2lnbmVkIGludCBpOw0KPiA+IC0JaW50
IHVzZV9hdXRoX251bGwgPSBmYWxzZTsNCj4gPiANCj4gPiAJLyoNCj4gPiAJICogSWYgdGhlIHNl
Yz0gbW91bnQgb3B0aW9uIGlzIHVzZWQsIHRoZSBzcGVjaWZpZWQgZmxhdm9yIG9yDQo+ID4gQVVU
SF9OVUxMDQo+ID4gQEAgLTE3MDEsNiArMTcwMSwxMCBAQCBzdGF0aWMgaW50IG5mc192ZXJpZnlf
YXV0aGZsYXZvcnMoc3RydWN0DQo+ID4gbmZzX3BhcnNlZF9tb3VudF9kYXRhICphcmdzLA0KPiA+
IAkgKiBBVVRIX05VTEwgaGFzIGEgc3BlY2lhbCBtZWFuaW5nIHdoZW4gaXQncyBpbiB0aGUgc2Vy
dmVyIGxpc3QNCj4gPiAtIGl0DQo+ID4gCSAqIG1lYW5zIHRoYXQgdGhlIHNlcnZlciB3aWxsIGln
bm9yZSB0aGUgcnBjIGNyZWRzLCBzbyBhbnkNCj4gPiBmbGF2b3INCj4gPiAJICogY2FuIGJlIHVz
ZWQgYnV0IHN0aWxsIHVzZSB0aGUgc2VjPSB0aGF0IHdhcyBzcGVjaWZpZWQuDQo+ID4gKwkgKg0K
PiA+ICsJICogTm90ZSBhbHNvIHRoYXQgdGhlIE1OVCBwcm9jZWR1cmUgaW4gTU5UdjEgZG9lcyBu
b3QNCj4gPiByZXR1cm4gYSBsaXN0DQo+ID4gKwkgKiBvZiBzdXBwb3J0ZWQgc2VjdXJpdHkgZmxh
dm9ycy4gSW4gdGhpcyBjYXNlLA0KPiA+IG5mc19tb3VudCgpIGZhYnJpY2F0ZXMNCj4gPiArCSAq
IGEgc2VjdXJpdHkgZmxhdm9yIGxpc3QgY29udGFpbmluZyBqdXN0IEFVVEhfTlVMTC4NCj4gPiAJ
ICovDQo+ID4gCWZvciAoaSA9IDA7IGkgPCBjb3VudDsgaSsrKSB7DQo+ID4gCQlmbGF2b3IgPSBz
ZXJ2ZXJfYXV0aGxpc3RbaV07DQo+ID4gQEAgLTE3MDksMTEgKzE3MTMsMTEgQEAgc3RhdGljIGlu
dCBuZnNfdmVyaWZ5X2F1dGhmbGF2b3JzKHN0cnVjdA0KPiA+IG5mc19wYXJzZWRfbW91bnRfZGF0
YSAqYXJncywNCj4gPiAJCQlnb3RvIG91dDsNCj4gPiANCj4gPiAJCWlmIChmbGF2b3IgPT0gUlBD
X0FVVEhfTlVMTCkNCj4gPiAtCQkJdXNlX2F1dGhfbnVsbCA9IHRydWU7DQo+ID4gKwkJCWZvdW5k
X2F1dGhfbnVsbCA9IHRydWU7DQo+ID4gCX0NCj4gPiANCj4gPiAtCWlmICh1c2VfYXV0aF9udWxs
KSB7DQo+ID4gLQkJZmxhdm9yID0gUlBDX0FVVEhfTlVMTDsNCj4gPiArCWlmIChmb3VuZF9hdXRo
X251bGwpIHsNCj4gPiArCQlmbGF2b3IgPSBhcmdzLT5hdXRoX2luZm8uZmxhdm9yc1swXTsNCj4g
PiAJCWdvdG8gb3V0Ow0KPiA+IAl9DQo+ID4gDQo+ID4gDQo+ID4gLS0NCj4gPiBUbyB1bnN1YnNj
cmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtDQo+
ID4gbmZzIiBpbg0KPiA+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5r
ZXJuZWwub3JnDQo+ID4gTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVs
Lm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQo+IA0KPiAtLQ0KPiBDaHVjayBMZXZlcg0KPiANCj4g
DQo+IA0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwg
UHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==