Add check of clientaddr argument

If the user supplies a clientaddr value, it should be either a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Signed-off-by Olga Kornievskaia <[email protected]>

---
 utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 utils/mount/network.h |  2 ++
 utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
 3 files changed, 72 insertions(+), 3 deletions(-)

utils/mount/network.c +45 -0

index e490399..2a54f82 100644
@@ -50,6 +50,8 @@
 #include <rpc/rpc.h>
 #include <rpc/pmap_prot.h>
 #include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>
 
 #include "sockaddr.h"
 #include "xcommon.h"

@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
 
 	return EX_SUCCESS;
 }
+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+	switch (nfs_saddr->sa_family) {
+	case AF_INET: {
+		if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
+				INADDR_ANY)
+			return 1;
+	}
+	case AF_INET6:
+		if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
+				&in6addr_any, sizeof(in6addr_any)))
+			return 1;
+	}
+	return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+	struct ifaddrs *myaddrs, *ifa;
+	int found = 0;
+
+	/* acquire exiting network interfaces */
+	if (getifaddrs(&myaddrs) != 0)
+		return 0;
+
+	/* interate over the available interfaces and check if we
+	 * we find a match to the supplied clientaddr value
+	 */
+	for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+		if (ifa->ifa_addr == NULL)
+			continue;
+		if (!(ifa->ifa_flags & IFF_UP))
+			continue;
+		if (!memcmp(ifa->ifa_addr, nfs_saddr,
+				sizeof(struct sockaddr))) {
+			found = 1;
+			break;
+		}
+	}
+	freeifaddrs(myaddrs);
+	return found;
+}

utils/mount/network.h +2 -0

index ecaac33..0fc98ac 100644
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
 int clnt_ping(struct sockaddr_in *, const unsigned long,
 		const unsigned long, const unsigned int,
 		struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);
 
 struct mount_options;
 

utils/mount/stropts.c +25 -3

index d1b0708..e22216f 100644
@@ -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,8 +243,29 @@ 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;
+	if (po_contains(options, "clientaddr") == PO_FOUND) {
+		char *addr = po_get(options, "clientaddr");
+		union nfs_sockaddr nfs_address;
+		struct sockaddr *nfs_saddr = &nfs_address.sa;
+		socklen_t nfs_salen = sizeof(nfs_address);
+
+		/* translate the input for clientaddr to nfs_sockaddr */
+		if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
+			return 0;
+
+		/* check for IPV4_ANY and IPV6_ANY */
+		if (nfs_is_inaddr_any(nfs_saddr))
+			return 1;
+
+		/* check if ip matches local network addresses */
+		if (!nfs_addr_matches_localips(nfs_saddr)) {
+			nfs_error(_("%s: supplied clientaddr=%s does not "
+				"match any existing network addresses"),
+				progname, addr);
+			return 0;
+		} else
+			return 1;
+	}
 
 	nfs_callback_address(sap, salen, my_addr, &my_len);
 

Quoted 5 lines · Olga Kornievskaia

If the user supplies a clientaddr value, it should be either a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Is this finally going to let one bind to a local IP address?

I have patches to do this for years now, would love to get that feature upstream!

Thanks,
Ben

Quoted 124 lines

Signed-off-by Olga Kornievskaia <[email protected]>

---
 utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 utils/mount/network.h |  2 ++
 utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
 3 files changed, 72 insertions(+), 3 deletions(-)

utils/mount/network.c +13 -0

index e490399..2a54f82 100644
@@ -50,6 +50,8 @@
 #include <rpc/rpc.h>
 #include <rpc/pmap_prot.h>
 #include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>
 
 #include "sockaddr.h"
 #include "xcommon.h"

@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options
 *options,
 
        return EX_SUCCESS;
 }
