[PATCH] portmap: new applet

Denys Vlasenko vda.linux at googlemail.com
Mon May 2 01:46:01 UTC 2011


On Sunday 01 May 2011 23:05, Lukas Huba wrote:
> Thanks. Modified (2).
> 
> Note:
> > > +	if (argc >= 2)
> > Is this `if' really needed?
> No, this is only to suppress warning message: "warning: unused parameter ‘argc’".
> 
> 
> Patch:
> 
> Signed-off-by: Lukas Huba <huba.lukas at centrum.cz>
> ---
>  networking/portmap.c |  247 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 247 insertions(+), 0 deletions(-)
>  create mode 100644 networking/portmap.c
> 
> diff --git a/networking/portmap.c b/networking/portmap.c
> new file mode 100644
> index 0000000..6164d0b
> --- /dev/null
> +++ b/networking/portmap.c
> @@ -0,0 +1,247 @@
> +/* vi: set sw=4 ts=4: */
> +/*
> + * mini portmap implementation for busybox
> + *
> + * Copyright (C) 2011 by Lukas Huba <huba.lukas at centrum.cz>
> + *
> + * Licensed under GPLv2, see file LICENSE in this source tree.
> + */
> +
> +//applet:IF_PORTMAP(APPLET(portmap, BB_DIR_SBIN, BB_SUID_DROP))
> +
> +//kbuild:lib-$(CONFIG_PORTMAP) += portmap.o
> +
> +//config:config PORTMAP
> +//config:	bool "portmap"
> +//config:	default n
> +//config:	select PLATFORM_LINUX
> +//config:	select FEATURE_SYSLOG
> +//config:	help
> +//config:		RPC program, version to DARPA port mapper.
> +//config:
> +//config:config PORTMAP_ITEMS_MAX
> +//config:	int "Maximum RPC services"
> +//config:	default 32
> +//config:	depends on PORTMAP
> +//config:	help
> +//config:		Maximum RPC services which portmap is able to store.
> +//config:		That's for better security.
> +//config:
> +
> +//usage:#define portmap_trivial_usage
> +//usage:	"[hpr]"
> +//usage:#define portmap_full_usage "\n\n"
> +//usage:	"RPC program, version to DARPA port mapper\n"
> +//usage:	"\nOptions:"
> +//usage:	"\n	-h	Specify specific IP addresses to bind to for requests"
> +//usage:	"\n	-p	Insecure mode. Allow calls to SET and UNSET from any
> +//usage:			port (allow port > 1024)"
> +//usage:	"\n	-r	Insecure mode. Allow calls to SET and UNSET from any
> +//usage:			host"

Output is broken:

$ ./busybox portmap --help
BusyBox v1.19.0.git (2011-05-01 03:00:25 CEST) multi-call binary.

Usage: portmap [hpr]

RPC program, version to DARPA port mapper

Options:
        -h      Specify specific IP addresses to bind to for requests
        -p      Insecure mode. Allow calls to SET and UNSET from any                    port (allow port > 1024)
        -r      Insecure mode. Allow calls to SET and UNSET from any                    host

It is also wrong: -h takes parameter (-h ADDR), why help doesn't show it?
Why it doesn't show '-' character for options?

The text should be *short* (but understanbable). such as:

Usage: portmap [-pr] [-h ADDR]

Port mapper daemon

Options:
        -h ADDR   Listen on ADDR
...

> +
> +#include "libbb.h"
> +#include <rpc/rpc.h>
> +#include <rpc/pmap_prot.h>
> +#include <net/if.h>
> +#include <sys/ioctl.h>
> +#include <syslog.h>
> +
> +/* commandline parameters */
> +#define FLAGS_ARG		"h:pr"
> +#define FLAG_HOST		(1 << 0)
> +#define FLAG_SPORT		(2 << 0)
> +#define FLAG_SREMOTE	(4 << 0)
> +
> +struct globals {
> +	int flags;	/* commanline flags */

Use already existing option_mask32 global variable.


> +	int pn;	/* count of items in portmap list */
> +	struct pmaplist *pl;	/* portmap list */
> +	int ifs;	/* default count of network interfaces (later it updates itself)
> +				it's for higher efficiency (re|m)alloc */
> +} FIX_ALIASING;
> +#define G (*(struct globals *)&bb_common_bufsiz1)
> +
> +static bool_t pmapproc_set(const struct pmap *pr)

Let's use bool or int instaed of bool_t?

> +{
> +	struct pmaplist *p, *pp = NULL, **n;
> +	if (G.pn >= CONFIG_PORTMAP_ITEMS_MAX) {
> +		bb_error_msg("All available resources are used! Count of registered "\
> +					"RPC services is limited to %i.", CONFIG_PORTMAP_ITEMS_MAX);
> +		return FALSE;
> +	}
> +	for (p = G.pl; p != NULL; p = p->pml_next) {
> +		if (p->pml_map.pm_prog == pr->pm_prog
> +			&& p->pml_map.pm_vers == pr->pm_vers
> +			&& p->pml_map.pm_prot == pr->pm_prot) {
> +			return FALSE;

I think this style visually separates conditions from statements better:

		if (p->pml_map.pm_prog == pr->pm_prog
		 && p->pml_map.pm_vers == pr->pm_vers
		 && p->pml_map.pm_prot == pr->pm_prot
		) {
			return FALSE;


> +		}
> +		pp = p;
> +	}
> +	n = pp == NULL ? &G.pl : &pp->pml_next;
> +	*n = xmalloc(sizeof(struct pmaplist));
> +	(*n)->pml_map = *pr;
> +	(*n)->pml_next = NULL;

Use xzalloc, then you can avoid setting pml_next field to NULL/0.

> +	G.pn++;
> +	return TRUE;
> +}
> +
> +static bool_t pmapproc_unset(const struct pmap *pr)
> +{
> +	bool_t res = FALSE;
> +	struct pmaplist *p, *pp = NULL;
> +	for (p = G.pl; p != NULL; p = p->pml_next) {
> +		if (p->pml_map.pm_prog == pr->pm_prog
> +			&& p->pml_map.pm_vers == pr->pm_vers) {
> +			res = TRUE;
> +			if (p->pml_next == NULL) {
> +				if (pp == NULL) {
> +					G.pl = NULL;
> +				} else {
> +					pp->pml_next = NULL;
> +				}
> +			} else {
> +				if (pp == NULL) {
> +					G.pl = p->pml_next;
> +				} else {
> +					pp->pml_next = p->pml_next;
> +				}
> +			}
> +			free(p);
> +			G.pn--;
> +		} else {
> +			pp = p;
> +		}
> +	}
> +	return res;
> +}
> +
> +static unsigned int pmapproc_getport(const struct pmap *pr)
> +{
> +	struct pmaplist *p;
> +	for (p = G.pl; p != NULL; p = p->pml_next) {
> +		if (p->pml_map.pm_prog == pr->pm_prog
> +			&& p->pml_map.pm_vers == pr->pm_vers) {
> +			return p->pml_map.pm_port;
> +		}
> +	}
> +	return 0;
> +}
> +


> +static bool_t check_security(SVCXPRT *xprt)
> +{
> +	if (!(G.flags & FLAG_SPORT))
> +		/* secure mode
> +		 * SET and UNSET procs can be exec only from port <= 1024 */
> +		if (htons(svc_getcaller(xprt)->sin_port) > 1024)
> +			return FALSE;
> +	if (!(G.flags & FLAG_SREMOTE)) {
> +		/* secure mode
> +		 * SET and UNSET procs can be exec only from local machine */
> +		bool_t res = FALSE;
> +		struct ifconf ifc = {.ifc_req = NULL};
> +		int s = xsocket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> +		for (int cnt = 0; !cnt || ifc.ifc_len/sizeof(struct ifreq) == cnt;
> +			cnt += G.ifs) {
> +			ifc.ifc_len = (cnt+G.ifs)*sizeof(struct ifreq);
> +			ifc.ifc_req = xrealloc(ifc.ifc_req, ifc.ifc_len);
> +			if (ioctl(s, SIOCGIFCONF, &ifc) < 0) {
> +				bb_error_msg("ioctl(SIOCGIFCONF) error");
> +				goto out;
> +			}
> +		}
> +		G.ifs = ifc.ifc_len/sizeof(struct ifreq)+1;
> +		for (int i = 0; i < G.ifs-1; i++) {
> +			if (((struct sockaddr_in *)&ifc.ifc_req[i].ifr_addr)->
> +				sin_addr.s_addr == svc_getcaller(xprt)->sin_addr.s_addr) {

Please use a temp variable for better readability...

> +				res = TRUE;
> +				goto out;
> +			}
> +		}
> +out:
> +		free(ifc.ifc_req);
> +		return res;
> +	}
> +	return TRUE;
> +}

The above function is IPv4-only. Can we get rid of this limitation?

1st, checking for ports < 1024 on remote calls is nearly pointless:
it is a verstige of the era when the case of *unprivileged* user
attacking over network was a usual case. These days, remote attackers
usually will have no trouble using a machine where they have root
(such as using their own laptop...).

2nd, checking for "localness" by enumerating all IPv4 addresses we have
looks like a hack to me. It is not IPv6 clean.
Is there a better way?


> +
> +static void pmapproc(struct svc_req *rqstp, SVCXPRT *xprt)
> +{
> +	unsigned int res;
> +	struct pmap pmap;
> +
> +	if (rqstp->rq_proc == PMAPPROC_SET || rqstp->rq_proc == PMAPPROC_UNSET)
> +		if (check_security(xprt) == FALSE) {
> +			svcerr_weakauth(xprt);
> +			return;
> +		}
> +
> +	if (rqstp->rq_proc > PMAPPROC_NULL && rqstp->rq_proc < PMAPPROC_DUMP)
> +		if (!svc_getargs(xprt, (xdrproc_t)xdr_pmap, (caddr_t)&pmap))
> +			return;
> +
> +	switch (rqstp->rq_proc) {
> +		case PMAPPROC_NULL:
> +			svc_sendreply(xprt, (xdrproc_t)xdr_void, (caddr_t)NULL);
> +			break;
> +		case PMAPPROC_SET:
> +			res = (unsigned int)pmapproc_set(&pmap);
> +			svc_sendreply(xprt, (xdrproc_t)xdr_bool, (caddr_t)&res);
> +			break;
> +		case PMAPPROC_UNSET:
> +			res = (unsigned int)pmapproc_unset(&pmap);
> +			svc_sendreply(xprt, (xdrproc_t)xdr_bool, (caddr_t)&res);
> +			break;
> +		case PMAPPROC_GETPORT:
> +			res = pmapproc_getport(&pmap);
> +			svc_sendreply(xprt, (xdrproc_t)xdr_u_int, (caddr_t)&res);
> +			break;
> +		case PMAPPROC_DUMP:
> +			svc_sendreply(xprt, (xdrproc_t)xdr_pmaplist, (caddr_t)&G.pl);
> +			break;
> +		default:
> +			svcerr_noproc(xprt);
> +	}
> +}
> +
> +int portmap_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> +int portmap_main(int argc, char **argv)
> +{
> +	const char *host = "0.0.0.0";

Please use NULL. "0.0.0.0" is a IPv4-ism.

> +	if (argc >= 2)
> +		G.flags = getopt32(argv, FLAGS_ARG, &host);
> +
> +	G.ifs = 5;
> +	bb_daemonize(DAEMON_CHDIR_ROOT);

bb_daemonize is not NOMMU-safe. Use bb_daemonize_or_rexec.
Also, I'd prefer to have an option to not daemonize.

> +
> +	openlog(applet_name, LOG_PID | LOG_ERR, LOG_DAEMON);
> +	logmode |= LOGMODE_SYSLOG;
> +
> +	{
> +		int sock;
> +		SVCXPRT *xprt;
> +		struct pmap pm = {PMAPPROG, PMAPVERS, IPPROTO_UDP, PMAPPORT};
> +
> +		/* udp listening */
> +		sock = create_and_bind_dgram_or_die(host, PMAPPORT);
> +		xprt = svcudp_create(sock);
> +		if (xprt == (SVCXPRT *)NULL)
> +			bb_error_msg_and_die("cannot start portmap");
> +		pmapproc_set(&pm);
> +		if (svc_register(xprt, PMAPPROG, PMAPVERS, pmapproc, 0))
> +			G.pn++;
> +
> +		/* tcp listening */
> +		sock = create_and_bind_stream_or_die(host, PMAPPORT);
> +		xprt = svctcp_create(sock, RPCSMALLMSGSIZE, RPCSMALLMSGSIZE);
> +		if (xprt == (SVCXPRT *)NULL)
> +			bb_error_msg_and_die("cannot start portmap");
> +		pm.pm_prot = IPPROTO_TCP;
> +		pmapproc_set(&pm);
> +		if (svc_register(xprt, PMAPPROG, PMAPVERS, pmapproc, 0))
> +			G.pn++;
> +	}
> +
> +	svc_run();
> +
> +	return EXIT_SUCCESS;
> +}



In general, I'm afraid the whole svc_FOO() stuff is IPv6-incapable...
Since we use such a small subset here, maybe we just open-code it?

-- 
vda


More information about the busybox mailing list