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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