+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+       switch (nfs_saddr->sa_family) {
+       case AF_INET: {
+               if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
+                               INADDR_ANY)
+                       return 1;
+       }
+       case AF_INET6:
+               if (!memcmp(&((struct sockaddr_in6

*)nfs_saddr)->sin6_addr,

utils/mount/network.c +32 -0

+                               &in6addr_any, sizeof(in6addr_any)))
+                       return 1;
+       }
+       return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+       struct ifaddrs *myaddrs, *ifa;
+       int found = 0;
+
+       /* acquire exiting network interfaces */
+       if (getifaddrs(&myaddrs) != 0)
+               return 0;
+
+       /* interate over the available interfaces and check if we
+        * we find a match to the supplied clientaddr value
+        */
+       for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+               if (ifa->ifa_addr == NULL)
+                       continue;
+               if (!(ifa->ifa_flags & IFF_UP))
+                       continue;
+               if (!memcmp(ifa->ifa_addr, nfs_saddr,
+                               sizeof(struct sockaddr))) {
+                       found = 1;
+                       break;
+               }
+       }
+       freeifaddrs(myaddrs);
+       return found;
+}

utils/mount/network.h +2 -0

index ecaac33..0fc98ac 100644
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const
 socklen_t,
 int clnt_ping(struct sockaddr_in *, const unsigned long,
                const unsigned long, const unsigned int,
                struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);
 
 struct mount_options;

utils/mount/stropts.c +2 -1

index d1b0708..e22216f 100644
@@ -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.

utils/mount/stropts.c +15 -2

@@ -242,8 +243,29 @@ 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;
+       if (po_contains(options, "clientaddr") == PO_FOUND) {
+               char *addr = po_get(options, "clientaddr");
+               union nfs_sockaddr nfs_address;
+               struct sockaddr *nfs_saddr = &nfs_address.sa;
+               socklen_t nfs_salen = sizeof(nfs_address);
+
+               /* translate the input for clientaddr to nfs_sockaddr */
+               if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
+                       return 0;
+
+               /* check for IPV4_ANY and IPV6_ANY */
+               if (nfs_is_inaddr_any(nfs_saddr))
+                       return 1;
+
+               /* check if ip matches local network addresses */

utils/mount/stropts.c static int nfs_append_clientaddr_option(const struct · line 271

Quoted 10 lines

utils/mount/stropts.c +8 -0

+               if (!nfs_addr_matches_localips(nfs_saddr)) {
+                       nfs_error(_("%s: supplied clientaddr=%s does not "
+                               "match any existing network addresses"),
+                               progname, addr);
+                       return 0;
+               } else
+                       return 1;
+       }
 
        nfs_callback_address(sap, salen, my_addr, &my_len);

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

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

Quoted 9 lines · Ben Greear

Quoted 5 lines · Olga Kornievskaia

If the user supplies a clientaddr value, it should be either a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Is this finally going to let one bind to a local IP address?

No this patch only checks for the validity of the arguments for the callback channel.

Quoted 2 lines

I have patches to do this for years now, would love to get that feature upstream!

Have you posted those patches to the mailing list?

On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> =

=20
If the user supplies a clientaddr value,

it should be either
a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Signed-off-by Olga Kornievskaia <[email protected]>

---
utils/mount/network.c | 45 =

utils/mount/network.c +22 -0

+				&in6addr_any, sizeof(in6addr_any)))
+			return 1;
+	}
+	return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+	struct ifaddrs *myaddrs, *ifa;
+	int found =3D 0;
+
+	/* acquire exiting network interfaces */
+	if (getifaddrs(&myaddrs) !=3D 0)
+		return 0;
+
+	/* interate over the available interfaces and check if we
+	 * we find a match to the supplied clientaddr value
+	 */
+	for (ifa =3D myaddrs; ifa !=3D NULL; ifa =3D ifa->ifa_next) {
+		if (ifa->ifa_addr =3D=3D NULL)
+			continue;
+		if (!(ifa->ifa_flags & IFF_UP))

=20
/*
* Called to discover our address and append an appropriate =

- * option to the options string.
+ * option to the options string. If the supplied 'clientaddr=3D' =

+ * not match either IPV4/IPv6 any or a local address, then fail the =

utils/mount/stropts.c +6 -0

+				"match any existing network addresses"),
+				progname, addr);
+			return 0;
+		} else
+			return 1;
+	}

=20

	nfs_callback_address(sap, salen, my_addr, &my_len);

=20
--=20
1.8.3.1
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Olga Kornievskaia

If the user supplies a clientaddr value,

Suggest instead:

"If an NFS client administrator supplies the clientaddr mount option,"

Quoted 62 lines

it should be either
a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Signed-off-by Olga Kornievskaia <[email protected]>

---

utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
utils/mount/network.h |  2 ++
utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
3 files changed, 72 insertions(+), 3 deletions(-)

utils/mount/network.c +35 -0

index e490399..2a54f82 100644
@@ -50,6 +50,8 @@
 #include <rpc/rpc.h>
 #include <rpc/pmap_prot.h>
 #include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>
 
 #include "sockaddr.h"
 #include "xcommon.h"

@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
 
      return EX_SUCCESS;
 }
+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+     switch (nfs_saddr->sa_family) {
+     case AF_INET: {
+             if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
+                             INADDR_ANY)
+                     return 1;
+     }
+     case AF_INET6:
+             if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
+                             &in6addr_any, sizeof(in6addr_any)))
+                     return 1;
+     }
+     return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+     struct ifaddrs *myaddrs, *ifa;
+     int found = 0;
+
+     /* acquire exiting network interfaces */
+     if (getifaddrs(&myaddrs) != 0)
+             return 0;
+
+     /* interate over the available interfaces and check if we
+      * we find a match to the supplied clientaddr value
+      */
+     for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+             if (ifa->ifa_addr == NULL)
+                     continue;
+             if (!(ifa->ifa_flags & IFF_UP))

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options *options, · line 1806

Quoted 10 lines

utils/mount/network.c +10 -0

+                     continue;
+             if (!memcmp(ifa->ifa_addr, nfs_saddr,
+                             sizeof(struct sockaddr))) {
+                     found = 1;
+                     break;
+             }
+     }
+     freeifaddrs(myaddrs);
+     return found;
+}

