emo/procps
1
0
Fork 0
Code Issues Pull Requests Packages Releases Activity

proc/readproc.c: Work around a design flaw in readeither().

Browse Source 
readeither() caches (in new_p) a pointer to the proc_t of a task-group
leader, but readeither()'s callers can do pretty much anything with the
proc_t structure passed to and/or returned by this function. For
example, they can 1/ free it or 2/ recycle it (by passing it to
readeither() as x).

1/ leads to a use-after-free, and 2/ leads to unexpected behavior when
taskreader()/simple_readtask() is called with new_p equal to x (this is
not a theoretical flaw: 2/ happens in readproctab3() when want_task()
returns false and p is a group leader).

As a workaround, we keep a copy of new_p's first member (tid) in static
storage, and the next times we enter readeither() we check this "canary"
against the tid in new_p: if they differ, we reset new_p to NULL, which
forces the allocation of a new proc_t (the new "leader", or reference).

This always detects 2/ (because free_acquired(x,1) memsets x and hence
new_p); always detects 1/ if freed via free_acquired() and/or freeproc()
(very likely, otherwise memory may be leaked); probably detects 1/ even
if freed directly via free() (because the canary is the first member of
proc_t, likely to be overwritten by free()); but can not detect 1/ if
free() does not write to new_p's chunk at all.

Moreover, accessing new_p->tid to check the canary in case 1/ is itself
a use-after-free, so a better long-term solution should be implemented
at some point (we wanted to avoid intrusive and backward-incompatible
changes in this library function, hence this imperfect workaround).
This commit is contained in:
Qualys Security Advisory 1970-01-01 00:00:00 +00:00 committed by Craig Small
parent 94eebb03b5
commit 14758ebc8f
1 changed files with 9 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
proc/readproc.c
View File

@ -1383,13 +1383,17 @@ out:
proc_t* readeither (PROCTAB *restrict const PT, proc_t *restrict x) {
static proc_t skel_p; // skeleton proc_t, only uses tid + tgid
static proc_t *new_p; // for process/task transitions
static int canary;
char path[PROCPATHLEN];
proc_t *saved_x, *ret;
saved_x = x;
if (!x) x = xcalloc(sizeof(*x));
else free_acquired(x,1);
if (new_p) goto next_task;
if (new_p) {
if (new_p->tid != canary) new_p = NULL;
goto next_task;
}
next_proc:
new_p = NULL;
@ -1406,7 +1410,10 @@ next_task:
|| (!(ret = PT->taskreader(PT,new_p,x,path)))) { // simple_readtask
goto next_proc;
}
if (!new_p) new_p = ret;
if (!new_p) {
new_p = ret;
canary = new_p->tid;
}
return ret;
end_procs: