[rpcbind PATCH 0/6] rpcbind: various hardening fixes

[rpcbind PATCH 0/6] rpcbind: various hardening fixes

[Scott Mayhew]

Various hardening fixes for rpcbind.  Many of these are related to
CALLIT/BCAST/INDIRECT processing, which in theory should be disabled
when --enable-rmtcalls was not passed to the configure script, except
currently that only prevents the forwarding port from being created.
Patch 3/6 addresses this by completely disabling remote call
functionality when --enable-rmtcalls is unset.

Scott Mayhew (6):
  rpcbind: only log failures in check_callit() if connection logging is
    enabled
  rpcbind: bound stats lists in rpcbs_getaddr() and rpcbs_rmtcall()
  rpcbind: fully disable remote calls when --enable-rmtcalls is unset
  rpcbind: restrict RPCBPROC_GETSTAT to loopback callers
  rpcbind: check for null netmask in addrmerge()
  rpcbind: only free the forward slot if the reply xid matched the
    request xid

 src/pmap_svc.c     |  2 ++
 src/rpcb_stat.c    | 37 ++++++++++++++++++++++++++++++++++---
 src/rpcb_svc.c     |  2 ++
 src/rpcb_svc_4.c   |  2 ++
 src/rpcb_svc_com.c | 24 ++++++++++++++++++++----
 src/rpcbind.h      |  5 +++++
 src/security.c     | 16 +++++++++++-----
 src/util.c         |  3 ++-
 8 files changed, 78 insertions(+), 13 deletions(-)

-- 
2.55.0

[rpcbind PATCH 1/6] rpcbind: only log failures in check_callit() if connection logging is enabled

[Scott Mayhew]

logit() forks a child process to call syslog().  A client can spam
CALLIT/BROADCAST/INDIRECT requests, leading to CPU, scheduler, and PID
pressure.  It's better to only log failures from check_callit() if
rpcbind is running with the '-l' option (connection logging).

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/security.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/security.c b/src/security.c
index 38967dd..02651d7 100644
--- a/src/security.c
+++ b/src/security.c
@@ -343,11 +343,13 @@ check_callit(SVCXPRT *xprt, struct r_rmtcall_args *args, int versnum /*__unused*
 	return 1;
 deny:
 #ifdef LIBWRAP
-	logit(deny_severity, sa, args->rmt_proc, args->rmt_prog,
-	    ": indirect call not allowed");
+	if (verboselog)
+		logit(deny_severity, sa, args->rmt_proc, args->rmt_prog,
+		      ": indirect call not allowed");
 #else
-	logit(LOG_AUTH|LOG_WARNING, sa, args->rmt_proc, args->rmt_prog,
-	    ": indirect call not allowed");
+	if (verboselog)
+		logit(LOG_AUTH|LOG_WARNING, sa, args->rmt_proc, args->rmt_prog,
+		      ": indirect call not allowed");
 #endif
 	return 0;
 }
-- 
2.55.0

[rpcbind PATCH 2/6] rpcbind: bound stats lists in rpcbs_getaddr() and rpcbs_rmtcall()

[Scott Mayhew]

Limit addrinfo and rmtinfo linked lists to 4096 entries by trimming
the oldest entries from the tail after prepending a new one. This
prevents unbounded memory growth from a large number of unique
program/version/netid combinations.

Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/rpcb_stat.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/rpcb_stat.c b/src/rpcb_stat.c
index 2e5226c..0799a47 100644
--- a/src/rpcb_stat.c
+++ b/src/rpcb_stat.c
@@ -50,6 +50,8 @@
 #include <string.h>
 #include "rpcbind.h"
 
+#define MAX_STAT_BUCKETS 4096
+
 static rpcb_stat_byvers inf;
 
 void
@@ -104,8 +106,9 @@ void
 rpcbs_getaddr(rpcvers_t rtype, rpcprog_t prog, rpcvers_t vers, char *netid,
 	      char *uaddr)
 {
-	rpcbs_addrlist *al;
+	rpcbs_addrlist *al, *cut = NULL, *tmp;
 	struct netconfig *nconf;
+	int listlen = 0;
 
 	if (rtype >= RPCBVERS_STAT)
 		return;
@@ -121,6 +124,9 @@ rpcbs_getaddr(rpcvers_t rtype, rpcprog_t prog, rpcvers_t vers, char *netid,
 				al->success++;
 			return;
 		}
+		listlen++;
+		if (listlen == MAX_STAT_BUCKETS - 1)
+			cut = al;
 	}
 	nconf = rpcbind_get_conf(netid);
 	if (nconf == NULL) {
@@ -142,14 +148,25 @@ rpcbs_getaddr(rpcvers_t rtype, rpcprog_t prog, rpcvers_t vers, char *netid,
 	}
 	al->next = inf[rtype].addrinfo;
 	inf[rtype].addrinfo = al;
+
+	if (cut) {
+		al = cut->next;
+		cut->next = NULL;
+		while (al) {
+			tmp = al;
+			al = al->next;
+			free(tmp);
+		}
+	}
 }
 
 void
 rpcbs_rmtcall(rpcvers_t rtype, rpcproc_t rpcbproc, rpcprog_t prog,
 	      rpcvers_t vers, rpcproc_t proc, char *netid, rpcblist_ptr rbl)
 {
-	rpcbs_rmtcalllist *rl;
+	rpcbs_rmtcalllist *rl, *cut = NULL, *tmp;
 	struct netconfig *nconf;
+	int listlen = 0;
 
 	if (rtype >= RPCBVERS_STAT)
 		return;
@@ -170,6 +187,9 @@ rpcbs_rmtcall(rpcvers_t rtype, rpcproc_t rpcbproc, rpcprog_t prog,
 				rl->indirect++;
 			return;
 		}
+		listlen++;
+		if (listlen == MAX_STAT_BUCKETS - 1)
+			cut = rl;
 	}
 	nconf = rpcbind_get_conf(netid);
 	if (nconf == NULL) {
@@ -194,7 +214,16 @@ rpcbs_rmtcall(rpcvers_t rtype, rpcproc_t rpcbproc, rpcprog_t prog,
 	rl->indirect = 1;
 	rl->next = inf[rtype].rmtinfo;
 	inf[rtype].rmtinfo = rl;
-	return;
+
+	if (cut) {
+		rl = cut->next;
+		cut->next = NULL;
+		while (rl) {
+			tmp = rl;
+			rl = rl->next;
+			free(tmp);
+		}
+	}
 }
 
 void *
-- 
2.55.0

[rpcbind PATCH 3/6] rpcbind: fully disable remote calls when --enable-rmtcalls is unset

[Scott Mayhew]

Commit 2e9c289 ("rpcbind: Disable remote calls by default") added the
'--enable-rmtcalls' option, which is off by default, but that only
suppresses the creation of the rmtcallfd for a given netconfig entry.

In this case a client can still send rpcbind CALLIT/BCAST/INDIRECT
requests, which will eventually bail out in rpcbproc_callit_com() after
find_rmtcallfd_by_netid() fails... but by that point rpcbind may have
called logit() (which forks a child process) or rpcbs_rmtcall() (which
allocates a stats bucket if the request contained a unique program or
procedure number).

If configure is run without the '--enable-rmtcalls' option, then fully
disable remote call functionality.  Attempts to call
CALLIT/BCAST/INDIRECT will receive an accept state of PROC_UNAVAIL.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/pmap_svc.c     |  2 ++
 src/rpcb_stat.c    |  2 ++
 src/rpcb_svc.c     |  2 ++
 src/rpcb_svc_4.c   |  2 ++
 src/rpcb_svc_com.c | 19 ++++++++++++++++---
 src/rpcbind.h      |  5 +++++
 src/security.c     |  4 ++++
 7 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/src/pmap_svc.c b/src/pmap_svc.c
index e5a5d0f..27091ac 100644
--- a/src/pmap_svc.c
+++ b/src/pmap_svc.c
@@ -125,6 +125,7 @@ pmap_service(struct svc_req *rqstp, SVCXPRT *xprt)
 		pmapproc_dump(rqstp, xprt);
 		break;
 
+#ifdef RMTCALLS
 	case PMAPPROC_CALLIT:
 		/*
 		 * Calls a procedure on the local machine. If the requested
@@ -135,6 +136,7 @@ pmap_service(struct svc_req *rqstp, SVCXPRT *xprt)
 		 */
 		rpcbproc_callit_com(rqstp, xprt, PMAPPROC_CALLIT, PMAPVERS);
 		break;
+#endif /* RMTCALLS */
 
 	default:
 		svcerr_noproc(xprt);
diff --git a/src/rpcb_stat.c b/src/rpcb_stat.c
index 0799a47..ef2b3a9 100644
--- a/src/rpcb_stat.c
+++ b/src/rpcb_stat.c
@@ -160,6 +160,7 @@ rpcbs_getaddr(rpcvers_t rtype, rpcprog_t prog, rpcvers_t vers, char *netid,
 	}
 }
 
+#ifdef RMTCALLS
 void
 rpcbs_rmtcall(rpcvers_t rtype, rpcproc_t rpcbproc, rpcprog_t prog,
 	      rpcvers_t vers, rpcproc_t proc, char *netid, rpcblist_ptr rbl)
@@ -225,6 +226,7 @@ rpcbs_rmtcall(rpcvers_t rtype, rpcproc_t rpcbproc, rpcprog_t prog,
 		}
 	}
 }
+#endif /* RMTCALLS */
 
 void *
 rpcbproc_getstat(void *arg /*__unused*/, struct svc_req *req /*__unused*/,
diff --git a/src/rpcb_svc.c b/src/rpcb_svc.c
index 091f530..e52e82f 100644
--- a/src/rpcb_svc.c
+++ b/src/rpcb_svc.c
@@ -122,9 +122,11 @@ rpcb_service_3(struct svc_req *rqstp, SVCXPRT *transp)
 		local = rpcbproc_dump_3_local;
 		break;
 
+#ifdef RMTCALLS
 	case RPCBPROC_CALLIT:
 		rpcbproc_callit_com(rqstp, transp, rqstp->rq_proc, RPCBVERS);
 		return;
+#endif /* RMTCALLS */
 
 	case RPCBPROC_GETTIME:
 #ifdef RPCBIND_DEBUG
diff --git a/src/rpcb_svc_4.c b/src/rpcb_svc_4.c
index eebbbbe..dbb8839 100644
--- a/src/rpcb_svc_4.c
+++ b/src/rpcb_svc_4.c
@@ -141,6 +141,7 @@ rpcb_service_4(struct svc_req *rqstp, SVCXPRT *transp)
 		local = rpcbproc_dump_4_local;
 		break;
 
+#ifdef RMTCALLS
 	case RPCBPROC_INDIRECT:
 #ifdef RPCBIND_DEBUG
 		if (debugging)
@@ -157,6 +158,7 @@ rpcb_service_4(struct svc_req *rqstp, SVCXPRT *transp)
 #endif
 		rpcbproc_callit_com(rqstp, transp, rqstp->rq_proc, RPCBVERS4);
 		return;
+#endif /* RMTCALLS */
 
 	case RPCBPROC_GETTIME:
 #ifdef RPCBIND_DEBUG
diff --git a/src/rpcb_svc_com.c b/src/rpcb_svc_com.c
index 6d0d72d..0763fc4 100644
--- a/src/rpcb_svc_com.c
+++ b/src/rpcb_svc_com.c
@@ -74,6 +74,8 @@
 
 
 static char *nullstring = "";
+
+#ifdef RMTCALLS
 static int rpcb_rmtcalls;
 
 struct rmtcallfd_list {
@@ -120,6 +122,8 @@ static void xprt_set_caller(SVCXPRT *, struct finfo *);
 static void send_svcsyserr(SVCXPRT *, struct finfo *);
 static void handle_reply(int, SVCXPRT *);
 static void find_versions(rpcprog_t, char *, rpcvers_t *, rpcvers_t *);
+#endif /* RMTCALLS */
+
 static rpcblist_ptr find_service(rpcprog_t, rpcvers_t, char *);
 static char *getowner(SVCXPRT *, char *, size_t);
 static int add_pmaplist(RPCB *);
@@ -429,7 +433,7 @@ rpcbproc_taddr2uaddr_com(void *arg, struct svc_req *rqstp /*__unused*/,
 	return (void *)&uaddr;
 }
 
-
+#ifdef RMTCALLS
 static bool_t
 xdr_encap_parms(XDR *xdrs, struct encap_parms *epp)
 {
@@ -1048,12 +1052,15 @@ netbuffree(struct netbuf *ap)
 	free(ap->buf);
 	free(ap);
 }
-
+#endif /* RMTCALLS */
 
 void
 my_svc_run()
 {
-	int poll_ret, check_ret;
+	int poll_ret;
+#ifdef RMTCALLS
+	int check_ret;
+#endif /* RMTCALLS */
 
 	for (;;) {
 	        struct pollfd my_pollfd[svc_max_pollfd];
@@ -1087,14 +1094,19 @@ my_svc_run()
 			 * don't call svc_getreq_poll.  Otherwise, there
 			 * must be another so we must call svc_getreq_poll.
 			 */
+#ifdef RMTCALLS
 			if ((check_ret = check_rmtcalls(my_pollfd, svc_max_pollfd)) ==
 			    poll_ret)
 				continue;
 			svc_getreq_poll(my_pollfd, poll_ret-check_ret);
+#else
+			svc_getreq_poll(my_pollfd, poll_ret);
+#endif /* RMTCALLS */
 		}
 	}
 }
 
+#ifdef RMTCALLS
 static int
 check_rmtcalls(struct pollfd *pfds, int nfds)
 {
@@ -1289,6 +1301,7 @@ find_versions(rpcprog_t prog, char *netid, rpcvers_t *lowvp, rpcvers_t *highvp)
 	*highvp = highv;
 	return;
 }
+#endif /* RMTCALLS */
 
 /*
  * returns the item with the given program, version number and netid.
diff --git a/src/rpcbind.h b/src/rpcbind.h
index 5b1a9bb..ef3aab8 100644
--- a/src/rpcbind.h
+++ b/src/rpcbind.h
@@ -89,8 +89,10 @@ void rpcbs_procinfo(rpcvers_t, rpcproc_t);
 void rpcbs_set(rpcvers_t, bool_t);
 void rpcbs_unset(rpcvers_t, bool_t);
 void rpcbs_getaddr(rpcvers_t, rpcprog_t, rpcvers_t, char *, char *);
+#ifdef RMTCALLS
 void rpcbs_rmtcall(rpcvers_t, rpcproc_t, rpcprog_t, rpcvers_t, rpcproc_t,
 			char *, rpcblist_ptr);
+#endif /* RMTCALLS */
 void *rpcbproc_getstat(void *, struct svc_req *, SVCXPRT *, rpcvers_t);
 
 void rpcb_service_3(struct svc_req *, SVCXPRT *);
@@ -110,9 +112,12 @@ void *rpcbproc_uaddr2taddr_com(void *, struct svc_req *,
 					     SVCXPRT *, rpcvers_t);
 void *rpcbproc_taddr2uaddr_com(void *, struct svc_req *, SVCXPRT *,
 				    rpcvers_t);
+#ifdef RMTCALLS
 int create_rmtcall_fd(struct netconfig *);
 void rpcbproc_callit_com(struct svc_req *, SVCXPRT *, rpcvers_t,
 			      rpcvers_t);
+#endif /* RMTCALLS */
+
 void my_svc_run(void);
 
 void rpcbind_abort(void);
diff --git a/src/security.c b/src/security.c
index 02651d7..6bd22fd 100644
--- a/src/security.c
+++ b/src/security.c
@@ -99,8 +99,10 @@ check_access(SVCXPRT *xprt, rpcproc_t proc, rpcprog_t prog, unsigned int rpcbver
 		}
 		break;
 	case RPCBPROC_GETADDR:
+#ifdef RMTCALLS
 	case RPCBPROC_CALLIT:
 	case RPCBPROC_INDIRECT:
+#endif /* RMTCALLS */
 	case RPCBPROC_DUMP:
 	case RPCBPROC_GETTIME:
 	case RPCBPROC_UADDR2TADDR:
@@ -290,6 +292,7 @@ logit(int severity, struct sockaddr *addr, rpcproc_t procnum, rpcprog_t prognum,
 	}
 }
 
+#ifdef RMTCALLS
 int
 check_callit(SVCXPRT *xprt, struct r_rmtcall_args *args, int versnum /*__unused*/)
 {
@@ -353,3 +356,4 @@ deny:
 #endif
 	return 0;
 }
+#endif /* RMTCALLS */
-- 
2.55.0

[rpcbind PATCH 4/6] rpcbind: restrict RPCBPROC_GETSTAT to loopback callers

[Scott Mayhew]

There's no reason to allow remote machines to query the local machine's
rpcbind statistics.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/security.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security.c b/src/security.c
index 6bd22fd..3b55151 100644
--- a/src/security.c
+++ b/src/security.c
@@ -87,6 +87,7 @@ check_access(SVCXPRT *xprt, rpcproc_t proc, rpcprog_t prog, unsigned int rpcbver
 	switch (proc) {
 	case RPCBPROC_SET:
 	case RPCBPROC_UNSET:
+	case RPCBPROC_GETSTAT:
 		if (!insecure && !is_loopback(caller)) {
 #ifdef RPCBIND_DEBUG
 			  if (debugging)
@@ -109,7 +110,6 @@ check_access(SVCXPRT *xprt, rpcproc_t proc, rpcprog_t prog, unsigned int rpcbver
 	case RPCBPROC_TADDR2UADDR:
 	case RPCBPROC_GETVERSADDR:
 	case RPCBPROC_GETADDRLIST:
-	case RPCBPROC_GETSTAT:
 	default:
 		break;
 	}
-- 
2.55.0

[rpcbind PATCH 5/6] rpcbind: check for null netmask in addrmerge()

[Scott Mayhew]

getifaddrs(3) states that ifa_netmask may contain a null pointer, so we
should skip interfaces where ifa_netmask is null to avoid triggering a
null dereference in bitmaskcmp().

This is mainly a defensive change - we only call bitmaskcmp() for
AF_INET/AF_INET6 addresses, and I'm not sure it's possible for an
interface to have an IP address but no netmask.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util.c b/src/util.c
index ead4899..a61a8c6 100644
--- a/src/util.c
+++ b/src/util.c
@@ -165,7 +165,8 @@ addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
 		ifsa = ifap->ifa_addr;
 		ifmasksa = ifap->ifa_netmask;
 
-		if (ifsa == NULL || ifsa->sa_family != hint_sa->sa_family ||
+		if (ifsa == NULL || ifmasksa == NULL ||
+		    ifsa->sa_family != hint_sa->sa_family ||
 		    !(ifap->ifa_flags & IFF_UP))
 			continue;
 
-- 
2.55.0

[rpcbind PATCH 6/6] rpcbind: only free the forward slot if the reply xid matched the request xid

[Scott Mayhew]

If remote calls are enabled and if the ephemeral forwarding ports are
exposed, it's possible to spoof replies to forwarded requests using a
bogus xid, causing the forward slots to be freed and potentially
interfering with sending replies to legitimate remote call requesters.

Only free the forward slot in handle_reply() if the reply xid matched
the original request xid.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 src/rpcb_svc_com.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/rpcb_svc_com.c b/src/rpcb_svc_com.c
index 0763fc4..2c007eb 100644
--- a/src/rpcb_svc_com.c
+++ b/src/rpcb_svc_com.c
@@ -1196,6 +1196,7 @@ handle_reply(int fd, SVCXPRT *xprt)
 	struct r_rmtcall_args a;
 	struct sockaddr_storage ss;
 	socklen_t fromlen;
+	bool_t matched_xid = FALSE;
 
 	buffer = malloc(RPC_BUF_MAX);
 	if (buffer == NULL)
@@ -1234,6 +1235,7 @@ handle_reply(int fd, SVCXPRT *xprt)
 	if (fi == NULL) {
 		goto done;
 	}
+	matched_xid = TRUE;
 	_seterr_reply(&reply_msg, &reply_error);
 	if (reply_error.re_status != RPC_SUCCESS) {
 		if (debugging)
@@ -1271,7 +1273,8 @@ done:
 		xlog(LOG_DEBUG, "handle_reply:  NULL xid on exit!\n");
 	}
 #endif
-	} else
+	}
+	if (matched_xid == TRUE)
 		(void) free_slot_by_xid(reply_msg.rm_xid);
 	return;
 }
-- 
2.55.0