[PATCH 1/1] nfs-utils: Add check of clientaddr argument

[PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

If the user supplies a clientaddr value, it should be either
a special value of either IPV4/IPV6 any address or a local address
on the same network that the server being mounted. Otherwise, we
disallow the client to use an arbitrary value of the clientaddr value.
This value is used to construct a client id of SETCLIENTID and
providing a false value can interfere with the real owner's mount.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/mount/stropts.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d1b0708..44a6ff5 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
 
 /*
  * Called to discover our address and append an appropriate 'clientaddr='
- * option to the options string.
+ * option to the options string. If the supplied 'clientaddr=' value does
+ * not match either IPV4/IPv6 any or a local address, then fail the mount.
  *
  * Returns 1 if 'clientaddr=' option created successfully or if
  * 'clientaddr=' option is already present; otherwise zero.
@@ -242,11 +243,26 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
 	struct sockaddr *my_addr = &address.sa;
 	socklen_t my_len = sizeof(address);
 
-	if (po_contains(options, "clientaddr") == PO_FOUND)
-		return 1;
-
 	nfs_callback_address(sap, salen, my_addr, &my_len);
 
+	if (po_contains(options, "clientaddr") == PO_FOUND) {
+		char *addr = po_get(options, "clientaddr");
+		char address[NI_MAXHOST];
+
+		if (!strcmp(addr, "0.0.0.0") || !strcmp(addr, "::"))
+			return 1;
+		if (!nfs_present_sockaddr(my_addr, my_len, address,
+						sizeof(address)))
+			goto out;
+
+		if (strcmp(addr, address)) {
+			nfs_error(_("%s: failed to validate clientaddr "
+					"address"), progname);
+			return 0;
+		}
+		return 1;
+	}
+out:
 	return nfs_append_generic_address_option(my_addr, my_len,
 							"clientaddr", options);
 }
-- 
1.8.3.1

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

Hi Olga-

> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> =
wrote:
>=20
> If the user supplies a clientaddr value,

Please say "NFS client administrator" not "user". A
"user" is any non-privileged user, so saying that a
"user" can set this value is misleading.


> it should be either
> a special value of either IPV4/IPV6 any address or a local address
> on the same network that the server being mounted.

This option should allow any local address the client has,
not just an address that is on the same network as the
server. See below for further explanation.


> Otherwise, we
> disallow the client to use an arbitrary value of the clientaddr value.
> This value is used to construct a client id of SETCLIENTID and
> providing a false value can interfere with the real owner's mount.

The patch description is misleading:

Interference occurs only if the real owner's lease is
not protected by Kerberos AND this client has the same
client ID string as another client.

The Linux client's client ID string also contains the
system's cl_nodename. Both the cl_nodename and the
callback address have to be the same as some other
client's, and they both have to be Linux, for this to
be a problem.

It's more likely that the customer's clients are all
named the same (maybe they are copied from the same
system image), and reverse DNS lookup is giving them
all the same clientaddr=3D . That's an unsupported
configuration and there are already ways to address
this.

Or perhaps I don't understand the use case that is
causing the problem. Can the patch description explain
why your customer is trying to set clientaddr=3D ?


> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
> utils/mount/stropts.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>=20
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index d1b0708..44a6ff5 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct =
sockaddr *sap,
>=20
> /*
>  * Called to discover our address and append an appropriate =
'clientaddr=3D'
> - * option to the options string.
> + * option to the options string. If the supplied 'clientaddr=3D' =
value does
> + * not match either IPV4/IPv6 any or a local address, then fail the =
mount.
>  *
>  * Returns 1 if 'clientaddr=3D' option created successfully or if
>  * 'clientaddr=3D' option is already present; otherwise zero.
> @@ -242,11 +243,26 @@ static int nfs_append_clientaddr_option(const =
struct sockaddr *sap,
> 	struct sockaddr *my_addr =3D &address.sa;
> 	socklen_t my_len =3D sizeof(address);
>=20
> -	if (po_contains(options, "clientaddr") =3D=3D PO_FOUND)
> -		return 1;
> -
> 	nfs_callback_address(sap, salen, my_addr, &my_len);
>=20
> +	if (po_contains(options, "clientaddr") =3D=3D PO_FOUND) {
> +		char *addr =3D po_get(options, "clientaddr");
> +		char address[NI_MAXHOST];
> +
> +		if (!strcmp(addr, "0.0.0.0") || !strcmp(addr, "::"))
> +			return 1;

IN6ADDR_ANY can be spelled in other ways than "::".

Please don't compare presentation address strings.
First step is to convert the clientaddr=3D value to an
nfs_sockaddr. If that cannot be done, then clearly
this is not a valid mount option.


> +		if (!nfs_present_sockaddr(my_addr, my_len, address,
> +						sizeof(address)))
> +			goto out;
> +
> +		if (strcmp(addr, address)) {
> +			nfs_error(_("%s: failed to validate clientaddr "
> +					"address"), progname);
> +			return 0;
> +		}

This needs to check whether the supplied address is a
local address on _any_ of the client's interfaces. That's
the point of allowing an admin to set clientaddr=3D ...
sometimes the automatic setting (which comes from
nfs_callback_address) is wrong.

Think of a multi-homed client, or a client with public and
private interfaces, or a weird firewall configuration.
This check will break all those cases.

So here, use a reliable mechanism for determining whether
the passed-in clientaddr=3D value specifies the address of
one of the local interfaces on the client.


> +		return 1;
> +	}
> +out:
> 	return nfs_append_generic_address_option(my_addr, my_len,
> 							"clientaddr", =
options);
> }

--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

Thank you for the comments. Will hopefully address them in the next version.

On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> Hi Olga-
>
>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>
>> If the user supplies a clientaddr value,
>
> Please say "NFS client administrator" not "user". A
> "user" is any non-privileged user, so saying that a
> "user" can set this value is misleading.

Ok will change it.

>> it should be either
>> a special value of either IPV4/IPV6 any address or a local address
>> on the same network that the server being mounted.
>
> This option should allow any local address the client has,
> not just an address that is on the same network as the
> server. See below for further explanation.

Ok, I added this to the comment specifically as I didn't know if this
would pose a problem. I didn't know if allowing any address was useful
as when it's not specified the address on the same network as the
server is chosen.

>> Otherwise, we
>> disallow the client to use an arbitrary value of the clientaddr value.
>> This value is used to construct a client id of SETCLIENTID and
>> providing a false value can interfere with the real owner's mount.
>
> The patch description is misleading:
>
> Interference occurs only if the real owner's lease is
> not protected by Kerberos AND this client has the same
> client ID string as another client.

Ok I will add this more explicit detail when the interference occurs
(when neither of the machines are using Kerberos and the other client
machine is not using a module parameter to add a unique identifier to
the client ID string). I think otherwise it is knowns that client ID
is created with the value of the clientaddr.

> The Linux client's client ID string also contains the
> system's cl_nodename. Both the cl_nodename and the
> callback address have to be the same as some other
> client's, and they both have to be Linux, for this to
> be a problem.
>
> It's more likely that the customer's clients are all
> named the same (maybe they are copied from the same
> system image), and reverse DNS lookup is giving them
> all the same clientaddr= . That's an unsupported
> configuration and there are already ways to address
> this.
>
> Or perhaps I don't understand the use case that is
> causing the problem. Can the patch description explain
> why your customer is trying to set clientaddr= ?

The customer case was a simple mistake of including the wrong address.
Do you fundamentally disagree that there is a case for
denial-of-service here?

>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>> utils/mount/stropts.c | 24 ++++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index d1b0708..44a6ff5 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
>>
>> /*
>>  * Called to discover our address and append an appropriate 'clientaddr='
>> - * option to the options string.
>> + * option to the options string. If the supplied 'clientaddr=' value does
>> + * not match either IPV4/IPv6 any or a local address, then fail the mount.
>>  *
>>  * Returns 1 if 'clientaddr=' option created successfully or if
>>  * 'clientaddr=' option is already present; otherwise zero.
>> @@ -242,11 +243,26 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
>>       struct sockaddr *my_addr = &address.sa;
>>       socklen_t my_len = sizeof(address);
>>
>> -     if (po_contains(options, "clientaddr") == PO_FOUND)
>> -             return 1;
>> -
>>       nfs_callback_address(sap, salen, my_addr, &my_len);
>>
>> +     if (po_contains(options, "clientaddr") == PO_FOUND) {
>> +             char *addr = po_get(options, "clientaddr");
>> +             char address[NI_MAXHOST];
>> +
>> +             if (!strcmp(addr, "0.0.0.0") || !strcmp(addr, "::"))
>> +                     return 1;
>
> IN6ADDR_ANY can be spelled in other ways than "::".
>
> Please don't compare presentation address strings.
> First step is to convert the clientaddr= value to an
> nfs_sockaddr. If that cannot be done, then clearly
> this is not a valid mount option.
>
>
>> +             if (!nfs_present_sockaddr(my_addr, my_len, address,
>> +                                             sizeof(address)))
>> +                     goto out;
>> +
>> +             if (strcmp(addr, address)) {
>> +                     nfs_error(_("%s: failed to validate clientaddr "
>> +                                     "address"), progname);
>> +                     return 0;
>> +             }
>
> This needs to check whether the supplied address is a
> local address on _any_ of the client's interfaces. That's
> the point of allowing an admin to set clientaddr= ...
> sometimes the automatic setting (which comes from
> nfs_callback_address) is wrong.
>
> Think of a multi-homed client, or a client with public and
> private interfaces, or a weird firewall configuration.
> This check will break all those cases.
>
> So here, use a reliable mechanism for determining whether
> the passed-in clientaddr= value specifies the address of
> one of the local interfaces on the client.
>
>
>> +             return 1;
>> +     }
>> +out:
>>       return nfs_append_generic_address_option(my_addr, my_len,
>>                                                       "clientaddr", options);
>> }
>
> --
> Chuck Lever
>
>
>
> --
> 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

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

Hi Olga-

> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> Thank you for the comments. Will hopefully address them in the next =
version.
>=20
> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>> Hi Olga-
>>=20
>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> =
wrote:
>>>=20
>>> If the user supplies a clientaddr value,
>>=20
>> Please say "NFS client administrator" not "user". A
>> "user" is any non-privileged user, so saying that a
>> "user" can set this value is misleading.
>=20
> Ok will change it.
>=20
>>> it should be either
>>> a special value of either IPV4/IPV6 any address or a local address
>>> on the same network that the server being mounted.
>>=20
>> This option should allow any local address the client has,
>> not just an address that is on the same network as the
>> server. See below for further explanation.
>=20
> Ok, I added this to the comment specifically as I didn't know if this
> would pose a problem. I didn't know if allowing any address was useful
> as when it's not specified the address on the same network as the
> server is chosen.

Yep, any of the client's local addresses should be allowed.


>>> Otherwise, we
>>> disallow the client to use an arbitrary value of the clientaddr =
value.
>>> This value is used to construct a client id of SETCLIENTID and
>>> providing a false value can interfere with the real owner's mount.
>>=20
>> The patch description is misleading:
>>=20
>> Interference occurs only if the real owner's lease is
>> not protected by Kerberos AND this client has the same
>> client ID string as another client.
>=20
> Ok I will add this more explicit detail when the interference occurs
> (when neither of the machines are using Kerberos and the other client
> machine is not using a module parameter to add a unique identifier to
> the client ID string). I think otherwise it is knowns that client ID
> is created with the value of the clientaddr.

The only way a problem occurs is if the clientaddr is the
same AND the cl_nodename is the same. How is that happening?
Why are the cl_nodenames the same? If they are not the same,
then it is not possible that the clients' leases are inter-
fering with each other, and something else is going on.


>> The Linux client's client ID string also contains the
>> system's cl_nodename. Both the cl_nodename and the
>> callback address have to be the same as some other
>> client's, and they both have to be Linux, for this to
>> be a problem.
>>=20
>> It's more likely that the customer's clients are all
>> named the same (maybe they are copied from the same
>> system image), and reverse DNS lookup is giving them
>> all the same clientaddr=3D . That's an unsupported
>> configuration and there are already ways to address
>> this.
>>=20
>> Or perhaps I don't understand the use case that is
>> causing the problem. Can the patch description explain
>> why your customer is trying to set clientaddr=3D ?
>=20
> The customer case was a simple mistake of including the wrong address.

But that doesn't answer the question. Why did the
customer feel the need to set clientaddr=3D ?


> Do you fundamentally disagree that there is a case for
> denial-of-service here?

The only service that is affected if the clientaddr is
set incorrectly is on the client where the mistake
occurs. If the cl_nodenames are all unique then other
clients should not be affected by the mistake. If
that is happening, that's a server bug.

If the problem was that the customer set the wrong
address, let's say that, rather than claiming that the
patch prevents lease tampering.


--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> Hi Olga-
>
>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>
>> Thank you for the comments. Will hopefully address them in the next version.
>>
>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>> Hi Olga-
>>>
>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>>>
>>>> If the user supplies a clientaddr value,
>>>
>>> Please say "NFS client administrator" not "user". A
>>> "user" is any non-privileged user, so saying that a
>>> "user" can set this value is misleading.
>>
>> Ok will change it.
>>
>>>> it should be either
>>>> a special value of either IPV4/IPV6 any address or a local address
>>>> on the same network that the server being mounted.
>>>
>>> This option should allow any local address the client has,
>>> not just an address that is on the same network as the
>>> server. See below for further explanation.
>>
>> Ok, I added this to the comment specifically as I didn't know if this
>> would pose a problem. I didn't know if allowing any address was useful
>> as when it's not specified the address on the same network as the
>> server is chosen.
>
> Yep, any of the client's local addresses should be allowed.
>
>
>>>> Otherwise, we
>>>> disallow the client to use an arbitrary value of the clientaddr value.
>>>> This value is used to construct a client id of SETCLIENTID and
>>>> providing a false value can interfere with the real owner's mount.
>>>
>>> The patch description is misleading:
>>>
>>> Interference occurs only if the real owner's lease is
>>> not protected by Kerberos AND this client has the same
>>> client ID string as another client.
>>
>> Ok I will add this more explicit detail when the interference occurs
>> (when neither of the machines are using Kerberos and the other client
>> machine is not using a module parameter to add a unique identifier to
>> the client ID string). I think otherwise it is knowns that client ID
>> is created with the value of the clientaddr.
>
> The only way a problem occurs is if the clientaddr is the
> same AND the cl_nodename is the same. How is that happening?

Client ID in the SETCLIENTID is constructed by
nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
and not cl_nodename.

> Why are the cl_nodenames the same? If they are not the same,
> then it is not possible that the clients' leases are inter-
> fering with each other, and something else is going on.
>
>
>>> The Linux client's client ID string also contains the
>>> system's cl_nodename. Both the cl_nodename and the
>>> callback address have to be the same as some other
>>> client's, and they both have to be Linux, for this to
>>> be a problem.
>>>
>>> It's more likely that the customer's clients are all
>>> named the same (maybe they are copied from the same
>>> system image), and reverse DNS lookup is giving them
>>> all the same clientaddr= . That's an unsupported
>>> configuration and there are already ways to address
>>> this.
>>>
>>> Or perhaps I don't understand the use case that is
>>> causing the problem. Can the patch description explain
>>> why your customer is trying to set clientaddr= ?
>>
>> The customer case was a simple mistake of including the wrong address.
>
> But that doesn't answer the question. Why did the
> customer feel the need to set clientaddr= ?

I don't know. In the end they decided they didn't need the clientaddr at all.

>> Do you fundamentally disagree that there is a case for
>> denial-of-service here?
>
> The only service that is affected if the clientaddr is
> set incorrectly is on the client where the mistake
> occurs. If the cl_nodenames are all unique then other
> clients should not be affected by the mistake. If
> that is happening, that's a server bug.
>
> If the problem was that the customer set the wrong
> address, let's say that, rather than claiming that the
> patch prevents lease tampering.

Ok I can change it to lease tampering (I really don't care that much).
But to just to discuss a bit further, how's lease tampering not a
denial-of-service? It interfere with a client's ability to make
progress.

>
>
> --
> Chuck Lever
>
>
>

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

On Fri, May 25, 2018 at 12:44 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>> Hi Olga-
>>
>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>
>>> Thank you for the comments. Will hopefully address them in the next version.
>>>
>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> Hi Olga-
>>>>
>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>>>>
>>>>> If the user supplies a clientaddr value,
>>>>
>>>> Please say "NFS client administrator" not "user". A
>>>> "user" is any non-privileged user, so saying that a
>>>> "user" can set this value is misleading.
>>>
>>> Ok will change it.
>>>
>>>>> it should be either
>>>>> a special value of either IPV4/IPV6 any address or a local address
>>>>> on the same network that the server being mounted.
>>>>
>>>> This option should allow any local address the client has,
>>>> not just an address that is on the same network as the
>>>> server. See below for further explanation.
>>>
>>> Ok, I added this to the comment specifically as I didn't know if this
>>> would pose a problem. I didn't know if allowing any address was useful
>>> as when it's not specified the address on the same network as the
>>> server is chosen.
>>
>> Yep, any of the client's local addresses should be allowed.
>>
>>
>>>>> Otherwise, we
>>>>> disallow the client to use an arbitrary value of the clientaddr value.
>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>> providing a false value can interfere with the real owner's mount.
>>>>
>>>> The patch description is misleading:
>>>>
>>>> Interference occurs only if the real owner's lease is
>>>> not protected by Kerberos AND this client has the same
>>>> client ID string as another client.
>>>
>>> Ok I will add this more explicit detail when the interference occurs
>>> (when neither of the machines are using Kerberos and the other client
>>> machine is not using a module parameter to add a unique identifier to
>>> the client ID string). I think otherwise it is knowns that client ID
>>> is created with the value of the clientaddr.
>>
>> The only way a problem occurs is if the clientaddr is the
>> same AND the cl_nodename is the same. How is that happening?
>
> Client ID in the SETCLIENTID is constructed by
> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
> and not cl_nodename.
>
>> Why are the cl_nodenames the same? If they are not the same,
>> then it is not possible that the clients' leases are inter-
>> fering with each other, and something else is going on.
>>
>>
>>>> The Linux client's client ID string also contains the
>>>> system's cl_nodename. Both the cl_nodename and the
>>>> callback address have to be the same as some other
>>>> client's, and they both have to be Linux, for this to
>>>> be a problem.
>>>>
>>>> It's more likely that the customer's clients are all
>>>> named the same (maybe they are copied from the same
>>>> system image), and reverse DNS lookup is giving them
>>>> all the same clientaddr= . That's an unsupported
>>>> configuration and there are already ways to address
>>>> this.
>>>>
>>>> Or perhaps I don't understand the use case that is
>>>> causing the problem. Can the patch description explain
>>>> why your customer is trying to set clientaddr= ?
>>>
>>> The customer case was a simple mistake of including the wrong address.
>>
>> But that doesn't answer the question. Why did the
>> customer feel the need to set clientaddr= ?
>
> I don't know. In the end they decided they didn't need the clientaddr at all.
>
>>> Do you fundamentally disagree that there is a case for
>>> denial-of-service here?
>>
>> The only service that is affected if the clientaddr is

Forgot to address this. This is definitely not true. Both clients are
effected as they end up "sharing" the lease. So each client once it
gets a lease error has to go and renegotiate the lease and can perhaps
get an operation in before the other client will send something which
would then get and error and it would deals with the lease error.

>> set incorrectly is on the client where the mistake
>> occurs. If the cl_nodenames are all unique then other
>> clients should not be affected by the mistake. If
>> that is happening, that's a server bug.
>>
>> If the problem was that the customer set the wrong
>> address, let's say that, rather than claiming that the
>> patch prevents lease tampering.
>
> Ok I can change it to lease tampering (I really don't care that much).
> But to just to discuss a bit further, how's lease tampering not a
> denial-of-service? It interfere with a client's ability to make
> progress.
>
>>
>>
>> --
>> Chuck Lever
>>
>>
>>

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On May 25, 2018, at 9:47 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Fri, May 25, 2018 at 12:44 PM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>> Hi Olga-
>>>=20
>>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>>=20
>>>> Thank you for the comments. Will hopefully address them in the next =
version.
>>>>=20
>>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>>> Hi Olga-
>>>>>=20
>>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> =
wrote:
>>>>>>=20
>>>>>> If the user supplies a clientaddr value,
>>>>>=20
>>>>> Please say "NFS client administrator" not "user". A
>>>>> "user" is any non-privileged user, so saying that a
>>>>> "user" can set this value is misleading.
>>>>=20
>>>> Ok will change it.
>>>>=20
>>>>>> it should be either
>>>>>> a special value of either IPV4/IPV6 any address or a local =
address
>>>>>> on the same network that the server being mounted.
>>>>>=20
>>>>> This option should allow any local address the client has,
>>>>> not just an address that is on the same network as the
>>>>> server. See below for further explanation.
>>>>=20
>>>> Ok, I added this to the comment specifically as I didn't know if =
this
>>>> would pose a problem. I didn't know if allowing any address was =
useful
>>>> as when it's not specified the address on the same network as the
>>>> server is chosen.
>>>=20
>>> Yep, any of the client's local addresses should be allowed.
>>>=20
>>>=20
>>>>>> Otherwise, we
>>>>>> disallow the client to use an arbitrary value of the clientaddr =
value.
>>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>>> providing a false value can interfere with the real owner's =
mount.
>>>>>=20
>>>>> The patch description is misleading:
>>>>>=20
>>>>> Interference occurs only if the real owner's lease is
>>>>> not protected by Kerberos AND this client has the same
>>>>> client ID string as another client.
>>>>=20
>>>> Ok I will add this more explicit detail when the interference =
occurs
>>>> (when neither of the machines are using Kerberos and the other =
client
>>>> machine is not using a module parameter to add a unique identifier =
to
>>>> the client ID string). I think otherwise it is knowns that client =
ID
>>>> is created with the value of the clientaddr.
>>>=20
>>> The only way a problem occurs is if the clientaddr is the
>>> same AND the cl_nodename is the same. How is that happening?
>>=20
>> Client ID in the SETCLIENTID is constructed by
>> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
>> and not cl_nodename.
>>=20
>>> Why are the cl_nodenames the same? If they are not the same,
>>> then it is not possible that the clients' leases are inter-
>>> fering with each other, and something else is going on.
>>>=20
>>>=20
>>>>> The Linux client's client ID string also contains the
>>>>> system's cl_nodename. Both the cl_nodename and the
>>>>> callback address have to be the same as some other
>>>>> client's, and they both have to be Linux, for this to
>>>>> be a problem.
>>>>>=20
>>>>> It's more likely that the customer's clients are all
>>>>> named the same (maybe they are copied from the same
>>>>> system image), and reverse DNS lookup is giving them
>>>>> all the same clientaddr=3D . That's an unsupported
>>>>> configuration and there are already ways to address
>>>>> this.
>>>>>=20
>>>>> Or perhaps I don't understand the use case that is
>>>>> causing the problem. Can the patch description explain
>>>>> why your customer is trying to set clientaddr=3D ?
>>>>=20
>>>> The customer case was a simple mistake of including the wrong =
address.
>>>=20
>>> But that doesn't answer the question. Why did the
>>> customer feel the need to set clientaddr=3D ?
>>=20
>> I don't know. In the end they decided they didn't need the clientaddr =
at all.
>>=20
>>>> Do you fundamentally disagree that there is a case for
>>>> denial-of-service here?
>>>=20
>>> The only service that is affected if the clientaddr is
>=20
> Forgot to address this. This is definitely not true. Both clients are
> effected as they end up "sharing" the lease. So each client once it
> gets a lease error has to go and renegotiate the lease and can perhaps
> get an operation in before the other client will send something which
> would then get and error and it would deals with the lease error.

My claim is correct for the uniform client ID string. I
did not realize that the non-uniform client ID string
is not well-constructed.


>>> set incorrectly is on the client where the mistake
>>> occurs. If the cl_nodenames are all unique then other
>>> clients should not be affected by the mistake. If
>>> that is happening, that's a server bug.
>>>=20
>>> If the problem was that the customer set the wrong
>>> address, let's say that, rather than claiming that the
>>> patch prevents lease tampering.
>>=20
>> Ok I can change it to lease tampering (I really don't care that =
much).
>> But to just to discuss a bit further, how's lease tampering not a
>> denial-of-service? It interfere with a client's ability to make
>> progress.
>>=20
>>>=20
>>>=20
>>> --
>>> Chuck Lever

--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

On Fri, May 25, 2018 at 1:05 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>> On May 25, 2018, at 9:47 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>
>> On Fri, May 25, 2018 at 12:44 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> Hi Olga-
>>>>
>>>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>
>>>>> Thank you for the comments. Will hopefully address them in the next version.
>>>>>
>>>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>> Hi Olga-
>>>>>>
>>>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>>>>>>
>>>>>>> If the user supplies a clientaddr value,
>>>>>>
>>>>>> Please say "NFS client administrator" not "user". A
>>>>>> "user" is any non-privileged user, so saying that a
>>>>>> "user" can set this value is misleading.
>>>>>
>>>>> Ok will change it.
>>>>>
>>>>>>> it should be either
>>>>>>> a special value of either IPV4/IPV6 any address or a local address
>>>>>>> on the same network that the server being mounted.
>>>>>>
>>>>>> This option should allow any local address the client has,
>>>>>> not just an address that is on the same network as the
>>>>>> server. See below for further explanation.
>>>>>
>>>>> Ok, I added this to the comment specifically as I didn't know if this
>>>>> would pose a problem. I didn't know if allowing any address was useful
>>>>> as when it's not specified the address on the same network as the
>>>>> server is chosen.
>>>>
>>>> Yep, any of the client's local addresses should be allowed.
>>>>
>>>>
>>>>>>> Otherwise, we
>>>>>>> disallow the client to use an arbitrary value of the clientaddr value.
>>>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>>>> providing a false value can interfere with the real owner's mount.
>>>>>>
>>>>>> The patch description is misleading:
>>>>>>
>>>>>> Interference occurs only if the real owner's lease is
>>>>>> not protected by Kerberos AND this client has the same
>>>>>> client ID string as another client.
>>>>>
>>>>> Ok I will add this more explicit detail when the interference occurs
>>>>> (when neither of the machines are using Kerberos and the other client
>>>>> machine is not using a module parameter to add a unique identifier to
>>>>> the client ID string). I think otherwise it is knowns that client ID
>>>>> is created with the value of the clientaddr.
>>>>
>>>> The only way a problem occurs is if the clientaddr is the
>>>> same AND the cl_nodename is the same. How is that happening?
>>>
>>> Client ID in the SETCLIENTID is constructed by
>>> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
>>> and not cl_nodename.
>>>
>>>> Why are the cl_nodenames the same? If they are not the same,
>>>> then it is not possible that the clients' leases are inter-
>>>> fering with each other, and something else is going on.
>>>>
>>>>
>>>>>> The Linux client's client ID string also contains the
>>>>>> system's cl_nodename. Both the cl_nodename and the
>>>>>> callback address have to be the same as some other
>>>>>> client's, and they both have to be Linux, for this to
>>>>>> be a problem.
>>>>>>
>>>>>> It's more likely that the customer's clients are all
>>>>>> named the same (maybe they are copied from the same
>>>>>> system image), and reverse DNS lookup is giving them
>>>>>> all the same clientaddr= . That's an unsupported
>>>>>> configuration and there are already ways to address
>>>>>> this.
>>>>>>
>>>>>> Or perhaps I don't understand the use case that is
>>>>>> causing the problem. Can the patch description explain
>>>>>> why your customer is trying to set clientaddr= ?
>>>>>
>>>>> The customer case was a simple mistake of including the wrong address.
>>>>
>>>> But that doesn't answer the question. Why did the
>>>> customer feel the need to set clientaddr= ?
>>>
>>> I don't know. In the end they decided they didn't need the clientaddr at all.
>>>
>>>>> Do you fundamentally disagree that there is a case for
>>>>> denial-of-service here?
>>>>
>>>> The only service that is affected if the clientaddr is
>>
>> Forgot to address this. This is definitely not true. Both clients are
>> effected as they end up "sharing" the lease. So each client once it
>> gets a lease error has to go and renegotiate the lease and can perhaps
>> get an operation in before the other client will send something which
>> would then get and error and it would deals with the lease error.
>
> My claim is correct for the uniform client ID string.

Agreed!

> I did not realize that the non-uniform client ID string
> is not well-constructed.

I'm glad we are finally on the same page :-)

>
>
>>>> set incorrectly is on the client where the mistake
>>>> occurs. If the cl_nodenames are all unique then other
>>>> clients should not be affected by the mistake. If
>>>> that is happening, that's a server bug.
>>>>
>>>> If the problem was that the customer set the wrong
>>>> address, let's say that, rather than claiming that the
>>>> patch prevents lease tampering.
>>>
>>> Ok I can change it to lease tampering (I really don't care that much).
>>> But to just to discuss a bit further, how's lease tampering not a
>>> denial-of-service? It interfere with a client's ability to make
>>> progress.
>>>
>>>>
>>>>
>>>> --
>>>> Chuck Lever
>
> --
> Chuck Lever
>
>
>

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On May 25, 2018, at 9:44 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>> Hi Olga-
>>=20
>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>=20
>>> Thank you for the comments. Will hopefully address them in the next =
version.
>>>=20
>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>> Hi Olga-
>>>>=20
>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> =
wrote:
>>>>>=20
>>>>> If the user supplies a clientaddr value,
>>>>=20
>>>> Please say "NFS client administrator" not "user". A
>>>> "user" is any non-privileged user, so saying that a
>>>> "user" can set this value is misleading.
>>>=20
>>> Ok will change it.
>>>=20
>>>>> it should be either
>>>>> a special value of either IPV4/IPV6 any address or a local address
>>>>> on the same network that the server being mounted.
>>>>=20
>>>> This option should allow any local address the client has,
>>>> not just an address that is on the same network as the
>>>> server. See below for further explanation.
>>>=20
>>> Ok, I added this to the comment specifically as I didn't know if =
this
>>> would pose a problem. I didn't know if allowing any address was =
useful
>>> as when it's not specified the address on the same network as the
>>> server is chosen.
>>=20
>> Yep, any of the client's local addresses should be allowed.
>>=20
>>=20
>>>>> Otherwise, we
>>>>> disallow the client to use an arbitrary value of the clientaddr =
value.
>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>> providing a false value can interfere with the real owner's mount.
>>>>=20
>>>> The patch description is misleading:
>>>>=20
>>>> Interference occurs only if the real owner's lease is
>>>> not protected by Kerberos AND this client has the same
>>>> client ID string as another client.
>>>=20
>>> Ok I will add this more explicit detail when the interference occurs
>>> (when neither of the machines are using Kerberos and the other =
client
>>> machine is not using a module parameter to add a unique identifier =
to
>>> the client ID string). I think otherwise it is knowns that client ID
>>> is created with the value of the clientaddr.
>>=20
>> The only way a problem occurs is if the clientaddr is the
>> same AND the cl_nodename is the same. How is that happening?
>=20
> Client ID in the SETCLIENTID is constructed by
> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
> and not cl_nodename.

5614         scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
5615                         clp->cl_ipaddr,
5616                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_ADDR),
5617                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_PROTO));

So I get your point now: if two Linux NFSv4.0 clients mount the
same server and specify the same callback address, and do not
specify "migration", they will indeed present exactly the same
client ID string to the server.

I had remembered there was more in that string, but I guess I
was thinking of the uniform client ID string.

And it will break badly if several clients decide to use
clientaddr=3D0.0.0.0. Trond, any thoughts? Why aren't the
nodename or uniquifier available in this string?


>> Why are the cl_nodenames the same? If they are not the same,
>> then it is not possible that the clients' leases are inter-
>> fering with each other, and something else is going on.
>>=20
>>=20
>>>> The Linux client's client ID string also contains the
>>>> system's cl_nodename. Both the cl_nodename and the
>>>> callback address have to be the same as some other
>>>> client's, and they both have to be Linux, for this to
>>>> be a problem.
>>>>=20
>>>> It's more likely that the customer's clients are all
>>>> named the same (maybe they are copied from the same
>>>> system image), and reverse DNS lookup is giving them
>>>> all the same clientaddr=3D . That's an unsupported
>>>> configuration and there are already ways to address
>>>> this.
>>>>=20
>>>> Or perhaps I don't understand the use case that is
>>>> causing the problem. Can the patch description explain
>>>> why your customer is trying to set clientaddr=3D ?
>>>=20
>>> The customer case was a simple mistake of including the wrong =
address.
>>=20
>> But that doesn't answer the question. Why did the
>> customer feel the need to set clientaddr=3D ?
>=20
> I don't know. In the end they decided they didn't need the clientaddr =
at all.
>=20
>>> Do you fundamentally disagree that there is a case for
>>> denial-of-service here?
>>=20
>> The only service that is affected if the clientaddr is
>> set incorrectly is on the client where the mistake
>> occurs. If the cl_nodenames are all unique then other
>> clients should not be affected by the mistake. If
>> that is happening, that's a server bug.
>>=20
>> If the problem was that the customer set the wrong
>> address, let's say that, rather than claiming that the
>> patch prevents lease tampering.
>=20
> Ok I can change it to lease tampering (I really don't care that much).
> But to just to discuss a bit further, how's lease tampering not a
> denial-of-service? It interfere with a client's ability to make
> progress.


--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On May 25, 2018, at 10:04 AM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>=20
>=20
>=20
>> On May 25, 2018, at 9:44 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>=20
>> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>> Hi Olga-
>>>=20
>>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>>=20
>>>> Thank you for the comments. Will hopefully address them in the next =
version.
>>>>=20
>>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>>> Hi Olga-
>>>>>=20
>>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> =
wrote:
>>>>>>=20
>>>>>> If the user supplies a clientaddr value,
>>>>>=20
>>>>> Please say "NFS client administrator" not "user". A
>>>>> "user" is any non-privileged user, so saying that a
>>>>> "user" can set this value is misleading.
>>>>=20
>>>> Ok will change it.
>>>>=20
>>>>>> it should be either
>>>>>> a special value of either IPV4/IPV6 any address or a local =
address
>>>>>> on the same network that the server being mounted.
>>>>>=20
>>>>> This option should allow any local address the client has,
>>>>> not just an address that is on the same network as the
>>>>> server. See below for further explanation.
>>>>=20
>>>> Ok, I added this to the comment specifically as I didn't know if =
this
>>>> would pose a problem. I didn't know if allowing any address was =
useful
>>>> as when it's not specified the address on the same network as the
>>>> server is chosen.
>>>=20
>>> Yep, any of the client's local addresses should be allowed.
>>>=20
>>>=20
>>>>>> Otherwise, we
>>>>>> disallow the client to use an arbitrary value of the clientaddr =
value.
>>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>>> providing a false value can interfere with the real owner's =
mount.
>>>>>=20
>>>>> The patch description is misleading:
>>>>>=20
>>>>> Interference occurs only if the real owner's lease is
>>>>> not protected by Kerberos AND this client has the same
>>>>> client ID string as another client.
>>>>=20
>>>> Ok I will add this more explicit detail when the interference =
occurs
>>>> (when neither of the machines are using Kerberos and the other =
client
>>>> machine is not using a module parameter to add a unique identifier =
to
>>>> the client ID string). I think otherwise it is knowns that client =
ID
>>>> is created with the value of the clientaddr.
>>>=20
>>> The only way a problem occurs is if the clientaddr is the
>>> same AND the cl_nodename is the same. How is that happening?
>>=20
>> Client ID in the SETCLIENTID is constructed by
>> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
>> and not cl_nodename.
>=20
> 5614         scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
> 5615                         clp->cl_ipaddr,
> 5616                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_ADDR),
> 5617                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_PROTO));
>=20
> So I get your point now: if two Linux NFSv4.0 clients mount the
> same server and specify the same callback address, and do not
> specify "migration", they will indeed present exactly the same
> client ID string to the server.
>=20
> I had remembered there was more in that string, but I guess I
> was thinking of the uniform client ID string.
>=20
> And it will break badly if several clients decide to use
> clientaddr=3D0.0.0.0. Trond, any thoughts? Why aren't the
> nodename or uniquifier available in this string?

I can think of legitimate cases where two unique NFSv4.0
clients having the same IP address might mount the same
server.

- clientaddr=3D0.0.0.0, as mentioned above

- clients behind NATs using a private subnet that happen
to be numbered the same (192.168.0.77, say).

Restricting the addresses allowed as the clientaddr=3D
value does not address these cases.


We already have some workarounds:

- Use NFSv4.1 or later

- Use NFSv4.0 with the "migration" option

- Use Kerberos (give the clients and server service
  principals) and fix the server to reject SETCLIENTID
  using a recognized client ID string but an unrecognized
  authentication flavor and principal

And possible fixes might include:

- Improve the Linux client's non-UCS format

- Make "migration" the default behavior

I am still in favor of validating the clientaddr value,
but again, IMO that just papers over the real problem,
which is the current non-UCS client ID string format.

Have a restful holiday weekend.


>>> Why are the cl_nodenames the same? If they are not the same,
>>> then it is not possible that the clients' leases are inter-
>>> fering with each other, and something else is going on.
>>>=20
>>>=20
>>>>> The Linux client's client ID string also contains the
>>>>> system's cl_nodename. Both the cl_nodename and the
>>>>> callback address have to be the same as some other
>>>>> client's, and they both have to be Linux, for this to
>>>>> be a problem.
>>>>>=20
>>>>> It's more likely that the customer's clients are all
>>>>> named the same (maybe they are copied from the same
>>>>> system image), and reverse DNS lookup is giving them
>>>>> all the same clientaddr=3D . That's an unsupported
>>>>> configuration and there are already ways to address
>>>>> this.
>>>>>=20
>>>>> Or perhaps I don't understand the use case that is
>>>>> causing the problem. Can the patch description explain
>>>>> why your customer is trying to set clientaddr=3D ?
>>>>=20
>>>> The customer case was a simple mistake of including the wrong =
address.
>>>=20
>>> But that doesn't answer the question. Why did the
>>> customer feel the need to set clientaddr=3D ?
>>=20
>> I don't know. In the end they decided they didn't need the clientaddr =
at all.
>>=20
>>>> Do you fundamentally disagree that there is a case for
>>>> denial-of-service here?
>>>=20
>>> The only service that is affected if the clientaddr is
>>> set incorrectly is on the client where the mistake
>>> occurs. If the cl_nodenames are all unique then other
>>> clients should not be affected by the mistake. If
>>> that is happening, that's a server bug.
>>>=20
>>> If the problem was that the customer set the wrong
>>> address, let's say that, rather than claiming that the
>>> patch prevents lease tampering.
>>=20
>> Ok I can change it to lease tampering (I really don't care that =
much).
>> But to just to discuss a bit further, how's lease tampering not a
>> denial-of-service? It interfere with a client's ability to make
>> progress.
>=20
>=20
> --
> Chuck Lever
>=20
>=20
>=20
> --
> 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 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

On Fri, May 25, 2018 at 6:35 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>> On May 25, 2018, at 10:04 AM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>
>>
>>> On May 25, 2018, at 9:44 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>
>>> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> Hi Olga-
>>>>
>>>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>>
>>>>> Thank you for the comments. Will hopefully address them in the next version.
>>>>>
>>>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>> Hi Olga-
>>>>>>
>>>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@netapp.com> wrote:
>>>>>>>
>>>>>>> If the user supplies a clientaddr value,
>>>>>>
>>>>>> Please say "NFS client administrator" not "user". A
>>>>>> "user" is any non-privileged user, so saying that a
>>>>>> "user" can set this value is misleading.
>>>>>
>>>>> Ok will change it.
>>>>>
>>>>>>> it should be either
>>>>>>> a special value of either IPV4/IPV6 any address or a local address
>>>>>>> on the same network that the server being mounted.
>>>>>>
>>>>>> This option should allow any local address the client has,
>>>>>> not just an address that is on the same network as the
>>>>>> server. See below for further explanation.
>>>>>
>>>>> Ok, I added this to the comment specifically as I didn't know if this
>>>>> would pose a problem. I didn't know if allowing any address was useful
>>>>> as when it's not specified the address on the same network as the
>>>>> server is chosen.
>>>>
>>>> Yep, any of the client's local addresses should be allowed.
>>>>
>>>>
>>>>>>> Otherwise, we
>>>>>>> disallow the client to use an arbitrary value of the clientaddr value.
>>>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>>>> providing a false value can interfere with the real owner's mount.
>>>>>>
>>>>>> The patch description is misleading:
>>>>>>
>>>>>> Interference occurs only if the real owner's lease is
>>>>>> not protected by Kerberos AND this client has the same
>>>>>> client ID string as another client.
>>>>>
>>>>> Ok I will add this more explicit detail when the interference occurs
>>>>> (when neither of the machines are using Kerberos and the other client
>>>>> machine is not using a module parameter to add a unique identifier to
>>>>> the client ID string). I think otherwise it is knowns that client ID
>>>>> is created with the value of the clientaddr.
>>>>
>>>> The only way a problem occurs is if the clientaddr is the
>>>> same AND the cl_nodename is the same. How is that happening?
>>>
>>> Client ID in the SETCLIENTID is constructed by
>>> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
>>> and not cl_nodename.
>>
>> 5614         scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
>> 5615                         clp->cl_ipaddr,
>> 5616                         rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
>> 5617                         rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO));
>>
>> So I get your point now: if two Linux NFSv4.0 clients mount the
>> same server and specify the same callback address, and do not
>> specify "migration", they will indeed present exactly the same
>> client ID string to the server.
>>
>> I had remembered there was more in that string, but I guess I
>> was thinking of the uniform client ID string.
>>
>> And it will break badly if several clients decide to use
>> clientaddr=0.0.0.0. Trond, any thoughts? Why aren't the
>> nodename or uniquifier available in this string?
>
> I can think of legitimate cases where two unique NFSv4.0
> clients having the same IP address might mount the same
> server.
>
> - clientaddr=0.0.0.0, as mentioned above

I think this should be prohibited. If this is used a way to signify to
the server to no give out delegations, then there could be other means
of doing so. Let's add a mount option 'nodeleg', client would send a
valid callback info to the server but if the option is set, then it
will not start a callback server. That would prevent the server from
being able to establish a callback path (which is the same thing as
sending 0.0.0.0).

> - clients behind NATs using a private subnet that happen
> to be numbered the same (192.168.0.77, say).

Are we talking about a scenario where we have two private network
which are numbered the same sitting behind a NAT-ed routers and both
then accessing an NFS server on the internet? So how about then adding
a MAC address?

> Restricting the addresses allowed as the clientaddr=
> value does not address these cases.

> We already have some workarounds:

Since this is really a not a "bug" but an attack, I think having
workarounds is not sufficient. I think whatever it is needs to be
enforced.

> - Use NFSv4.1 or later
>
> - Use NFSv4.0 with the "migration" option
>
> - Use Kerberos (give the clients and server service
>   principals) and fix the server to reject SETCLIENTID
>   using a recognized client ID string but an unrecognized
>   authentication flavor and principal
>
> And possible fixes might include:
>
> - Improve the Linux client's non-UCS format

How about adding a MAC address to the string.

> - Make "migration" the default behavior

That would mean we totally scrap the use of
nfs4_init_nonuniform_string(). Spec suggestions that client ip, server
ip, + others to be used in creation of the client ID. A uniform client
string does not include client ip + server ip (+transport). We can't
assume existence of the unique identifier, so the only thing that'll
be there is a cl_nodename. Can it be assumed to always be configured
correctly? Can't this value frequently be just "localhost.localhost"?

How about instead of having uniform and non-uniform have just a single
function that will include all of it client ip, server ip, transport,
unique identifier (if present), and cl_nodename (and add a mac)? Of
course all if it will have to fit into 1024 opaque limit size.


> I am still in favor of validating the clientaddr value,
> but again, IMO that just papers over the real problem,
> which is the current non-UCS client ID string format.

I think at the nfs-utils, we can still check that supplied value is
one of the network addresses machine has (do not allow 0.0.0.0 or also
probably not 127.0.0.1 (or ipv6 equivalent)). I'm not sure if mac
should be queried and passed into the kernel or the kernel acquires
it.


> Have a restful holiday weekend.
>
>
>>>> Why are the cl_nodenames the same? If they are not the same,
>>>> then it is not possible that the clients' leases are inter-
>>>> fering with each other, and something else is going on.
>>>>
>>>>
>>>>>> The Linux client's client ID string also contains the
>>>>>> system's cl_nodename. Both the cl_nodename and the
>>>>>> callback address have to be the same as some other
>>>>>> client's, and they both have to be Linux, for this to
>>>>>> be a problem.
>>>>>>
>>>>>> It's more likely that the customer's clients are all
>>>>>> named the same (maybe they are copied from the same
>>>>>> system image), and reverse DNS lookup is giving them
>>>>>> all the same clientaddr= . That's an unsupported
>>>>>> configuration and there are already ways to address
>>>>>> this.
>>>>>>
>>>>>> Or perhaps I don't understand the use case that is
>>>>>> causing the problem. Can the patch description explain
>>>>>> why your customer is trying to set clientaddr= ?
>>>>>
>>>>> The customer case was a simple mistake of including the wrong address.
>>>>
>>>> But that doesn't answer the question. Why did the
>>>> customer feel the need to set clientaddr= ?
>>>
>>> I don't know. In the end they decided they didn't need the clientaddr at all.
>>>
>>>>> Do you fundamentally disagree that there is a case for
>>>>> denial-of-service here?
>>>>
>>>> The only service that is affected if the clientaddr is
>>>> set incorrectly is on the client where the mistake
>>>> occurs. If the cl_nodenames are all unique then other
>>>> clients should not be affected by the mistake. If
>>>> that is happening, that's a server bug.
>>>>
>>>> If the problem was that the customer set the wrong
>>>> address, let's say that, rather than claiming that the
>>>> patch prevents lease tampering.
>>>
>>> Ok I can change it to lease tampering (I really don't care that much).
>>> But to just to discuss a bit further, how's lease tampering not a
>>> denial-of-service? It interfere with a client's ability to make
>>> progress.
>>
>>
>> --
>> Chuck Lever
>>
>>
>>
>> --
>> 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 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On May 29, 2018, at 4:07 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Fri, May 25, 2018 at 6:35 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>=20
>>=20
>>> On May 25, 2018, at 10:04 AM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>>=20
>>>=20
>>>=20
>>>> On May 25, 2018, at 9:44 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>>=20
>>>> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>>> Hi Olga-
>>>>>=20
>>>>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>>>>=20
>>>>>> Thank you for the comments. Will hopefully address them in the =
next version.
>>>>>>=20
>>>>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>>>>> Hi Olga-
>>>>>>>=20
>>>>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia =
<kolga@netapp.com> wrote:
>>>>>>>>=20
>>>>>>>> If the user supplies a clientaddr value,
>>>>>>>=20
>>>>>>> Please say "NFS client administrator" not "user". A
>>>>>>> "user" is any non-privileged user, so saying that a
>>>>>>> "user" can set this value is misleading.
>>>>>>=20
>>>>>> Ok will change it.
>>>>>>=20
>>>>>>>> it should be either
>>>>>>>> a special value of either IPV4/IPV6 any address or a local =
address
>>>>>>>> on the same network that the server being mounted.
>>>>>>>=20
>>>>>>> This option should allow any local address the client has,
>>>>>>> not just an address that is on the same network as the
>>>>>>> server. See below for further explanation.
>>>>>>=20
>>>>>> Ok, I added this to the comment specifically as I didn't know if =
this
>>>>>> would pose a problem. I didn't know if allowing any address was =
useful
>>>>>> as when it's not specified the address on the same network as the
>>>>>> server is chosen.
>>>>>=20
>>>>> Yep, any of the client's local addresses should be allowed.
>>>>>=20
>>>>>=20
>>>>>>>> Otherwise, we
>>>>>>>> disallow the client to use an arbitrary value of the clientaddr =
value.
>>>>>>>> This value is used to construct a client id of SETCLIENTID and
>>>>>>>> providing a false value can interfere with the real owner's =
mount.
>>>>>>>=20
>>>>>>> The patch description is misleading:
>>>>>>>=20
>>>>>>> Interference occurs only if the real owner's lease is
>>>>>>> not protected by Kerberos AND this client has the same
>>>>>>> client ID string as another client.
>>>>>>=20
>>>>>> Ok I will add this more explicit detail when the interference =
occurs
>>>>>> (when neither of the machines are using Kerberos and the other =
client
>>>>>> machine is not using a module parameter to add a unique =
identifier to
>>>>>> the client ID string). I think otherwise it is knowns that client =
ID
>>>>>> is created with the value of the clientaddr.
>>>>>=20
>>>>> The only way a problem occurs is if the clientaddr is the
>>>>> same AND the cl_nodename is the same. How is that happening?
>>>>=20
>>>> Client ID in the SETCLIENTID is constructed by
>>>> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr
>>>> and not cl_nodename.
>>>=20
>>> 5614         scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
>>> 5615                         clp->cl_ipaddr,
>>> 5616                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_ADDR),
>>> 5617                         rpc_peeraddr2str(clp->cl_rpcclient, =
RPC_DISPLAY_PROTO));
>>>=20
>>> So I get your point now: if two Linux NFSv4.0 clients mount the
>>> same server and specify the same callback address, and do not
>>> specify "migration", they will indeed present exactly the same
>>> client ID string to the server.
>>>=20
>>> I had remembered there was more in that string, but I guess I
>>> was thinking of the uniform client ID string.
>>>=20
>>> And it will break badly if several clients decide to use
>>> clientaddr=3D0.0.0.0. Trond, any thoughts? Why aren't the
>>> nodename or uniquifier available in this string?
>>=20
>> I can think of legitimate cases where two unique NFSv4.0
>> clients having the same IP address might mount the same
>> server.
>>=20
>> - clientaddr=3D0.0.0.0, as mentioned above
>=20
> I think this should be prohibited. If this is used a way to signify to
> the server to no give out delegations, then there could be other means
> of doing so. Let's add a mount option 'nodeleg', client would send a
> valid callback info to the server but if the option is set, then it
> will not start a callback server. That would prevent the server from
> being able to establish a callback path (which is the same thing as
> sending 0.0.0.0).

That introduces delays while the server is probing the client's
callback server. The spec specifically allows a client to send
0.0.0.0 to signify that the server should not use callbacks.

And mount.nfs has allowed that setting for a long time.

IMO we have to continue to allow 0.0.0.0.


>> - clients behind NATs using a private subnet that happen
>> to be numbered the same (192.168.0.77, say).
>=20
> Are we talking about a scenario where we have two private network
> which are numbered the same sitting behind a NAT-ed routers and both
> then accessing an NFS server on the internet? So how about then adding
> a MAC address?

If there are multiple ethernet devices on the client, how do
we guarantee the NFS client will choose the same MAC address
after every reboot?

What if the administrator changes the MAC or replaces the
network device between client reboots?

Of course a malicious admin can create a VM that has the
same MAC as some other client, or a hardware company can
re-use MAC addresses.


>> Restricting the addresses allowed as the clientaddr=3D
>> value does not address these cases.
>=20
>> We already have some workarounds:
>=20
> Since this is really a not a "bug" but an attack, I think having
> workarounds is not sufficient. I think whatever it is needs to be
> enforced.

These are all valid if someone came to us this afternoon with
an existing client and said "I can't deploy a code change for
a while. How do I close this hole today?"

I'm not proposing these as the fix (although some might be
inclined to opine that "stop using NFSv4.0" is a/the real fix).


>> - Use NFSv4.1 or later
>>=20
>> - Use NFSv4.0 with the "migration" option
>>=20
>> - Use Kerberos (give the clients and server service
>>  principals) and fix the server to reject SETCLIENTID
>>  using a recognized client ID string but an unrecognized
>>  authentication flavor and principal
>>=20
>> And possible fixes might include:
>>=20
>> - Improve the Linux client's non-UCS format
>=20
> How about adding a MAC address to the string.

See above.

Ideally we could have a tool that generated a random UUID
and added it to /etc/grub/defaults on the kernel command
line. There is already at least one UUID there on all my
systems.


>> - Make "migration" the default behavior
>=20
> That would mean we totally scrap the use of
> nfs4_init_nonuniform_string().

Not at all. Making "migration" the default means that where
needed, an administrator can choose to use nonuniform.
There are other reasons why making "migration" the default
is not a real option.


> Spec suggestions that client ip, server
> ip, + others to be used in creation of the client ID.

The spec _suggests_ the use of these things. It does not
require any of them. A full analysis of the issues with
the spec's recommended client ID string is discussed here:

https://datatracker.ietf.org/doc/rfc7931/


> A uniform client
> string does not include client ip + server ip (+transport). We can't
> assume existence of the unique identifier, so the only thing that'll
> be there is a cl_nodename. Can it be assumed to always be configured
> correctly? Can't this value frequently be just "localhost.localhost"?

Yes. As I said before several times, the only way to address
this issue securely is to use Kerberos. We are not going to
get this hole closed 100% without the use of cryptography.


> How about instead of having uniform and non-uniform have just a single
> function that will include all of it client ip, server ip, transport,
> unique identifier (if present), and cl_nodename (and add a mac)? Of
> course all if it will have to fit into 1024 opaque limit size.

Have a look at RFC 7931 to understand why we have uniform and
non-uniform client ID strings.

Uniform client ID string can't have anything in it that's going
to be different depending on which server it's connected to.
The string is the same no matter which server is in use, so
that the client looks the same to two arbitrary servers
participating in a migration event.


>> I am still in favor of validating the clientaddr value,
>> but again, IMO that just papers over the real problem,
>> which is the current non-UCS client ID string format.
>=20
> I think at the nfs-utils, we can still check that supplied value is
> one of the network addresses machine has (do not allow 0.0.0.0 or also
> probably not 127.0.0.1 (or ipv6 equivalent)). I'm not sure if mac
> should be queried and passed into the kernel or the kernel acquires
> it.

Yes, mount.nfs should do some basic validation.

Not putting the client's local address in the client ID string
would be beneficial. But I think using a MAC address has
similar weaknesses as using the client's local address.

My preference is to put a random UUID on the kernel boot
command line; but to do that means the code that constructs
the non-uniform client ID string has to be fixed up.


--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On May 29, 2018, at 4:53 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>=20
>> On May 29, 2018, at 4:07 PM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>=20
>> On Fri, May 25, 2018 at 6:35 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>>=20
>>> I can think of legitimate cases where two unique NFSv4.0
>>> clients having the same IP address might mount the same
>>> server.
>>>=20
>>> - clientaddr=3D0.0.0.0, as mentioned above
>>=20
>> I think this should be prohibited. If this is used a way to signify =
to
>> the server to no give out delegations, then there could be other =
means
>> of doing so. Let's add a mount option 'nodeleg', client would send a
>> valid callback info to the server but if the option is set, then it
>> will not start a callback server. That would prevent the server from
>> being able to establish a callback path (which is the same thing as
>> sending 0.0.0.0).
>=20
> That introduces delays while the server is probing the client's
> callback server. The spec specifically allows a client to send
> 0.0.0.0 to signify that the server should not use callbacks.
>=20
> And mount.nfs has allowed that setting for a long time.
>=20
> IMO we have to continue to allow 0.0.0.0.

Still thinking about this.

Not using cl_ipaddr in the client ID string helps. I have a patch
that does this.

The question I've been wondering since the original post is "are there
any other use cases where mount.nfs can't properly guess which address
to provide?" That's why I was asking why your customer was using
clientaddr.

If we have confidence that mount.nfs is 100% reliable about choosing
a good local address, except in the NAT case where we should just be
disabling CB, then we could have mount.nfs strip off a user-supplied
clientaddr and construct its own in all cases but the "clientaddr=3D
0.0.0.0" case (and it's IPv6 equivalent).


--
Chuck Lever

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

On Fri, Jun 1, 2018 at 5:42 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>> On May 29, 2018, at 4:53 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>> On May 29, 2018, at 4:07 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>>>
>>> On Fri, May 25, 2018 at 6:35 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> I can think of legitimate cases where two unique NFSv4.0
>>>> clients having the same IP address might mount the same
>>>> server.
>>>>
>>>> - clientaddr=0.0.0.0, as mentioned above
>>>
>>> I think this should be prohibited. If this is used a way to signify to
>>> the server to no give out delegations, then there could be other means
>>> of doing so. Let's add a mount option 'nodeleg', client would send a
>>> valid callback info to the server but if the option is set, then it
>>> will not start a callback server. That would prevent the server from
>>> being able to establish a callback path (which is the same thing as
>>> sending 0.0.0.0).
>>
>> That introduces delays while the server is probing the client's
>> callback server. The spec specifically allows a client to send
>> 0.0.0.0 to signify that the server should not use callbacks.
>>
>> And mount.nfs has allowed that setting for a long time.
>>
>> IMO we have to continue to allow 0.0.0.0.
>
> Still thinking about this.
>
> Not using cl_ipaddr in the client ID string helps. I have a patch
> that does this.
>
> The question I've been wondering since the original post is "are there
> any other use cases where mount.nfs can't properly guess which address
> to provide?" That's why I was asking why your customer was using
> clientaddr.
>
> If we have confidence that mount.nfs is 100% reliable about choosing
> a good local address, except in the NAT case where we should just be
> disabling CB, then we could have mount.nfs strip off a user-supplied
> clientaddr and construct its own in all cases but the "clientaddr=
> 0.0.0.0" case (and it's IPv6 equivalent).

When you were saying that we want to enable the client to specify any
address that's available to it, I was thinking you were considering a
case of a multihome machine where the client wants to have normal nfs
connection go over one network but have the callback able to go over
another interface. I'm almost certain that was not the setup that the
customer was using but I can ask. In general, I don't think we really
know how the customers are using this because we only hear about
issues and not when things are just working.

So that I'm clear about what you saying: are you proposing two
different approaches. First is to do away with cl_ipaddr in client ID.
Then nothing is changed with the mount.nfs. Or second, we keep
cl_ipaddr as is in client ID and then instead change mount.nfs to
always choose the callback address for the user and only allow for
clientaddr=0.0.0.0 user input case?

Let me start with the 2nd one, I'm ok with it as long as we clearly
document it in man pages. As is right now I'm disappointed that
nowhere is the clientaddr=0.0.0.0 is documented (or even clearly
talked about in the spec). However, if there is at all of value having
a callback being on a different network, then I think instead, my
original approach that does the checks would be a better way to do (i
have a draft patch for it).

If we are changing how the client Id is constructed and not using the
cl_ipaddr, then I would like to know what would be used. But as long
as you feel like such in my view a major change doesn't do anything
bad, then I'm ok with it as it addresses the issue of user input to
mount.nfs mistakenly or maliciously causing problems.

Re: [PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Chuck Lever]

> On Jun 2, 2018, at 9:37 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Fri, Jun 1, 2018 at 5:42 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>=20
>>=20
>>> On May 29, 2018, at 4:53 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>>=20
>>>> On May 29, 2018, at 4:07 PM, Olga Kornievskaia <aglo@umich.edu> =
wrote:
>>>>=20
>>>> On Fri, May 25, 2018 at 6:35 PM, Chuck Lever =
<chuck.lever@oracle.com> wrote:
>>>>>=20
>>>>> I can think of legitimate cases where two unique NFSv4.0
>>>>> clients having the same IP address might mount the same
>>>>> server.
>>>>>=20
>>>>> - clientaddr=3D0.0.0.0, as mentioned above
>>>>=20
>>>> I think this should be prohibited. If this is used a way to signify =
to
>>>> the server to no give out delegations, then there could be other =
means
>>>> of doing so. Let's add a mount option 'nodeleg', client would send =
a
>>>> valid callback info to the server but if the option is set, then it
>>>> will not start a callback server. That would prevent the server =
from
>>>> being able to establish a callback path (which is the same thing as
>>>> sending 0.0.0.0).
>>>=20
>>> That introduces delays while the server is probing the client's
>>> callback server. The spec specifically allows a client to send
>>> 0.0.0.0 to signify that the server should not use callbacks.
>>>=20
>>> And mount.nfs has allowed that setting for a long time.
>>>=20
>>> IMO we have to continue to allow 0.0.0.0.
>>=20
>> Still thinking about this.
>>=20
>> Not using cl_ipaddr in the client ID string helps. I have a patch
>> that does this.
>>=20
>> The question I've been wondering since the original post is "are =
there
>> any other use cases where mount.nfs can't properly guess which =
address
>> to provide?" That's why I was asking why your customer was using
>> clientaddr.
>>=20
>> If we have confidence that mount.nfs is 100% reliable about choosing
>> a good local address, except in the NAT case where we should just be
>> disabling CB, then we could have mount.nfs strip off a user-supplied
>> clientaddr and construct its own in all cases but the "clientaddr=3D
>> 0.0.0.0" case (and it's IPv6 equivalent).
>=20
> When you were saying that we want to enable the client to specify any
> address that's available to it, I was thinking you were considering a
> case of a multihome machine where the client wants to have normal nfs
> connection go over one network but have the callback able to go over
> another interface. I'm almost certain that was not the setup that the
> customer was using but I can ask. In general, I don't think we really
> know how the customers are using this because we only hear about
> issues and not when things are just working.
>=20
> So that I'm clear about what you saying: are you proposing two
> different approaches. First is to do away with cl_ipaddr in client ID.
> Then nothing is changed with the mount.nfs. Or second, we keep
> cl_ipaddr as is in client ID and then instead change mount.nfs to
> always choose the callback address for the user and only allow for
> clientaddr=3D0.0.0.0 user input case?
>=20
> Let me start with the 2nd one, I'm ok with it as long as we clearly
> document it in man pages. As is right now I'm disappointed that
> nowhere is the clientaddr=3D0.0.0.0 is documented (or even clearly
> talked about in the spec). However, if there is at all of value having
> a callback being on a different network, then I think instead, my
> original approach that does the checks would be a better way to do (i
> have a draft patch for it).
>=20
> If we are changing how the client Id is constructed and not using the
> cl_ipaddr, then I would like to know what would be used. But as long
> as you feel like such in my view a major change doesn't do anything
> bad, then I'm ok with it as it addresses the issue of user input to
> mount.nfs mistakenly or maliciously causing problems.

Some combination of both approaches is necessary. They are
complementary and can be applied together.

I believe we have to change the way the client ID string is
constructed. Even if mount.nfs is working perfectly, there are
use cases where the client's address is not a good addition to
the string; it can change over reboots, for example, which is
bad for a persistent client ID. My patch changes the non-UCS to
use the same raw components as the UCS (plus non-UCS still has
to include the server address).

I agree that we don't have a way of knowing how clientaddr=3D
can be used, and that's why mount.nfs has been left the way it
is for so long. I can't think of a reason clientaddr should
point to another machine. Thus restricting it to just local
addresses and ANY addresses seems like it would be OK to do.


--
Chuck Lever

[PATCH 1/1] nfs-utils: Add check of clientaddr argument

[Olga Kornievskaia]

If the user supplies a clientaddr value, it should be either
a special value of either IPV4/IPV6 any address or a local address
on the same network that the server being mounted. Otherwise, we
disallow the client to use an arbitrary value of the clientaddr value.
This value is used to construct a client id of SETCLIENTID and
providing a false value can interfere with the real owner's mount.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 utils/mount/stropts.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d1b0708..44a6ff5 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
 
 /*
  * Called to discover our address and append an appropriate 'clientaddr='
- * option to the options string.
+ * option to the options string. If the supplied 'clientaddr=' value does
+ * not match either IPV4/IPv6 any or a local address, then fail the mount.
  *
  * Returns 1 if 'clientaddr=' option created successfully or if
  * 'clientaddr=' option is already present; otherwise zero.
@@ -242,11 +243,26 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
 	struct sockaddr *my_addr = &address.sa;
 	socklen_t my_len = sizeof(address);
 
-	if (po_contains(options, "clientaddr") == PO_FOUND)
-		return 1;
-
 	nfs_callback_address(sap, salen, my_addr, &my_len);
 
+	if (po_contains(options, "clientaddr") == PO_FOUND) {
+		char *addr = po_get(options, "clientaddr");
+		char address[NI_MAXHOST];
+
+		if (!strcmp(addr, "0.0.0.0") || !strcmp(addr, "::"))
+			return 1;
+		if (!nfs_present_sockaddr(my_addr, my_len, address,
+						sizeof(address)))
+			goto out;
+
+		if (strcmp(addr, address)) {
+			nfs_error(_("%s: failed to validate clientaddr "
+					"address"), progname);
+			return 0;
+		}
+		return 1;
+	}
+out:
 	return nfs_append_generic_address_option(my_addr, my_len,
 							"clientaddr", options);
 }
-- 
1.8.3.1