From owner-freebsd-audit Tue May 22 16:19:45 2001 Delivered-To: freebsd-audit@freebsd.org Received: from mail.gmx.net (pop.gmx.net [194.221.183.20]) by hub.freebsd.org (Postfix) with SMTP id 92D8337B422 for ; Tue, 22 May 2001 16:19:40 -0700 (PDT) (envelope-from tmoestl@gmx.net) Received: (qmail 13921 invoked by uid 0); 22 May 2001 23:19:38 -0000 Received: from pd900c63b.dip.t-dialin.net (HELO forge.local) (217.0.198.59) by mail.gmx.net (mail01) with SMTP; 22 May 2001 23:19:38 -0000 Received: from tmm by forge.local with local (Exim 3.20 #1) id 152LRC-0002Ti-00; Wed, 23 May 2001 01:19:38 +0200 Date: Wed, 23 May 2001 01:19:38 +0200 From: Thomas Moestl To: audit@freebsd.org Cc: Dima Dorfman Subject: Re: Patch to remove setgid bit from ipcs(1) Message-ID: <20010523011938.A8824@crow.dom2ip.de> Mail-Followup-To: Thomas Moestl , audit@freebsd.org, Dima Dorfman References: <20010520231752.10A2B3E0B@bazooka.unixfreak.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20010520231752.10A2B3E0B@bazooka.unixfreak.org>; from dima@unixfreak.org on Sun, May 20, 2001 at 04:17:51PM -0700 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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). > 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. > 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. > 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. > + } 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();). Apart from these few nits, nice work! It's really cool to have one less of those setgid beasts, especially a non-trivial one. - thomas To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message