Subject: Re: core file name format: diffs
To: None <tech-kern@netbsd.org>
From: der Mouse <mouse@Rodents.Montreal.QC.CA>
List: tech-kern
Date: 09/21/1999 12:59:04
>>> The basename of the per-process corename format have to be either
>>> 'core' or end with '.core' [...]
>> I'm not at all sure I like this; I think [sysctl] calls made by root
>> should be exempt from this,
> Easy to change

True enough.  (It also fits in with the "don't protect root from
itself" philosophy that applies to things like unlink(2) on directories
and "dd if=/dev/zero of=/dev/kmem".)

>> and personally would argue an option should exist to disable the
>> check entirely.
> I'm not sure adding yet another kernel option or sysctl variable for
> this is the rigth thing to do.

I'm not sure either.  When I wrote "option", I had no particular
meaning in mind, merely that some way should be there (some relatively
convenient way; one can always argue that patching the source code is
an available way to get this behavior :-).

> We should really change the semantic of securelevel to a mask-based
> one.

Why?  What does a mask-style securelevel buy you over a bunch of
boolean kern.<foo> sysctls?  Nothing I can see, except that they can be
stored more compactly in the kernel - and that can be addressed with a
new sysctl type "boolean", with a sysctl_boolean() implementation that
bashes on individual bits in a mask.

In fact, you could provide both, kern.security.all for the mask so you
can load all the bits at once and kern.security.lkm for LKMs,
kern.security.mount for mounting disks, etc, for twiddling them
individually.

I'm not quite sure what this would do to the "raise only" semantics of
the current securelevel.  Perhaps there'd be a kern.security.insecure
bit that, if clear, means you can set but not clear the others - and
cannot itself be set except by init.

>>> +.It Li p	The PID of the process
>>> +.It Li t	The process's creation date

>> It would be good to state how %p and %t are rendered into text
>> (primarily for completeness).

> What about this ? :
> .It Li p        The PID of the process (integer)
> .It Li t        The process's creation date (in seconds)

I'd make it more like

.It Li p        The PID of the process (in decimal)
.It Li t        The process's creation date (a la
.Xr time 3 ,
in decimal)

> The fifth level name is one of PROC_PID_LIMIT_TYPE_SOFT or
> PROC_PID_LIMIT_TYPE_HARD, to select respectively the soft or hard limit.
> Both are of type integer.                           

Yeah, that fixes that entirely.

>>> +	if ((indx = findname(string, "5th", bufpp, lp)) == -1)
>> Shouldn't that be "fifth", for consistency?
> When I wrote this I didn't know how to spell 'fifth' :)

Oh. :-)

> +void
> +p_sugid(p)
> +	struct proc *p;
> +{
> +	extern char defcorename[];
> +
> +	p->p_flag |= P_SUGID;
> +	/* reset what needs to be reset in plimit */
> +	if (p->p_limit->pl_corename != defcorename) {
> +		if (p->p_limit->p_refcnt > 1) {
> +			p->p_limit->p_refcnt--;
> +			p->p_limit = limcopy(p->p_limit);
> +		}
> +		p->p_limit->pl_corename = defcorename;
> +	}
> +}

I believe this will leak memory under some circumstances.

(1) Process sets its per-process corename.  Memory is allocated.
(2) Process execs a set-id executable.  p_sugid() is called.
(3) p_refcnt is 1 because nobody else shares this limit.
(4) pl_corename is summarily bashed, dropping the only pointer to the
     mallocked string.
(5) On exit, pl_corename equals defcorename, so it's not freed.

Given how limcopy() works, I think there is also a use-after-free bug:

(1) Process sets its per-process corename.  Memory is allocated.
(2) Process forks.  p_refcnt goes to 2.
(3) One of the two processes sets some other limit.  The limit
     structure splits into two, each with p_refcnt 1.  Both have
     identical pl_corename pointers (see limcopy()).
(4) One process exits.  Its p_refcnt is 1 and its pl_corename is not
     equal to defcorename, so pl_corename is freed.
(5a) The other process dumps core.  It follows its pl_corename pointer
     to find...whatever currently happens to be lying around there.
(5b) The other process exits.  Its p_refcnt is 1 and its pl_corename is
     not equal to defcorename, so pl_corename is freed...again.

					der Mouse

			       mouse@rodents.montreal.qc.ca
		     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B