This looks like what we discussed, and it is implemented IMO correctly. Can you also provide an update to nfs(5)?

After all the dialog and thinking more about it, I'm less convinced that prohibiting non-local addresses is wise. The client can mount through a NAT and specify a clientaddr that points to the NAT. That seems like a correct configuration that would now be prohibited.

However I don't know of any actual deployments like this, so it's hard to argue convincingly that mount.nfs needs to continue to allow it.

May I suggest a slight alteration?

- If the value of clientaddr= is not a presentation address, report the problem and fail the mount (no change)

- Allow the value of clientaddr= to be an ANY address without complaint (no change)

- If the value of clientaddr= is not a local address, warn but allow it (rather than failing)

That permits flexibility but also gives a good indication to administrators who have made an honest mistake.

Quoted 63 lines

utils/mount/network.h +2 -0

index ecaac33..0fc98ac 100644
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
 int clnt_ping(struct sockaddr_in *, const unsigned long,
              const unsigned long, const unsigned int,
              struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);
 
 struct mount_options;
 

utils/mount/stropts.c +25 -4

index d1b0708..e22216f 100644
@@ -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,8 +243,29 @@ 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;
+     if (po_contains(options, "clientaddr") == PO_FOUND) {
+             char *addr = po_get(options, "clientaddr");
+             union nfs_sockaddr nfs_address;
+             struct sockaddr *nfs_saddr = &nfs_address.sa;
+             socklen_t nfs_salen = sizeof(nfs_address);
+
+             /* translate the input for clientaddr to nfs_sockaddr */
+             if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
+                     return 0;
+
+             /* check for IPV4_ANY and IPV6_ANY */
+             if (nfs_is_inaddr_any(nfs_saddr))
+                     return 1;
+
+             /* check if ip matches local network addresses */
+             if (!nfs_addr_matches_localips(nfs_saddr)) {
+                     nfs_error(_("%s: supplied clientaddr=%s does not "
+                             "match any existing network addresses"),
+                             progname, addr);
+                     return 0;
+             } else
+                     return 1;
+     }
 
      nfs_callback_address(sap, salen, my_addr, &my_len);
 
--

1.8.3.1

Quoted 4 lines

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

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

=20
On Tue, Jun 5, 2018 at 12:49 PM, Chuck Lever <[email protected]> =

Quoted 3 lines

=20
=20

Quoted 1 line

On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> =

Quoted 6 lines

Quoted 2 lines

=20
If the user supplies a clientaddr value,

=20
Suggest instead:
=20
"If an NFS client administrator supplies the clientaddr mount =

=20
Ok.
=20

Quoted 8 lines

Quoted 4 lines

it should be either
a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

=20

Quoted 3 lines

Signed-off-by Olga Kornievskaia <[email protected]>

