From 14758ebc8f9d59369cbb15ea2a4ccbe56a658b23 Mon Sep 17 00:00:00 2001 From: Qualys Security Advisory Date: Thu, 1 Jan 1970 00:00:00 +0000 Subject: [PATCH] proc/readproc.c: Work around a design flaw in readeither(). 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). --- proc/readproc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/proc/readproc.c b/proc/readproc.c index f8561c1e..0f002315 100644 --- a/proc/readproc.c +++ b/proc/readproc.c @@ -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: