Date: Wed, 23 May 2001 19:40:51 -0700 From: Dima Dorfman <dima@unixfreak.org> To: Thomas Moestl <tmm@freebsd.org>, audit@freebsd.org Subject: Re: Patch to remove setgid bit from ipcs(1) Message-ID: <20010524024051.A40E83E8E@bazooka.unixfreak.org> In-Reply-To: <20010523175221.C594@crow.dom2ip.de>; from tmm@freebsd.org on "Wed, 23 May 2001 17:52:21 %2B0200"
next in thread | previous in thread | raw e-mail | index | archive | help
Thomas Moestl <tmm@freebsd.org> writes:
> On Tue, 2001/05/22 at 20:35:07 -0700, Dima Dorfman wrote:
> > > Hmm, it seems to me that we export all members of this structure, so
> > > why export it again as a whole? While it might be better to pack things
> > > into a structure (which may however introduce problems when the
> > > structure changes), I'm not sure whether should really export this
> > > more than once just because of that.
> > > This also seems to apply to shared memory part.
> >
> > It's a compromise between exorting the entire structure or
> > complicating the userland part to do sysctl on all the different
> > fields and construct the structure itself. This was the simpler
> > approach, which is why I chose it. Do you think it's worth
> > complicating the kget() routine instead?
>
> Hmm. I don't really know. On any case, the separate sysctls need to
> stay because some of them are writable. Some message queue parameters
> might become tunable too some day, so if you decide to gather the
> information from separate variables, it might be better to export
> the message queue ones this way, too.
> I don't really like having duplicate data in the sysctl tree, OTOH it
> is a valid point that many applications would want to know all data
> at once and would thus need to do many more syscalls. Still... I don't
> like that ;)
I wasn't terribly concerned with performance; rather, I was concerned
with maintainability. And I didn't particuarly like it, either.
All that said, I think I might've found a happy medium. With a little
preprocessor magic in ipcs(1), I can make it maintainable without
having to export the data as a structure. Essentially, the new
function sysctlgatherstruct() takes a pointer to the structure to fill
in, the size, and an array of "vectors"; one for every field that
needs to be filled in. The latter has information on which sysctl to
use to get the information, the size, and where in the structure it
should be put.
One thing I don't really like is that the code in the userland is now
quite a bit bigger (and, since I tried to make sysctlgatherstruct
generic so it can potentially be used in another program, it's
probably a little bigger than it could've been), but I think it's
worth it to lose the setgid bit; once that's done, it is no longer
much of a threat to security.
I've attached an updated patch. It incorporates all of your
suggestions, including exporting the individual fields rather than the
structure as described above.
Comments? Suggestions?
Thanks,
Dima Dorfman
dima@unixfreak.org
Index: sys/kern/sysv_msg.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/sysv_msg.c,v
retrieving revision 1.30
diff -u -r1.30 sysv_msg.c
--- sys/kern/sysv_msg.c 2001/02/21 06:39:54 1.30
+++ sys/kern/sysv_msg.c 2001/05/24 02:37:42
@@ -1166,3 +1166,21 @@
p->p_retval[0] = msgsz;
return(0);
}
+
+static int
+sysctl_msqids(SYSCTL_HANDLER_ARGS)
+{
+
+ return (SYSCTL_OUT(req, msqids,
+ sizeof(struct msqid_ds) * msginfo.msgmni));
+}
+
+SYSCTL_DECL(_kern_ipc);
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgmax, CTLFLAG_RD, &msginfo.msgmax, 0, "");
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgmni, CTLFLAG_RD, &msginfo.msgmni, 0, "");
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgmnb, CTLFLAG_RD, &msginfo.msgmnb, 0, "");
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgtql, CTLFLAG_RD, &msginfo.msgtql, 0, "");
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgssz, CTLFLAG_RD, &msginfo.msgssz, 0, "");
+SYSCTL_INT(_kern_ipc, OID_AUTO, msgseg, CTLFLAG_RD, &msginfo.msgseg, 0, "")
+SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLFLAG_ANYBODY | CTLFLAG_RD,
+ NULL, 0, sysctl_msqids, "", "Message queue IDs");
Index: sys/kern/sysv_sem.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/sysv_sem.c,v
retrieving revision 1.32
diff -u -r1.32 sysv_sem.c
--- sys/kern/sysv_sem.c 2001/02/21 06:39:54 1.32
+++ sys/kern/sysv_sem.c 2001/05/24 02:37:42
@@ -28,6 +28,7 @@
static int sysvsem_modload __P((struct module *, int, void *));
static int semunload __P((void));
static void semexit_myhook __P((struct proc *p));
+static int sysctl_sema __P((SYSCTL_HANDLER_ARGS));
#ifndef _SYS_SYSPROTO_H_
struct __semctl_args;
@@ -148,6 +149,8 @@
SYSCTL_INT(_kern_ipc, OID_AUTO, semusz, CTLFLAG_RD, &seminfo.semusz, 0, "");
SYSCTL_INT(_kern_ipc, OID_AUTO, semvmx, CTLFLAG_RW, &seminfo.semvmx, 0, "");
SYSCTL_INT(_kern_ipc, OID_AUTO, semaem, CTLFLAG_RW, &seminfo.semaem, 0, "");
+SYSCTL_PROC(_kern_ipc, OID_AUTO, sema, CTLFLAG_RD | CTLFLAG_ANYBODY,
+ NULL, 0, sysctl_sema, "", "");
#if 0
RO seminfo.semmap /* SEMMAP unused */
@@ -1065,4 +1068,12 @@
#endif
suptr->un_proc = NULL;
*supptr = suptr->un_next;
+}
+
+static int
+sysctl_sema(SYSCTL_HANDLER_ARGS)
+{
+
+ return (SYSCTL_OUT(req, sema,
+ sizeof(struct semid_ds) * seminfo.semmni));
}
Index: sys/kern/sysv_shm.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/sysv_shm.c,v
retrieving revision 1.60
diff -u -r1.60 sysv_shm.c
--- sys/kern/sysv_shm.c 2001/05/23 23:38:05 1.60
+++ sys/kern/sysv_shm.c 2001/05/24 02:37:42
@@ -104,6 +104,7 @@
static int shmunload __P((void));
static void shmexit_myhook __P((struct proc *p));
static void shmfork_myhook __P((struct proc *p1, struct proc *p2));
+static int sysctl_shmsegs __P((SYSCTL_HANDLER_ARGS));
/*
* Tuneable values.
@@ -145,6 +146,8 @@
SYSCTL_INT(_kern_ipc, OID_AUTO, shmall, CTLFLAG_RW, &shminfo.shmall, 0, "");
SYSCTL_INT(_kern_ipc, OID_AUTO, shm_use_phys, CTLFLAG_RW,
&shm_use_phys, 0, "");
+SYSCTL_PROC(_kern_ipc, OID_AUTO, shmsegs, CTLFLAG_RD,
+ NULL, 0, sysctl_shmsegs, "", "");
static int
shm_find_segment_by_key(key)
@@ -740,6 +743,13 @@
shmexit_hook = NULL;
shmfork_hook = NULL;
return (0);
+}
+
+static int
+sysctl_shmsegs(SYSCTL_HANDLER_ARGS)
+{
+
+ return (SYSCTL_OUT(req, shmsegs, shmalloced * sizeof(shmsegs[0])));
}
static int
Index: usr.bin/ipcs/ipcs.1
===================================================================
RCS file: /home/ncvs/src/usr.bin/ipcs/ipcs.1,v
retrieving revision 1.11
diff -u -r1.11 ipcs.1
--- usr.bin/ipcs/ipcs.1 2000/12/14 11:49:46 1.11
+++ usr.bin/ipcs/ipcs.1 2001/05/24 02:37:43
@@ -37,7 +37,7 @@
.Nd report System V interprocess communication facilities status
.Sh SYNOPSIS
.Nm
-.Op Fl abcmopqstMQST
+.Op Fl abcmopqstMQSTy
.Op Fl C Ar system
.Op Fl N Ar core
.Sh DESCRIPTION
@@ -101,12 +101,16 @@
Extract the name list from the specified system instead of the
default
.Dq Pa /kernel .
+Implies
+.Fl y .
.It Fl M
Display system information about shared memory.
.It Fl N Ar core
Extract values associated with the name list from the specified
core instead of the default
.Dq Pa /dev/kmem .
+Implies
+.Fl y .
.It Fl Q
Display system information about messages queues.
.It Fl S
@@ -114,6 +118,19 @@
.It Fl T
Display system information about shared memory, message queues
and semaphores.
+.It Fl y
+Use the
+.Xr kvm 3
+interface instead of the
+.Xr sysctl 3
+interface to extract the required information.
+If
+.Nm
+is to operate on the running system,
+using
+.Xr kvm 3
+will require read privileges to
+.Pa /dev/kmem .
.El
.Pp
If none of the
Index: usr.bin/ipcs/ipcs.c
===================================================================
RCS file: /home/ncvs/src/usr.bin/ipcs/ipcs.c,v
retrieving revision 1.14
diff -u -r1.14 ipcs.c
--- usr.bin/ipcs/ipcs.c 2000/05/01 10:49:41 1.14
+++ usr.bin/ipcs/ipcs.c 2001/05/24 02:37:43
@@ -30,11 +30,14 @@
"$FreeBSD: src/usr.bin/ipcs/ipcs.c,v 1.14 2000/05/01 10:49:41 peter Exp $";
#endif /* not lint */
+#include <assert.h>
#include <err.h>
#include <fcntl.h>
#include <kvm.h>
#include <nlist.h>
+#include <limits.h>
#include <paths.h>
+#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
@@ -43,12 +46,21 @@
#include <sys/param.h>
#include <sys/time.h>
#include <sys/proc.h>
+#include <sys/sysctl.h>
#define _KERNEL
#include <sys/ipc.h>
#include <sys/sem.h>
#include <sys/shm.h>
#include <sys/msg.h>
+/* SysCtlGatherStruct structure. */
+struct scgs_vector {
+ char *sysctl;
+ off_t offset;
+ size_t size;
+};
+
+int use_sysctl = 1;
struct semid_ds *sema;
struct seminfo seminfo;
struct msginfo msginfo;
@@ -56,26 +68,66 @@
struct shminfo shminfo;
struct shmid_ds *shmsegs;
+void sysctlgatherstruct __P((void *addr, size_t size,
+ struct scgs_vector *vec));
+void kget __P((int idx, void *addr, size_t size));
void usage __P((void));
static struct nlist symbols[] = {
- {"_sema"},
+ {"sema"},
#define X_SEMA 0
- {"_seminfo"},
+ {"seminfo"},
#define X_SEMINFO 1
- {"_semu"},
-#define X_SEMU 2
- {"_msginfo"},
-#define X_MSGINFO 3
- {"_msqids"},
-#define X_MSQIDS 4
- {"_shminfo"},
-#define X_SHMINFO 5
- {"_shmsegs"},
-#define X_SHMSEGS 6
+ {"msginfo"},
+#define X_MSGINFO 2
+ {"msqids"},
+#define X_MSQIDS 3
+ {"shminfo"},
+#define X_SHMINFO 4
+ {"shmsegs"},
+#define X_SHMSEGS 5
{NULL}
};
+#define SHMINFO_XVEC \
+X(shmmax, sizeof(int)) \
+X(shmmin, sizeof(int)) \
+X(shmmni, sizeof(int)) \
+X(shmseg, sizeof(int)) \
+X(shmall, sizeof(int))
+
+#define SEMINFO_XVEC \
+X(semmap, sizeof(int)) \
+X(semmni, sizeof(int)) \
+X(semmns, sizeof(int)) \
+X(semmnu, sizeof(int)) \
+X(semmsl, sizeof(int)) \
+X(semopm, sizeof(int)) \
+X(semume, sizeof(int)) \
+X(semusz, sizeof(int)) \
+X(semvmx, sizeof(int)) \
+X(semaem, sizeof(int))
+
+#define MSGINFO_XVEC \
+X(msgmax, sizeof(int)) \
+X(msgmni, sizeof(int)) \
+X(msgmnb, sizeof(int)) \
+X(msgtql, sizeof(int)) \
+X(msgssz, sizeof(int)) \
+X(msgseg, sizeof(int))
+
+#define X(a, b) { "kern.ipc." #a, offsetof(TYPEC, a), (b) },
+#define TYPEC struct shminfo
+struct scgs_vector shminfo_scgsv[] = { SHMINFO_XVEC { NULL } };
+#undef TYPEC
+#define TYPEC struct seminfo
+struct scgs_vector seminfo_scgsv[] = { SEMINFO_XVEC { NULL } };
+#undef TYPEC
+#define TYPEC struct msginfo
+struct scgs_vector msginfo_scgsv[] = { MSGINFO_XVEC { NULL } };
+#undef TYPEC
+#undef X
+
static kvm_t *kd;
char *
@@ -135,9 +187,10 @@
int display = SHMINFO | MSGINFO | SEMINFO;
int option = 0;
char *core = NULL, *namelist = NULL;
+ char kvmoferr[_POSIX2_LINE_MAX]; /* Error buf for kvm_openfiles. */
int i;
- while ((i = getopt(argc, argv, "MmQqSsabC:cN:optT")) != -1)
+ while ((i = getopt(argc, argv, "MmQqSsabC:cN:optTy")) != -1)
switch (i) {
case 'M':
display = SHMTOTAL;
@@ -184,39 +237,44 @@
case 't':
option |= TIME;
break;
+ case 'y':
+ use_sysctl = 0;
+ break;
default:
usage();
}
/*
- * Discard setgid privileges if not the running kernel so that bad
- * guys can't print interesting stuff from kernel memory.
+ * If paths to the exec file or core file were specified, we
+ * aren't operating on the running kernel, so we can't use
+ * sysctl.
*/
if (namelist != NULL || core != NULL)
- setgid(getgid());
-
- if ((kd = kvm_open(namelist, core, NULL, O_RDONLY, "ipcs")) == NULL)
- exit(1);
+ use_sysctl = 0;
- switch (kvm_nlist(kd, symbols)) {
- case 0:
- break;
- case -1:
- errx(1, "unable to read kernel symbol table");
- default:
+ if (!use_sysctl) {
+ kd = kvm_openfiles(namelist, core, NULL, O_RDONLY, kvmoferr);
+ if (kd == NULL)
+ errx(1, "kvm_openfiles: %s", kvmoferr);
+ switch (kvm_nlist(kd, symbols)) {
+ case 0:
+ break;
+ case -1:
+ errx(1, "unable to read kernel symbol table");
+ default:
#ifdef notdef /* they'll be told more civilly later */
- warnx("nlist failed");
- for (i = 0; symbols[i].n_name != NULL; i++)
- if (symbols[i].n_value == 0)
- warnx("symbol %s not found",
- symbols[i].n_name);
- break;
+ warnx("nlist failed");
+ for (i = 0; symbols[i].n_name != NULL; i++)
+ if (symbols[i].n_value == 0)
+ warnx("symbol %s not found",
+ symbols[i].n_name);
+ break;
#endif
+ }
}
- if ((display & (MSGINFO | MSGTOTAL)) &&
- kvm_read(kd, symbols[X_MSGINFO].n_value, &msginfo, sizeof(msginfo))== sizeof(msginfo)) {
-
+ kget(X_MSGINFO, &msginfo, sizeof(msginfo));
+ if ((display & (MSGINFO | MSGTOTAL))) {
if (display & MSGTOTAL) {
printf("msginfo:\n");
printf("\tmsgmax: %6d\t(max characters in a message)\n",
@@ -234,10 +292,12 @@
}
if (display & MSGINFO) {
struct msqid_ds *xmsqids;
+ size_t xmsqids_len;
+
- kvm_read(kd, symbols[X_MSQIDS].n_value, &msqids, sizeof(msqids));
- xmsqids = malloc(sizeof(struct msqid_ds) * msginfo.msgmni);
- kvm_read(kd, (u_long) msqids, xmsqids, sizeof(struct msqid_ds) * msginfo.msgmni);
+ xmsqids_len = sizeof(struct msqid_ds) * msginfo.msgmni;
+ xmsqids = malloc(xmsqids_len);
+ kget(X_MSQIDS, xmsqids, xmsqids_len);
printf("Message Queues:\n");
printf("T ID KEY MODE OWNER GROUP");
@@ -304,8 +364,9 @@
fprintf(stderr,
"SVID messages facility not configured in the system\n");
}
- if ((display & (SHMINFO | SHMTOTAL)) &&
- kvm_read(kd, symbols[X_SHMINFO].n_value, &shminfo, sizeof(shminfo))) {
+
+ kget(X_SHMINFO, &shminfo, sizeof(shminfo));
+ if ((display & (SHMINFO | SHMTOTAL))) {
if (display & SHMTOTAL) {
printf("shminfo:\n");
printf("\tshmmax: %7d\t(max shared memory segment size)\n",
@@ -321,11 +382,11 @@
}
if (display & SHMINFO) {
struct shmid_ds *xshmids;
+ size_t xshmids_len;
- kvm_read(kd, symbols[X_SHMSEGS].n_value, &shmsegs, sizeof(shmsegs));
- xshmids = malloc(sizeof(struct shmid_ds) * shminfo.shmmni);
- kvm_read(kd, (u_long) shmsegs, xshmids, sizeof(struct shmid_ds) *
- shminfo.shmmni);
+ xshmids_len = sizeof(struct shmid_ds) * shminfo.shmmni;
+ xshmids = malloc(xshmids_len);
+ kget(X_SHMSEGS, xshmids, xshmids_len);
printf("Shared Memory:\n");
printf("T ID KEY MODE OWNER GROUP");
@@ -391,9 +452,11 @@
fprintf(stderr,
"SVID shared memory facility not configured in the system\n");
}
- if ((display & (SEMINFO | SEMTOTAL)) &&
- kvm_read(kd, symbols[X_SEMINFO].n_value, &seminfo, sizeof(seminfo))) {
+
+ kget(X_SEMINFO, &seminfo, sizeof(seminfo));
+ if ((display & (SEMINFO | SEMTOTAL))) {
struct semid_ds *xsema;
+ size_t xsema_len;
if (display & SEMTOTAL) {
printf("seminfo:\n");
@@ -419,9 +482,9 @@
seminfo.semaem);
}
if (display & SEMINFO) {
- kvm_read(kd, symbols[X_SEMA].n_value, &sema, sizeof(sema));
- xsema = malloc(sizeof(struct semid_ds) * seminfo.semmni);
- kvm_read(kd, (u_long) sema, xsema, sizeof(struct semid_ds) * seminfo.semmni);
+ xsema_len = sizeof(struct semid_ds) * seminfo.semmni;
+ xsema = malloc(xsema_len);
+ kget(X_SEMA, xsema, xsema_len);
printf("Semaphores:\n");
printf("T ID KEY MODE OWNER GROUP");
@@ -471,16 +534,122 @@
if (display & (SEMINFO | SEMTOTAL)) {
fprintf(stderr, "SVID semaphores facility not configured in the system\n");
}
- kvm_close(kd);
+ if (!use_sysctl)
+ kvm_close(kd);
exit(0);
}
void
+sysctlgatherstruct(addr, size, vecarr)
+ void *addr;
+ size_t size;
+ struct scgs_vector *vecarr;
+{
+ struct scgs_vector *xp;
+ int tsiz, rv;
+
+ for (xp = vecarr; xp->sysctl != NULL; xp++) {
+ assert(xp->offset <= size);
+ tsiz = xp->size;
+ rv = sysctlbyname(xp->sysctl, addr + xp->offset, &tsiz,
+ NULL, 0);
+ if (rv == -1)
+ errx(1, "sysctlbyname: %s", xp->sysctl);
+ if (tsiz != xp->size)
+ errx(1, "%s size mismatch (expected %d, got %d)",
+ xp->sysctl, xp->size, tsiz);
+ }
+}
+
+void
+kget(idx, addr, size)
+ int idx;
+ void *addr;
+ size_t size;
+{
+ char *symn; /* symbol name */
+ int rv, tsiz;
+ unsigned long kaddr;
+ char *sym2sysctl[] = { /* symbol to sysctl name table */
+ "kern.ipc.sema",
+ "kern.ipc.seminfo",
+ "kern.ipc.msginfo",
+ "kern.ipc.msqids",
+ "kern.ipc.shminfo",
+ "kern.ipc.shmsegs" };
+
+ assert(idx <= sizeof(sym2sysctl) / sizeof(*sym2sysctl));
+ if (!use_sysctl) {
+ symn = symbols[idx].n_name;
+ if (*symn == '_')
+ symn++;
+ if (symbols[idx].n_type == 0 || symbols[idx].n_value == 0)
+ errx(1, "symbol %s undefined", symn);
+ /*
+ * For some symbols, the value we retreieve is
+ * actually a pointer; since we want the actual value,
+ * we have to manually dereference it.
+ */
+ switch (idx) {
+ case X_MSQIDS:
+ tsiz = sizeof(msqids);
+ rv = kvm_read(kd, symbols[idx].n_value,
+ &msqids, tsiz);
+ kaddr = (u_long)msqids;
+ break;
+ case X_SHMSEGS:
+ tsiz = sizeof(shmsegs);
+ rv = kvm_read(kd, symbols[idx].n_value,
+ &shmsegs, tsiz);
+ kaddr = (u_long)shmsegs;
+ break;
+ case X_SEMA:
+ tsiz = sizeof(sema);
+ rv = kvm_read(kd, symbols[idx].n_value,
+ &sema, tsiz);
+ kaddr = (u_long)sema;
+ break;
+ default:
+ rv = tsiz = 0;
+ kaddr = symbols[idx].n_value;
+ break;
+ }
+ if (rv != tsiz)
+ errx(1, "%s: %s", symn, kvm_geterr(kd));
+ if (kvm_read(kd, kaddr, addr, size) != size)
+ errx(1, "%s: %s", symn, kvm_geterr(kd));
+ } else {
+ switch (idx) {
+ case X_SHMINFO:
+ sysctlgatherstruct(addr, size, shminfo_scgsv);
+ break;
+ case X_SEMINFO:
+ sysctlgatherstruct(addr, size, seminfo_scgsv);
+ break;
+ case X_MSGINFO:
+ sysctlgatherstruct(addr, size, msginfo_scgsv);
+ break;
+ default:
+ tsiz = size;
+ rv = sysctlbyname(sym2sysctl[idx], addr, &tsiz,
+ NULL, 0);
+ if (rv == -1)
+ err(1, "sysctlbyname: %s", sym2sysctl[idx]);
+ if (tsiz != size)
+ errx(1, "%s size mismatch "
+ "(expected %d, got %d)",
+ sym2sysctl[idx], size, tsiz);
+ break;
+ }
+ }
+}
+
+void
usage()
{
fprintf(stderr,
- "usage: ipcs [-abcmopqst] [-C corefile] [-N namelist]\n");
+ "usage: ipcs [-abcmopqsty] [-C corefile] [-N namelist]\n");
exit(1);
}
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010524024051.A40E83E8E>