---
utils/mount/network.c | 45 =

Quoted 10 lines

Quoted 10 lines

utils/mount/network.h | 2 ++ utils/mount/stropts.c | 28 +++++++++++++++++++++++++---

3 files changed, 72 insertions(+), 3 deletions(-)

=20

utils/mount/network.c +0 -0

index e490399..2a54f82 100644
@@ -50,6 +50,8 @@
 #include <rpc/rpc.h>

Quoted 8 lines

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options =

Quoted 8 lines

utils/mount/network.c +2 -0

 #include <rpc/pmap_prot.h>
 #include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>

=20
#include "sockaddr.h"
#include "xcommon.h"

utils/mount/network.c +0 -0

@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options =

Quoted 9 lines

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options *options,

Quoted 9 lines

=20

     return EX_SUCCESS;
}

utils/mount/network.c +6 -0

+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+     switch (nfs_saddr->sa_family) {
+     case AF_INET: {
+             if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr =

Quoted 5 lines

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options *options,

Quoted 5 lines

utils/mount/network.c +5 -0

+                             INADDR_ANY)
+                     return 1;
+     }
+     case AF_INET6:
+             if (!memcmp(&((struct sockaddr_in6 =

Quoted 27 lines

Quoted 27 lines

utils/mount/network.c +27 -0

+                             &in6addr_any, sizeof(in6addr_any)))
+                     return 1;
+     }
+     return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+     struct ifaddrs *myaddrs, *ifa;
+     int found =3D 0;
+
+     /* acquire exiting network interfaces */
+     if (getifaddrs(&myaddrs) !=3D 0)
+             return 0;
+
+     /* interate over the available interfaces and check if we
+      * we find a match to the supplied clientaddr value
+      */
+     for (ifa =3D myaddrs; ifa !=3D NULL; ifa =3D ifa->ifa_next) {
+             if (ifa->ifa_addr =3D=3D NULL)
+                     continue;
+             if (!(ifa->ifa_flags & IFF_UP))
+                     continue;
+             if (!memcmp(ifa->ifa_addr, nfs_saddr,
+                             sizeof(struct sockaddr))) {
+                     found =3D 1;
+                     break;

Quoted 8 lines

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options *options, · line 1806

Quoted 5 lines

utils/mount/network.c +5 -0

+             }
+     }
+     freeifaddrs(myaddrs);
+     return found;
+}

=20
This looks like what we discussed, and it is implemented IMO correctly.

Quoted 16 lines

Can you also provide an update to nfs(5)?

After all the dialog and thinking more about it, I'm less convinced that prohibiting non-local addresses is wise. The client can mount through a NAT and specify a clientaddr that points to the NAT. That seems like a correct configuration that would now be prohibited. =20
However I don't know of any actual deployments like this, so it's hard to argue convincingly that mount.nfs needs to continue to allow it.
=20
May I suggest a slight alteration?
=20
- If the value of clientaddr=3D is not a presentation address, report the problem and fail the mount (no change)
=20

Quoted 8 lines

- Allow the value of clientaddr=3D to be an ANY address without complaint (no change)
=20
- If the value of clientaddr=3D is not a local address, warn but allow it (rather than failing)
=20
That permits flexibility but also gives a good indication to administrators who have made an honest mistake.

=20
Given that we are changing the kernel to not use the cl_ipaddr to construct the client ID, then I'm ok with allowing a none local address with a warning. But can you tell me how you think specifying a non local address would actually work? It doesn't make sense to me as to how the callback would ever reach the appropriate client. When the server establishes connected back to the NAT router IP, there is nothing that's listening there. And also NAT router doesn't know where to send it to.

If we are allowing but warning about the use of the arbitrary IP, do we need any change to the nfs(5)?

Quoted 5 lines

