[PATCH RFC 0/3] Avoid well-known port numbers

[PATCH RFC 0/3] Avoid well-known port numbers

[Chuck Lever]

Following up on https://bugzilla.linux-nfs.org/show_bug.cgi?id=320 .

Here's a possible way to get libtirpc to avoid well-known port
numbers when a caller requests a dynamically-assigned port. This
compiles without complaint, but I haven't tested it yet. I'm
interested in comments on the approach.

I understand that Fedora 28 is closing soon. It would be nice to
see a fix for this issue in libtirpc for that release.

---

Chuck Lever (3):
      Add an internal helper for binding to a dynamically-assigned port
      Avoid choosing reserved ports in svc_tli_create(3)
      Avoid choosing reserved ports in clnt_tli_create(3)


 src/Makefile.am    |    5 +-
 src/binddynport.c  |  132 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/clnt_generic.c |    4 +-
 src/rpc_soc.c      |    7 ++-
 src/svc_generic.c  |   12 ++---
 5 files changed, 147 insertions(+), 13 deletions(-)
 create mode 100644 src/binddynport.c

--
Chuck Lever

[PATCH RFC 1/3] Add an internal helper for binding to a dynamically-assigned port

[Chuck Lever]

Create a helper function akin to bindresvport(3) that instead binds
to a dynamically assigned port. It uses the rules in RFC 6335
Section 6 to avoid all IANA-assigned service port numbers, even
when the caller has the CAP_NET_ADMIN_BIND privilege.

This is intended to remain an internal helper for the time being, so
this commit provides no header declaration.

All internal bindresvport(3) call sites manufacture an INADDR_ANY-
type address to pass to bind(2), so the helper handles that as well,
to avoid code duplication. This means that callers do not need to
pass in a sockaddr. Only an open socket is required.

BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=320
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/Makefile.am   |    5 +-
 src/binddynport.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 2 deletions(-)
 create mode 100644 src/binddynport.c

diff --git a/src/Makefile.am b/src/Makefile.am
index fba2aa4..932414d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -15,8 +15,9 @@ lib_LTLIBRARIES = libtirpc.la
 libtirpc_la_LDFLAGS = @LDFLAG_NOUNDEFINED@ -no-undefined -lpthread
 libtirpc_la_LDFLAGS += -version-info @LT_VERSION_INFO@
 
-libtirpc_la_SOURCES = auth_none.c auth_unix.c authunix_prot.c bindresvport.c clnt_bcast.c \
-        clnt_dg.c clnt_generic.c clnt_perror.c clnt_raw.c clnt_simple.c \
+libtirpc_la_SOURCES = auth_none.c auth_unix.c authunix_prot.c \
+        binddynport.c bindresvport.c \
+        clnt_bcast.c clnt_dg.c clnt_generic.c clnt_perror.c clnt_raw.c clnt_simple.c \
         clnt_vc.c rpc_dtablesize.c getnetconfig.c getnetpath.c getrpcent.c \
         getrpcport.c mt_misc.c pmap_clnt.c pmap_getmaps.c pmap_getport.c \
         pmap_prot.c pmap_prot2.c pmap_rmt.c rpc_prot.c rpc_commondata.c \
diff --git a/src/binddynport.c b/src/binddynport.c
new file mode 100644
index 0000000..1580117
--- /dev/null
+++ b/src/binddynport.c
@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2018, Oracle America, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * - Redistributions of source code must retain the above copyright notice,
+ *   this list of conditions and the following disclaimer.
+ * - Redistributions in binary form must reproduce the above copyright notice,
+ *   this list of conditions and the following disclaimer in the documentation
+ *   and/or other materials provided with the distribution.
+ * - Neither the name of "Oracle America, Inc." nor the names of its
+ *   contributors may be used to endorse or promote products derived
+ *   from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <netdb.h>
+#include <netinet/in.h>
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+
+#include <rpc/rpc.h>
+
+#include "reentrant.h"
+#include "rpc_com.h"
+
+extern pthread_mutex_t port_lock;
+
+/*
+ * Dynamic port range as defined in RFC 6335 Section 6.
+ * This range avoids all IANA-assigned service port
+ * numbers.
+ */
+enum {
+	LOWPORT		= 49152,
+	ENDPORT		= 65534,
+	NPORTS		= ENDPORT - LOWPORT + 1,
+};
+
+/*
+ * Bind a socket to a dynamically-assigned IP port.
+ *
+ * @fd is an open but unbound socket.
+ *
+ * On each call, a port number is chosen at random from
+ * within the dynamic/private port range, even if the
+ * caller has CAP_NET_ADMIN_BIND.
+ *
+ * Returns 0 on success, -1 on failure. errno may be
+ * set to a non-determinant value.
+ *
+ * This function is re-entrant.
+ */
+int __binddynport(int fd)
+{
+	struct sockaddr_storage ss;
+#ifdef INET6
+	struct sockaddr_in6 *sin6;
+#endif
+	struct sockaddr_in *sin;
+	in_port_t port, *portp;
+	struct sockaddr *sap;
+	socklen_t salen;
+	unsigned int seed;
+	int i, res;
+
+	if (__rpc_sockisbound(fd))
+		return 0;
+
+	res = -1;
+	sap = (struct sockaddr *)(void *)&ss;
+	memset(sap, 0, sizeof(ss));
+
+	mutex_lock(&port_lock);
+
+	if (getsockname(fd, sap, &salen) == -1)
+		goto out;
+
+	switch (ss.ss_family) {
+	case AF_INET:
+		sin = (struct sockaddr_in *)(void *)&ss;
+		portp = &sin->sin_port;
+		salen = sizeof(struct sockaddr_in);
+		break;
+#ifdef INET6
+	case AF_INET6:
+		sin6 = (struct sockaddr_in6 *)(void *)&ss;
+		portp = &sin6->sin6_port;
+		salen = sizeof(struct sockaddr_in6);
+		break;
+#endif
+	default:
+		goto out;
+	}
+
+	seed = time(NULL);
+	port = (rand_r(&seed) % NPORTS) + LOWPORT;
+	for (i = 0; i < NPORTS; ++i) {
+		*portp = htons(port++);
+		res = bind(fd, sap, salen);
+		if (res >= 0) {
+			res = 0;
+			break;
+		}
+		if (errno != EADDRINUSE)
+			break;
+		if (port > ENDPORT)
+			port = LOWPORT;
+	}
+
+out:
+	mutex_unlock(&port_lock);
+	return res;
+}

[PATCH RFC 2/3] Avoid choosing reserved ports in svc_tli_create(3)

[Chuck Lever]

Callers of svc_tli_create(3) can specify that an arbitrary port
number be dynamically assigned for the service listener being
created. svc_tli_create(3) tries bindresvport(3) first in this
case. bindresvport(3) chooses a reserved port if the caller has
CAP_NET_ADMIN_BIND privilege. If this fails, bind(2) is used to
assign a port number from the range above 1024.

This approach becomes a problem whenever bindresvport(3) happens to
choose the port number of a well-known service. If the caller is a
long-running service (like rpc.statd), it indefinitely prevents the
IANA-assigned well-known service from starting.

It seems that a reserved port is completely unnecessary for listener
sockets. It does not confer any extra privilege or functionality to
the listener socket, nor do remote clients infer any extra privilege
from a listener on a port number lower than 1024.

Therefore, remove the bindresvport step to prevent svc_tli_create(3)
from choosing a port number that interferes with well-known services
assigned to a privileged port.

svc_tli_create(3) will now never assign a privileged port
dynamically. If needed, a caller may still bind to a dynamically-
assigned reserved port by invoking bindresvport(3) directly and
passing the already-bound file descriptor to svc_tli_create(3).

But that should be a special case. It is no longer the default
behavior.

BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=320
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/rpc_soc.c     |    7 +++++--
 src/svc_generic.c |   12 ++++--------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/rpc_soc.c b/src/rpc_soc.c
index ed0892a..5ec96a7 100644
--- a/src/rpc_soc.c
+++ b/src/rpc_soc.c
@@ -67,6 +67,8 @@
 
 extern mutex_t	rpcsoc_lock;
 
+extern int __binddynport(int fd);
+
 static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
     int *, u_int, u_int, char *, int);
 static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
@@ -145,7 +147,8 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
 	bindaddr.maxlen = bindaddr.len =  sizeof (struct sockaddr_in);
 	bindaddr.buf = raddr;
 
