Date: Wed, 23 May 2001 01:19:38 +0200 From: Thomas Moestl <tmoestl@gmx.net> To: audit@freebsd.org Cc: Dima Dorfman <dima@unixfreak.org> Subject: Re: Patch to remove setgid bit from ipcs(1) Message-ID: <20010523011938.A8824@crow.dom2ip.de> In-Reply-To: <20010520231752.10A2B3E0B@bazooka.unixfreak.org>; from dima@unixfreak.org on Sun, May 20, 2001 at 04:17:51PM -0700 References: <20010520231752.10A2B3E0B@bazooka.unixfreak.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010523011938.A8824>