utils/mount/network.h int nfs_callback_address(const struct sockaddr *, =

Quoted 5 lines

utils/mount/network.h +0 -0

index ecaac33..0fc98ac 100644
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, =

Quoted 5 lines

Quoted 5 lines

utils/mount/network.h +2 -0

 int clnt_ping(struct sockaddr_in *, const unsigned long,
             const unsigned long, const unsigned int,
             struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);

Quoted 8 lines

utils/mount/stropts.c static int nfs_append_addr_option(const struct =

Quoted 8 lines

=20
struct mount_options;
=20

utils/mount/stropts.c +0 -0

index d1b0708..e22216f 100644
@@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct =

Quoted 3 lines

Quoted 3 lines

=20
/*
* Called to discover our address and append an appropriate =

Quoted 2 lines

Quoted 2 lines

- * option to the options string.
+ * option to the options string. If the supplied 'clientaddr=3D' =

Quoted 1 line

Quoted 1 line

+ * not match either IPV4/IPv6 any or a local address, then fail the =

Quoted 4 lines

utils/mount/stropts.c static int nfs_append_clientaddr_option(const =

Quoted 4 lines

*
* Returns 1 if 'clientaddr=3D' option created successfully or if
* 'clientaddr=3D' option is already present; otherwise zero.

utils/mount/stropts.c +0 -0

@@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const =

Quoted 4 lines

Quoted 4 lines

utils/mount/stropts.c +0 -0

     struct sockaddr *my_addr =3D &address.sa;
     socklen_t my_len =3D sizeof(address);

=20
- if (po_contains(options, "clientaddr") =3D=3D PO_FOUND)

Quoted 8 lines

utils/mount/stropts.c

Quoted 8 lines

utils/mount/stropts.c +7 -1

-             return 1;
+     if (po_contains(options, "clientaddr") =3D=3D PO_FOUND) {
+             char *addr =3D po_get(options, "clientaddr");
+             union nfs_sockaddr nfs_address;
+             struct sockaddr *nfs_saddr =3D &nfs_address.sa;
+             socklen_t nfs_salen =3D sizeof(nfs_address);
+
+             /* translate the input for clientaddr to nfs_sockaddr =

Quoted 1 line

Quoted 1 line

+ if (!nfs_string_to_sockaddr(addr, nfs_saddr, =

Quoted 9 lines

utils/mount/stropts.c static int nfs_append_clientaddr_option(const struct sockaddr *sap,

Quoted 9 lines

utils/mount/stropts.c +9 -0

+                     return 0;
+
+             /* check for IPV4_ANY and IPV6_ANY */
+             if (nfs_is_inaddr_any(nfs_saddr))
+                     return 1;
+
+             /* check if ip matches local network addresses */
+             if (!nfs_addr_matches_localips(nfs_saddr)) {
+                     nfs_error(_("%s: supplied clientaddr=3D%s does =

Quoted 1 line

Quoted 1 line

+ "match any existing network =

Quoted 5 lines

Quoted 5 lines

utils/mount/stropts.c +5 -0

+                             progname, addr);
+                     return 0;
+             } else
+                     return 1;
+     }

Quoted 8 lines

Quoted 8 lines

=20

     nfs_callback_address(sap, salen, my_addr, &my_len);

=20
--
1.8.3.1
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

Quoted 10 lines

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

=20
--
Chuck Lever
=20
=20
=20
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Olga Kornievskaia

Quoted 4 lines · Chuck Lever

Quoted 4 lines · Olga Kornievskaia

Quoted 83 lines

Quoted 5 lines

Quoted 1 line

If the user supplies a clientaddr value,

Suggest instead:

"If an NFS client administrator supplies the clientaddr mount option,"

Ok.

Quoted 75 lines

Quoted 62 lines

it should be either
a special value of either IPv4/IPv6 any address or one of the machine's network addresses. Otherwise, the use of an arbitrary value of the clientaddr value is not allowed.

Signed-off-by Olga Kornievskaia <[email protected]>

---

utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
utils/mount/network.h |  2 ++
utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
3 files changed, 72 insertions(+), 3 deletions(-)

utils/mount/network.c +35 -0

index e490399..2a54f82 100644
@@ -50,6 +50,8 @@
 #include <rpc/rpc.h>
 #include <rpc/pmap_prot.h>
 #include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>
 
 #include "sockaddr.h"
 #include "xcommon.h"

@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
 
     return EX_SUCCESS;
 }
+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+     switch (nfs_saddr->sa_family) {
+     case AF_INET: {
+             if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
+                             INADDR_ANY)
+                     return 1;
+     }
+     case AF_INET6:
+             if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
+                             &in6addr_any, sizeof(in6addr_any)))
+                     return 1;
+     }
+     return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+     struct ifaddrs *myaddrs, *ifa;
+     int found = 0;
+
+     /* acquire exiting network interfaces */
+     if (getifaddrs(&myaddrs) != 0)
+             return 0;
+
+     /* interate over the available interfaces and check if we
+      * we find a match to the supplied clientaddr value
+      */
+     for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+             if (ifa->ifa_addr == NULL)
+                     continue;
+             if (!(ifa->ifa_flags & IFF_UP))