-	bindresvport(fd, NULL);
+	if (__binddynport(fd) == -1)
+		goto err;
 	cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
 				sendsz, recvsz);
 	if (cl) {
@@ -332,7 +335,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
 
 	memset(&sin, 0, sizeof sin);
 	sin.sin_family = AF_INET;
-	bindresvport(fd, &sin);
+	bind(fd, (struct sockaddr *)(void *)&sin, sizeof sin);
 	listen(fd, SOMAXCONN);
 	svc = svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
 	(void) freenetconfigent(nconf);
diff --git a/src/svc_generic.c b/src/svc_generic.c
index 7aae796..52a56c2 100644
--- a/src/svc_generic.c
+++ b/src/svc_generic.c
@@ -53,6 +53,7 @@
 #include <rpc/svc.h>
 
 extern int __svc_vc_setflag(SVCXPRT *, int);
+extern int __binddynport(int fd);
 
 /*
  * The highest level interface for server creation.
@@ -220,15 +221,10 @@ svc_tli_create(fd, nconf, bindaddr, sendsz, recvsz)
 	 */
 	if (madefd || !__rpc_sockisbound(fd)) {
 		if (bindaddr == NULL) {
-			if (bindresvport(fd, NULL) < 0) {
-				memset(&ss, 0, sizeof ss);
-				ss.ss_family = si.si_af;
-				if (bind(fd, (struct sockaddr *)(void *)&ss,
-				    (socklen_t)si.si_alen) < 0) {
-					warnx(
+			if (__binddynport(fd) == -1) {
+				warnx(
 			"svc_tli_create: could not bind to anonymous port");
-					goto freedata;
-				}
+				goto freedata;
 			}
 			listen(fd, SOMAXCONN);
 		} else {

Re: [PATCH RFC 2/3] Avoid choosing reserved ports in svc_tli_create(3)

[Chuck Lever]

> On Feb 15, 2018, at 3:22 PM, Chuck Lever <chuck.lever@oracle.com> =
wrote:
>=20
> Callers of svc_tli_create(3) can specify that an arbitrary port
> number be dynamically assigned for the service listener being
> created. svc_tli_create(3) tries bindresvport(3) first in this
> case. bindresvport(3) chooses a reserved port if the caller has
> CAP_NET_ADMIN_BIND privilege. If this fails, bind(2) is used to
> assign a port number from the range above 1024.
>=20
> This approach becomes a problem whenever bindresvport(3) happens to
> choose the port number of a well-known service. If the caller is a
> long-running service (like rpc.statd), it indefinitely prevents the
> IANA-assigned well-known service from starting.
>=20
> It seems that a reserved port is completely unnecessary for listener
> sockets. It does not confer any extra privilege or functionality to
> the listener socket, nor do remote clients infer any extra privilege
> from a listener on a port number lower than 1024.
>=20
> Therefore, remove the bindresvport step to prevent svc_tli_create(3)
> from choosing a port number that interferes with well-known services
> assigned to a privileged port.
>=20
> svc_tli_create(3) will now never assign a privileged port
> dynamically. If needed, a caller may still bind to a dynamically-
> assigned reserved port by invoking bindresvport(3) directly and
> passing the already-bound file descriptor to svc_tli_create(3).
>=20
> But that should be a special case. It is no longer the default
> behavior.
>=20
> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D320
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> src/rpc_soc.c     |    7 +++++--
> src/svc_generic.c |   12 ++++--------
> 2 files changed, 9 insertions(+), 10 deletions(-)
>=20
> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
> index ed0892a..5ec96a7 100644
> --- a/src/rpc_soc.c
> +++ b/src/rpc_soc.c
> @@ -67,6 +67,8 @@
>=20
> extern mutex_t	rpcsoc_lock;
>=20
> +extern int __binddynport(int fd);
> +
> static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, =
rpcvers_t,
>     int *, u_int, u_int, char *, int);
> static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
> @@ -145,7 +147,8 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, =
recvsz, tp, flags)
> 	bindaddr.maxlen =3D bindaddr.len =3D  sizeof (struct =
sockaddr_in);
> 	bindaddr.buf =3D raddr;
>=20
> -	bindresvport(fd, NULL);
> +	if (__binddynport(fd) =3D=3D -1)
> +		goto err;

This hunk belongs in the clnt_tli_create(3) patch.


> 	cl =3D clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
> 				sendsz, recvsz);
> 	if (cl) {
> @@ -332,7 +335,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
>=20
> 	memset(&sin, 0, sizeof sin);
> 	sin.sin_family =3D AF_INET;
> -	bindresvport(fd, &sin);
> +	bind(fd, (struct sockaddr *)(void *)&sin, sizeof sin);

This allows dynamic allocation of a reserved port if the caller
has CAP_NET_ADMIN_BIND. A better choice might be to just pass the
fd to svc_tli_create and let it do the bind if needed.

I'll be sending along a "v2" RFC series at some point.


> 	listen(fd, SOMAXCONN);
> 	svc =3D svc_tli_create(fd, nconf, NULL, sendsize, recvsize);
> 	(void) freenetconfigent(nconf);
> diff --git a/src/svc_generic.c b/src/svc_generic.c
> index 7aae796..52a56c2 100644
> --- a/src/svc_generic.c
> +++ b/src/svc_generic.c
> @@ -53,6 +53,7 @@
> #include <rpc/svc.h>
>=20
> extern int __svc_vc_setflag(SVCXPRT *, int);
> +extern int __binddynport(int fd);
>=20
> /*
>  * The highest level interface for server creation.
> @@ -220,15 +221,10 @@ svc_tli_create(fd, nconf, bindaddr, sendsz, =
recvsz)
> 	 */
> 	if (madefd || !__rpc_sockisbound(fd)) {
> 		if (bindaddr =3D=3D NULL) {
> -			if (bindresvport(fd, NULL) < 0) {
> -				memset(&ss, 0, sizeof ss);
> -				ss.ss_family =3D si.si_af;
> -				if (bind(fd, (struct sockaddr *)(void =
*)&ss,
> -				    (socklen_t)si.si_alen) < 0) {
> -					warnx(
> +			if (__binddynport(fd) =3D=3D -1) {
> +				warnx(
> 			"svc_tli_create: could not bind to anonymous =
port");
> -					goto freedata;
> -				}
> +				goto freedata;
> 			}
> 			listen(fd, SOMAXCONN);
> 		} else {
>=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

[PATCH RFC 3/3] Avoid choosing reserved ports in clnt_tli_create(3)

[Chuck Lever]

Callers of clnt_tli_create(3) can specify that an arbitrary port
number be dynamically assigned for the client socket being
created. clnt_tli_create(3) tries bindresvport(3) first in this
case. bindresvport(3) chooses a reserved port if the caller has
CAP_NET_ADMIN_BIND privilege. If this fails, bind(2) is used to
assign a port number from the range above 1024.

This approach becomes a problem whenever bindresvport(3) happens to
choose the port number of a well-known service. If the caller is a
long-running service (like rpc.statd), it indefinitely prevents the
IANA-assigned well-known service from starting.

When using the AUTH_SYS authentication flavor, RPC services can use
the remote client's port number to determine whether the client is
privileged, and thus the UID and GID numbers in the RPC are
trustworthy. However, it's pretty easy for a man-in-the-middle to
replace these while the RPC is in flight anyway. The port number
provides no actual security.

Therefore, remove the bindresvport step to prevent svc_tli_create(3)
from choosing a port number that interferes with well-known services
assigned to a privileged port.

clnt_tli_create(3) will now never assign a privileged port
dynamically. If needed, a caller may still bind to a dynamically-
assigned reserved port by invoking bindresvport(3) directly and
passing the already-bound file descriptor to clnt_tli_create(3).

But that should be a special case. It is no longer the default
behavior.

BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=320
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/clnt_generic.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/clnt_generic.c b/src/clnt_generic.c
index 3f3dabf..e5a314f 100644
--- a/src/clnt_generic.c
+++ b/src/clnt_generic.c
@@ -47,6 +47,7 @@
 
 extern bool_t __rpc_is_local_host(const char *);
 int __rpc_raise_fd(int);
+extern int __binddynport(int fd);
 
 #ifndef NETIDLEN
 #define	NETIDLEN 32
@@ -340,7 +341,8 @@ clnt_tli_create(int fd, const struct netconfig *nconf,
 		servtype = nconf->nc_semantics;
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;
-		bindresvport(fd, NULL);
+		if (__binddynport(fd) == -1)
+			goto err;
 	} else {
 		if (!__rpc_fd2sockinfo(fd, &si))
 			goto err;