Date: Tue, 22 May 2001 20:29:24 -0700 From: Dima Dorfman <dima@unixfreak.org> To: Thomas Moestl <tmm@unixfreak.org>, audit@freebsd.org Subject: Re: Patch to remove setgid bit from ipcs(1) Message-ID: <20010523032924.3878C3E7D@bazooka.unixfreak.org> In-Reply-To: <20010523011938.A8824@crow.dom2ip.de>; from tmoestl@gmx.net on "Wed, 23 May 2001 01:19:38 %2B0200"
next in thread | previous in thread | raw e-mail | index | archive | help
Thomas Moestl <tmoestl@gmx.net> writes:
> On Sun, 2001/05/20 at 16:17:51 -0700, Dima Dorfman wrote:
> > Index: sys/kern/sysv_msg.c
> > ===================================================================
> > [...]
> > +SYSCTL_DECL(_kern_ipc);
> > +SYSCTL_STRUCT(_kern_ipc, OID_AUTO, msginfo, CTLFLAG_RD, &msginfo, msginfo,
> > + "System V message info");
> > +SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLFLAG_ANYBODY | CTLFLAG_RD,
> > + NULL, 0, sysctl_msqids, "", "Message queue IDs");
>
> CTLFLAG_ANYBODY is pointless here since this variable is not
> writable. Reading is allowed for all users (unless the handler
> implements special restrictions).
Indeed. I'll get rid of that.
>
> > Index: sys/kern/sysv_sem.c
> > ===================================================================
> > RCS file: /stl/src/FreeBSD/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/20 22:54:55
> > @@ -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,9 @@
> > 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_STRUCT(_kern_ipc, OID_AUTO, seminfo, CTLFLAG_RD, &seminfo, seminfo,
> "");
>
> 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?
>
> > Index: usr.bin/ipcs/ipcs.c
> > ===================================================================
> > [...]
> > static struct nlist symbols[] = {
> > @@ -63,16 +67,14 @@
> > #define X_SEMA 0
> > {"_seminfo"},
> > #define X_SEMINFO 1
> > - {"_semu"},
> > -#define X_SEMU 2
> > {"_msginfo"},
> > -#define X_MSGINFO 3
> > +#define X_MSGINFO 2
> > {"_msqids"},
> > -#define X_MSQIDS 4
> > +#define X_MSQIDS 3
> > {"_shminfo"},
> > -#define X_SHMINFO 5
> > +#define X_SHMINFO 4
> > {"_shmsegs"},
> > -#define X_SHMSEGS 6
> > +#define X_SHMSEGS 5
> > {NULL}
> > };
>
> I think you can safely remove the underscores, they are not needed any
> more.
Fair enough. I tried to leave alone as many things as I could, but I
guess this is the best time to do that.
>
> > void
> > +kget(idx, addr, size)
> > + int idx;
> > + void *addr;
> > + size_t size;
> > +{
> > [...]
> > + 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));
>
> I think the error message is redundant here, since you have passed an
> error string to kvm_open (which causes kvm library routines to print
> errors to stderr). Maybe printing the symbol name and just exiting
> should be sufficient. The error buffer seems not even to be filled
> when that string is set.
This is indeed the documented behavior, but this isn't how the libkvm
code behaves. If I stick a "kaddr = 0;" right above that block, I get
the expected output:
dima@spike% sudo ./ipcs -y
ipcs: msginfo: kvm_read: Bad address
I can confirm that it is indeed the errx that is printing that. Am I
missing something, or is libkvm doing the wrong thing here?
>
> > + } else {
> > + tsiz = size;
> > + if (sysctlbyname(sym2sysctl[idx], addr, &tsiz, NULL, 0)
> > + == -1)
> > + err(1, "sysctlbyname: %s", sym2sysctl[idx]);
> > + }
> > +}
>
> I think you should check that the size that we read is actually the
> one we expected (if (tsiz != size) barf();).
Good idea. I'll do that.
I'll send an updated patch when I/we figure out what to with regards
to the sysctl's and the libkvm stuff.
Thanks for the input,
Dima Dorfman
dima@unixfreak.org
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?20010523032924.3878C3E7D>