utils/mount/network.c int nfs_umount_do_umnt(struct mount_options *options, · line 1806

Quoted 10 lines

utils/mount/network.c +10 -0

+                     continue;
+             if (!memcmp(ifa->ifa_addr, nfs_saddr,
+                             sizeof(struct sockaddr))) {
+                     found = 1;
+                     break;
+             }
+     }
+     freeifaddrs(myaddrs);
+     return found;
+}

This looks like what we discussed, and it is implemented IMO correctly.

While I was watching TV and eating lunch, it occurred to me that mount.nfs could pass the clientaddr value to bind(2). bind(2) should work for an ANY address or a local address, but would fail with a non-local address, unless I'm confused. Just a random thought.

Quoted 25 lines

Quoted 16 lines

Can you also provide an update to nfs(5)?

After all the dialog and thinking more about it, I'm less convinced that prohibiting non-local addresses is wise. The client can mount through a NAT and specify a clientaddr that points to the NAT. That seems like a correct configuration that would now be prohibited.

However I don't know of any actual deployments like this, so it's hard to argue convincingly that mount.nfs needs to continue to allow it.

May I suggest a slight alteration?

- If the value of clientaddr= is not a presentation address, report the problem and fail the mount (no change)

Quoted 8 lines

- Allow the value of clientaddr= to be an ANY address without complaint (no change)

- If the value of clientaddr= is not a local address, warn but allow it (rather than failing)

That permits flexibility but also gives a good indication to administrators who have made an honest mistake.

Quoted 8 lines

Given that we are changing the kernel to not use the cl_ipaddr to construct the client ID, then I'm ok with allowing a none local address with a warning. But can you tell me how you think specifying a non local address would actually work? It doesn't make sense to me as to how the callback would ever reach the appropriate client. When the server establishes connected back to the NAT router IP, there is nothing that's listening there. And also NAT router doesn't know where to send it to.

How it might work:

- Each client would have it's own callback service running on a distinct port

- The NAT would need to be programmed to route the callback connection to the correct client, by port

Quoted 2 lines

If we are allowing but warning about the use of the arbitrary IP, do we need any change to the nfs(5)?

Hm...

You mentioned nfs(5) doesn't document the clientaddr=ANY trick.

If we want to leave that undocumented (say, to try to implement a more pleasant administrative interface to disable NFSv4.0 callback) then I suppose we could leave nfs(5) alone.

Quoted 72 lines

Quoted 72 lines

Quoted 62 lines

utils/mount/network.h +2 -0

index ecaac33..0fc98ac 100644
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
 int clnt_ping(struct sockaddr_in *, const unsigned long,
             const unsigned long, const unsigned int,
             struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);
 
 struct mount_options;
 

utils/mount/stropts.c +25 -4

index d1b0708..e22216f 100644
@@ -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,8 +243,29 @@ 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;
+     if (po_contains(options, "clientaddr") == PO_FOUND) {
+             char *addr = po_get(options, "clientaddr");
+             union nfs_sockaddr nfs_address;
+             struct sockaddr *nfs_saddr = &nfs_address.sa;
+             socklen_t nfs_salen = sizeof(nfs_address);
+
+             /* translate the input for clientaddr to nfs_sockaddr */
+             if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
+                     return 0;
+
+             /* check for IPV4_ANY and IPV6_ANY */
+             if (nfs_is_inaddr_any(nfs_saddr))
+                     return 1;
+
+             /* check if ip matches local network addresses */
+             if (!nfs_addr_matches_localips(nfs_saddr)) {
+                     nfs_error(_("%s: supplied clientaddr=%s does not "
+                             "match any existing network addresses"),
+                             progname, addr);
+                     return 0;
+             } else
+                     return 1;
+     }
 
     nfs_callback_address(sap, salen, my_addr, &my_len);
 
--

1.8.3.1

Quoted 4 lines

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

Quoted 8 lines

Quoted 4 lines

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

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