Date: Mon, 13 Jan 2014 21:28:08 +0400 From: Mikhail <mp39590@gmail.com> To: freebsd-security@freebsd.org Subject: Re: capsicum and ping(8) Message-ID: <20140113172808.GA10345@edge.bac.lab> In-Reply-To: <20140109200256.GA1658@garage.freebsd.pl> References: <20140109161904.GA96816@edge.bac.lab> <20140109200256.GA1658@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
--Y/WcH0a6A93yCHGr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Hello, Pawel!
On 00:02 10-Jan 2014 Pawel Jakub Dawidek wrote:
> Now that you added casper to the game, I'd move gethostbyname2() until
> after we enter the sandbox and open system.dns service, but before we
> limit the service to only reverse lookups. It does process network
> packets after all.
I've moved casper initialization before our first gethostbyname2() and
added statements to use those, if they're available. I didn't move
cap_enter() for now, since our hands are still tied with bind() and
connect() system calls which use the results of those DNS lookups.
Also, code, limiting DNS resolution only to 'ADDR' after those
gethostbyname2()'s has been added.
> [...]
> > +int s; /* send socket file descriptor */
> > +int s1; /* receive socket file descriptor */
>
> I'd much prefer to use some more meaningful variable names. Like 'ssend'
> and 'srecv' or something similar.
Fixed.
> > +#ifdef HAVE_LIBCAPSICUM
> > +cap_channel_t *capdns;
> > +#endif /* HAVE_LIBCAPSICUM */
>
> Not sure why other globals variables aren't static, but it should be
> static. I don't think it is used anywhere else outside this source file.
Fixed.
> > s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
> > sockerrno = errno;
> > + s1 = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
> > + sock1errno = errno;
>
> I wonder if dup(2) would be more intuitive here. Looking at the code I
> expect those two sockets to be different, but they aren't. dup(2) would
> make that 100% clear. I'll leave it to you to decide, I don't have a
> strong opinion on this, really.
>
> I'd still prefer to see the explanation you gave in the e-mail why do we
> need those two sockets in a comment.
Comment has been added.
I've tried dup(), but seem it duplicates just a reference to an object,
but not the object itself, and we end up with one socket (as in old
ping) and two references to it, and this socket is failing to receive
multicast/broadcast. Maybe I misunderstood your proposition.
> > + if (options & F_NUMERIC)
> > + cansandbox = 1;
>
> Can you make 'cansandbox' to be of type 'bool'?
Fixed.
> > + /*
> > + * Here we enter capapility mode (see capsicum(4)). Further down
> > + * creation of new file descriptors is forbidden.
>
> This is not entirely true. You can create file descriptors with dup(2),
> dup2(2), fcntl(F_DUP2FD), socket(2), etc. What you can't do is to
> address global namespaces. I'd clarify that comment.
Fixed.
> > + cap_rights_init(&rights_s1, CAP_RECV, CAP_EVENT, CAP_SETSOCKOPT);
> > + if (cap_rights_limit(s1, &rights_s1) < 0 && errno != ENOSYS)
> > + err(1, "cap_rights_limit socket1");
> > +
> > + cap_rights_init(&rights_s, CAP_SEND, CAP_SETSOCKOPT);
> > + if (cap_rights_limit(s, &rights_s) < 0 && errno != ENOSYS)
> > + err(1, "cap_rights_limit socket");
> [...]
> > + cap_rights_clear(&rights_s1, CAP_SETSOCKOPT);
> > + if (cap_rights_limit(s1, &rights_s1) < 0 && errno != ENOSYS)
> > + err(1, "cap_rights_limit socket1 setsockopt");
> [...]
> > + /*
> > + * We don't call setsockopt() anywhere further for 's', we don't need
> > + * corresponding capability, drop it.
> > + */
> > + cap_rights_clear(&rights_s, CAP_SETSOCKOPT);
> > + if (cap_rights_limit(s, &rights_s) < 0 && errno != ENOSYS)
> > + err(1, "cap_rights_limit socket setsockopt");
>
> This made me wonder. We have two choices here: either use
> cap_rights_init() first and then drop capability rights we don't need
> anymore with cap_rights_clear() as you did or to use cap_rights_init()
> twice and provide list of capability rights explicitly. I think I'm more
> in favour of providing capability rights explicitly. If we add some
> additional rights in the future to the first cap_rights_init() we may
> forget add them to cap_rights_clear() below. That's why I'd change the
> code not to use cap_rights_clear(), but add a comment above second
> cap_rights_init() round saying which right we are removing. As a nice
> side-effect this would allow us to use only one 'rights' variable.
Fixed.
Also, I've fixed few other things which were found:
- Makefile is changed to not to define 'HAVE_LIBCAPSICUM', if 'RESCUE'
is defined, it allows us to pass 'rescue' build since for now we
can't link it with libcapsicum (changing Makefile looks more neat
than .c file);
- clearing CAP_SETSOCKOPT for 'srecv' was moved lower;
- closing comments for short #ifdef/#endifs were removed;
- libnv references were removed from Makefile, since we don't use it
directly.
--Y/WcH0a6A93yCHGr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="ping_20140113.patch"
Index: sbin/ping/Makefile
===================================================================
--- sbin/ping/Makefile (revision 260583)
+++ sbin/ping/Makefile (working copy)
@@ -1,6 +1,8 @@
# @(#)Makefile 8.1 (Berkeley) 6/5/93
# $FreeBSD$
+.include <bsd.own.mk>
+
PROG= ping
MAN= ping.8
BINOWN= root
@@ -9,6 +11,12 @@
DPADD= ${LIBM}
LDADD= -lm
+.if ${MK_CASPER} != "no" && !defined(RESCUE)
+DPADD+= ${LIBCAPSICUM}
+LDADD+= -lcapsicum
+CFLAGS+=-DHAVE_LIBCAPSICUM
+.endif
+
.if !defined(RELEASE_CRUNCH)
CFLAGS+=-DIPSEC
DPADD+= ${LIBIPSEC}
Index: sbin/ping/ping.c
===================================================================
--- sbin/ping/ping.c (revision 260583)
+++ sbin/ping/ping.c (working copy)
@@ -63,6 +63,7 @@
*/
#include <sys/param.h> /* NB: we rely on this for <sys/types.h> */
+#include <sys/capability.h>
#include <sys/socket.h>
#include <sys/sysctl.h>
#include <sys/time.h>
@@ -74,6 +75,11 @@
#include <netinet/ip_icmp.h>
#include <netinet/ip_var.h>
#include <arpa/inet.h>
+#if HAVE_LIBCAPSICUM
+#include <libcapsicum.h>
+#include <libcapsicum_dns.h>
+#include <libcapsicum_service.h>
+#endif
#ifdef IPSEC
#include <netipsec/ipsec.h>
@@ -157,7 +163,8 @@
struct sockaddr_in whereto; /* who to ping */
int datalen = DEFDATALEN;
int maxpayload;
-int s; /* socket file descriptor */
+int ssend; /* send socket file descriptor */
+int srecv; /* receive socket file descriptor */
u_char outpackhdr[IP_MAXPACKET], *outpack;
char BBELL = '\a'; /* characters written for MISSED and AUDIBLE */
char BSPACE = '\b'; /* characters written for flood */
@@ -197,8 +204,15 @@
volatile sig_atomic_t finish_up; /* nonzero if we've been told to finish up */
volatile sig_atomic_t siginfo_p;
+#if HAVE_LIBCAPSICUM
+static cap_channel_t *capdns;
+#endif
+
static void fill(char *, char *);
static u_short in_cksum(u_short *, int);
+#if HAVE_LIBCAPSICUM
+static cap_channel_t *capdns_setup(void);
+#endif
static void check_status(void);
static void finish(void) __dead2;
static void pinger(void);
@@ -233,8 +247,8 @@
struct sockaddr_in *to;
double t;
u_long alarmtimeout, ultmp;
- int almost_done, ch, df, hold, i, icmp_len, mib[4], preload, sockerrno,
- tos, ttl;
+ int almost_done, ch, df, hold, i, icmp_len, mib[4], preload,
+ ssend_errno, srecv_errno, tos, ttl;
char ctrl[CMSG_SPACE(sizeof(struct timeval))];
char hnamebuf[MAXHOSTNAMELEN], snamebuf[MAXHOSTNAMELEN];
#ifdef IP_OPTIONS
@@ -246,14 +260,26 @@
#ifdef IPSEC_POLICY_IPSEC
policy_in = policy_out = NULL;
#endif
+ cap_rights_t rights;
+ bool cansandbox = false;
/*
* Do the stuff that we need root priv's for *first*, and
* then drop our setuid bit. Save error reporting for
* after arg parsing.
+ *
+ * Historicaly ping was using one socket 's' for sending and for
+ * receiving. After capsicum(4) related changes we use two
+ * sockets. It was done for special ping use case - when user
+ * issue ping on multicast or broadcast address replies come
+ * from different addresses, not from the address we
+ * connect(2)'ed to, and send socket do not receive those
+ * packets.
*/
- s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
- sockerrno = errno;
+ ssend = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
+ ssend_errno = errno;
+ srecv = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
+ srecv_errno = errno;
if (setuid(getuid()) != 0)
err(EX_NOPERM, "setuid() failed");
@@ -527,6 +553,9 @@
if (options & F_PINGFILLED) {
fill((char *)datap, payload);
}
+#if HAVE_LIBCAPSICUM
+ capdns = capdns_setup();
+#endif
if (source) {
bzero((char *)&sock_in, sizeof(sock_in));
sock_in.sin_family = AF_INET;
@@ -533,7 +562,13 @@
if (inet_aton(source, &sock_in.sin_addr) != 0) {
shostname = source;
} else {
- hp = gethostbyname2(source, AF_INET);
+#if HAVE_LIBCAPSICUM
+ if (capdns != NULL)
+ hp = cap_gethostbyname2(capdns, source,
+ AF_INET);
+ else
+#endif
+ hp = gethostbyname2(source, AF_INET);
if (!hp)
errx(EX_NOHOST, "cannot resolve %s: %s",
source, hstrerror(h_errno));
@@ -549,7 +584,7 @@
snamebuf[sizeof(snamebuf) - 1] = '\0';
shostname = snamebuf;
}
- if (bind(s, (struct sockaddr *)&sock_in, sizeof sock_in) == -1)
+ if (bind(ssend, (struct sockaddr *)&sock_in, sizeof sock_in) == -1)
err(1, "bind");
}
@@ -560,7 +595,12 @@
if (inet_aton(target, &to->sin_addr) != 0) {
hostname = target;
} else {
- hp = gethostbyname2(target, AF_INET);
+#if HAVE_LIBCAPSICUM
+ if (capdns != NULL)
+ hp = cap_gethostbyname2(capdns, target, AF_INET);
+ else
+#endif
+ hp = gethostbyname2(target, AF_INET);
if (!hp)
errx(EX_NOHOST, "cannot resolve %s: %s",
target, hstrerror(h_errno));
@@ -573,6 +613,29 @@
hostname = hnamebuf;
}
+#if HAVE_LIBCAPSICUM
+ /* From now on we will use only reverse DNS lookups */
+ if (capdns != NULL) {
+ const char *types[1];
+ types[0] = "ADDR";
+ if (cap_dns_type_limit(capdns, types, 1) < 0)
+ err(1, "unable to limit access to system.dns service");
+ }
+#endif
+
+ if (ssend < 0) {
+ errno = ssend_errno;
+ err(EX_OSERR, "ssend socket");
+ }
+
+ if (srecv < 0) {
+ errno = srecv_errno;
+ err(EX_OSERR, "srecv socket");
+ }
+
+ if (connect(ssend, (struct sockaddr *)&whereto, sizeof(whereto)) != 0)
+ err(1, "connect");
+
if (options & F_FLOOD && options & F_INTERVAL)
errx(EX_USAGE, "-f and -i: incompatible options");
@@ -593,16 +656,15 @@
ident = getpid() & 0xFFFF;
- if (s < 0) {
- errno = sockerrno;
- err(EX_OSERR, "socket");
- }
hold = 1;
- if (options & F_SO_DEBUG)
- (void)setsockopt(s, SOL_SOCKET, SO_DEBUG, (char *)&hold,
+ if (options & F_SO_DEBUG) {
+ (void)setsockopt(ssend, SOL_SOCKET, SO_DEBUG, (char *)&hold,
sizeof(hold));
+ (void)setsockopt(srecv, SOL_SOCKET, SO_DEBUG, (char *)&hold,
+ sizeof(hold));
+ }
if (options & F_SO_DONTROUTE)
- (void)setsockopt(s, SOL_SOCKET, SO_DONTROUTE, (char *)&hold,
+ (void)setsockopt(ssend, SOL_SOCKET, SO_DONTROUTE, (char *)&hold,
sizeof(hold));
#ifdef IPSEC
#ifdef IPSEC_POLICY_IPSEC
@@ -612,7 +674,7 @@
buf = ipsec_set_policy(policy_in, strlen(policy_in));
if (buf == NULL)
errx(EX_CONFIG, "%s", ipsec_strerror());
- if (setsockopt(s, IPPROTO_IP, IP_IPSEC_POLICY,
+ if (setsockopt(srecv, IPPROTO_IP, IP_IPSEC_POLICY,
buf, ipsec_get_policylen(buf)) < 0)
err(EX_CONFIG,
"ipsec policy cannot be configured");
@@ -623,7 +685,7 @@
buf = ipsec_set_policy(policy_out, strlen(policy_out));
if (buf == NULL)
errx(EX_CONFIG, "%s", ipsec_strerror());
- if (setsockopt(s, IPPROTO_IP, IP_IPSEC_POLICY,
+ if (setsockopt(ssend, IPPROTO_IP, IP_IPSEC_POLICY,
buf, ipsec_get_policylen(buf)) < 0)
err(EX_CONFIG,
"ipsec policy cannot be configured");
@@ -644,7 +706,7 @@
if (sysctl(mib, 4, &ttl, &sz, NULL, 0) == -1)
err(1, "sysctl(net.inet.ip.ttl)");
}
- setsockopt(s, IPPROTO_IP, IP_HDRINCL, &hold, sizeof(hold));
+ setsockopt(ssend, IPPROTO_IP, IP_HDRINCL, &hold, sizeof(hold));
ip->ip_v = IPVERSION;
ip->ip_hl = sizeof(struct ip) >> 2;
ip->ip_tos = tos;
@@ -655,6 +717,33 @@
ip->ip_src.s_addr = source ? sock_in.sin_addr.s_addr : INADDR_ANY;
ip->ip_dst = to->sin_addr;
}
+
+ if (options & F_NUMERIC)
+ cansandbox = true;
+#if HAVE_LIBCAPSICUM
+ else if (capdns != NULL)
+ cansandbox = true;
+#endif
+
+ /*
+ * Here we enter capapility mode. Further down access to global
+ * namespaces (e.g filesystem) is restricted (see capsicum(4)).
+ * We must connect(2) our socket before this point.
+ */
+ if (cansandbox == true && cap_enter() < 0 && errno != ENOSYS)
+ err(1, "cap_enter");
+
+ if (cap_sandboxed())
+ fprintf(stderr, "capability mode sandbox enabled\n");
+
+ cap_rights_init(&rights, CAP_RECV, CAP_EVENT, CAP_SETSOCKOPT);
+ if (cap_rights_limit(srecv, &rights) < 0 && errno != ENOSYS)
+ err(1, "cap_rights_limit srecv");
+
+ cap_rights_init(&rights, CAP_SEND, CAP_SETSOCKOPT);
+ if (cap_rights_limit(ssend, &rights) < 0 && errno != ENOSYS)
+ err(1, "cap_rights_limit ssend");
+
/* record route option */
if (options & F_RROUTE) {
#ifdef IP_OPTIONS
@@ -663,7 +752,7 @@
rspace[IPOPT_OLEN] = sizeof(rspace) - 1;
rspace[IPOPT_OFFSET] = IPOPT_MINOFF;
rspace[sizeof(rspace) - 1] = IPOPT_EOL;
- if (setsockopt(s, IPPROTO_IP, IP_OPTIONS, rspace,
+ if (setsockopt(ssend, IPPROTO_IP, IP_OPTIONS, rspace,
sizeof(rspace)) < 0)
err(EX_OSERR, "setsockopt IP_OPTIONS");
#else
@@ -673,25 +762,25 @@
}
if (options & F_TTL) {
- if (setsockopt(s, IPPROTO_IP, IP_TTL, &ttl,
+ if (setsockopt(ssend, IPPROTO_IP, IP_TTL, &ttl,
sizeof(ttl)) < 0) {
err(EX_OSERR, "setsockopt IP_TTL");
}
}
if (options & F_NOLOOP) {
- if (setsockopt(s, IPPROTO_IP, IP_MULTICAST_LOOP, &loop,
+ if (setsockopt(ssend, IPPROTO_IP, IP_MULTICAST_LOOP, &loop,
sizeof(loop)) < 0) {
err(EX_OSERR, "setsockopt IP_MULTICAST_LOOP");
}
}
if (options & F_MTTL) {
- if (setsockopt(s, IPPROTO_IP, IP_MULTICAST_TTL, &mttl,
+ if (setsockopt(ssend, IPPROTO_IP, IP_MULTICAST_TTL, &mttl,
sizeof(mttl)) < 0) {
err(EX_OSERR, "setsockopt IP_MULTICAST_TTL");
}
}
if (options & F_MIF) {
- if (setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, &ifaddr,
+ if (setsockopt(ssend, IPPROTO_IP, IP_MULTICAST_IF, &ifaddr,
sizeof(ifaddr)) < 0) {
err(EX_OSERR, "setsockopt IP_MULTICAST_IF");
}
@@ -698,7 +787,7 @@
}
#ifdef SO_TIMESTAMP
{ int on = 1;
- if (setsockopt(s, SOL_SOCKET, SO_TIMESTAMP, &on, sizeof(on)) < 0)
+ if (setsockopt(srecv, SOL_SOCKET, SO_TIMESTAMP, &on, sizeof(on)) < 0)
err(EX_OSERR, "setsockopt SO_TIMESTAMP");
}
#endif
@@ -733,11 +822,19 @@
* as well.
*/
hold = IP_MAXPACKET + 128;
- (void)setsockopt(s, SOL_SOCKET, SO_RCVBUF, (char *)&hold,
+ (void)setsockopt(srecv, SOL_SOCKET, SO_RCVBUF, (char *)&hold,
sizeof(hold));
+ /* CAP_SETSOCKOPT removed */
+ cap_rights_init(&rights, CAP_RECV, CAP_EVENT);
+ if (cap_rights_limit(srecv, &rights) < 0 && errno != ENOSYS)
+ err(1, "cap_rights_limit srecv setsockopt");
if (uid == 0)
- (void)setsockopt(s, SOL_SOCKET, SO_SNDBUF, (char *)&hold,
+ (void)setsockopt(ssend, SOL_SOCKET, SO_SNDBUF, (char *)&hold,
sizeof(hold));
+ /* CAP_SETSOCKOPT removed */
+ cap_rights_init(&rights, CAP_SEND);
+ if (cap_rights_limit(ssend, &rights) < 0 && errno != ENOSYS)
+ err(1, "cap_rights_limit ssend setsockopt");
if (to->sin_family == AF_INET) {
(void)printf("PING %s (%s)", hostname,
@@ -817,10 +914,10 @@
int cc, n;
check_status();
- if ((unsigned)s >= FD_SETSIZE)
+ if ((unsigned)srecv >= FD_SETSIZE)
errx(EX_OSERR, "descriptor too large");
FD_ZERO(&rfds);
- FD_SET(s, &rfds);
+ FD_SET(srecv, &rfds);
(void)gettimeofday(&now, NULL);
timeout.tv_sec = last.tv_sec + intvl.tv_sec - now.tv_sec;
timeout.tv_usec = last.tv_usec + intvl.tv_usec - now.tv_usec;
@@ -834,7 +931,7 @@
}
if (timeout.tv_sec < 0)
timerclear(&timeout);
- n = select(s + 1, &rfds, NULL, NULL, &timeout);
+ n = select(srecv + 1, &rfds, NULL, NULL, &timeout);
if (n < 0)
continue; /* Must be EINTR. */
if (n == 1) {
@@ -845,7 +942,7 @@
msg.msg_controllen = sizeof(ctrl);
#endif
msg.msg_namelen = sizeof(from);
- if ((cc = recvmsg(s, &msg, 0)) < 0) {
+ if ((cc = recvmsg(srecv, &msg, 0)) < 0) {
if (errno == EINTR)
continue;
warn("recvmsg");
@@ -981,9 +1078,7 @@
ip->ip_sum = in_cksum((u_short *)outpackhdr, cc);
packet = outpackhdr;
}
- i = sendto(s, (char *)packet, cc, 0, (struct sockaddr *)&whereto,
- sizeof(whereto));
-
+ i = send(ssend, (char *)packet, cc, 0);
if (i < 0 || i != cc) {
if (i < 0) {
if (options & F_FLOOD && errno == ENOBUFS) {
@@ -1604,12 +1699,21 @@
struct hostent *hp;
static char buf[16 + 3 + MAXHOSTNAMELEN];
- if ((options & F_NUMERIC) ||
- !(hp = gethostbyaddr((char *)&ina, 4, AF_INET)))
+ if (options & F_NUMERIC)
return inet_ntoa(ina);
+
+#if HAVE_LIBCAPSICUM
+ if (capdns != NULL)
+ hp = cap_gethostbyaddr(capdns, (char *)&ina, 4, AF_INET);
else
- (void)snprintf(buf, sizeof(buf), "%s (%s)", hp->h_name,
- inet_ntoa(ina));
+#endif
+ hp = gethostbyaddr((char *)&ina, 4, AF_INET);
+
+ if (hp == NULL)
+ return inet_ntoa(ina);
+
+ (void)snprintf(buf, sizeof(buf), "%s (%s)", hp->h_name,
+ inet_ntoa(ina));
return(buf);
}
@@ -1682,6 +1786,36 @@
}
}
+#if HAVE_LIBCAPSICUM
+static cap_channel_t *
+capdns_setup(void)
+{
+ cap_channel_t *capcas, *capdnsloc;
+ const char *types[2];
+ int families[1];
+
+ capcas = cap_init();
+ if (capcas == NULL) {
+ warn("unable to contact casperd");
+ return (NULL);
+ }
+ capdnsloc = cap_service_open(capcas, "system.dns");
+ /* Casper capability no longer needed. */
+ cap_close(capcas);
+ if (capdnsloc == NULL)
+ err(1, "unable to open system.dns service");
+ types[0] = "NAME";
+ types[1] = "ADDR";
+ if (cap_dns_type_limit(capdnsloc, types, 2) < 0)
+ err(1, "unable to limit access to system.dns service");
+ families[0] = AF_INET;
+ if (cap_dns_family_limit(capdnsloc, families, 1) < 0)
+ err(1, "unable to limit access to system.dns service");
+
+ return (capdnsloc);
+}
+#endif /* HAVE_LIBCAPSICUM */
+
#if defined(IPSEC) && defined(IPSEC_POLICY_IPSEC)
#define SECOPT " [-P policy]"
#else
--Y/WcH0a6A93yCHGr--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140113172808.GA10345>