Commit Graph

1979 Commits

Author SHA1 Message Date
Qualys Security Advisory
0847390b83 top: Always exit from sig_abexit().
The default action for SIGURG is to ignore the signal, for example.
This is very similar to the patch "ps/display.c: Always exit from
signal_handler()."
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
7fc7062127 top: Initialize struct sigaction in before(). 2018-05-19 07:32:34 +10:00
Qualys Security Advisory
d69966962c top: Fix snprintf() call in capsmk().
Replace "snprintf(msg, sizeof(pmt)" with "snprintf(msg, sizeof(msg)".
Luckily sizeof(pmt) == sizeof(msg), but fix it anyway.
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
131e5e2fe6 top: Prevent integer overflows in procs_refresh().
This is very similar to our patch against integer overflows in
readproctab*().
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
b6be15d3cb top: Initialize cp in task_show().
Found no problematic case at the moment, but this is a cheap
just-in-case.
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
713381b10d top: Protect macro parameters. 2018-05-19 07:32:34 +10:00
Qualys Security Advisory
d5b8ac7139 top: Check sortindx.
Every time sortindx is used as an index, or loaded from the
configuration file. Otherwise it leads to out-of-bounds reads and
arbitrary code execution.
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
bbe58d7e0a top: Check width and col.
Otherwise they may lead to out-of-bounds writes (snprintf() returns the
number of characters which would have been written if enough space had
been available).

Also, make sure buf is null-terminated after COLPLUSCH has been written.
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
97a989cbcd top: Check Rc.fixed_widest.
Otherwise it leads to crashes (for example, setting it to 2147483600 in
the configuration file segfaults top).
2018-05-19 07:32:34 +10:00
Qualys Security Advisory
cd8ba5670e top: Check graph_cpus, graph_mems, and summ_mscale.
Otherwise they lead to out-of-bounds reads and format-string bugs.

Since these variables are set/written to in several places (for example,
config_file()), check them in the only place where they are read/used.

Also, constify the static gtab[]s.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
bd91bbf7f1 top: Check i when setting Curwin in config_file().
Otherwise it leads to out-of-bounds reads (and maybe writes).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
b45c4803dd top: Do not default to the cwd in configs_read().
If the HOME environment variable is not set, or not absolute, use the
home directory returned by getpwuid(getuid()), if set and absolute
(instead of the cwd "."); otherwise, set p_home to NULL.

To keep the changes to a minimum, we rely on POSIX, which requires that
fopen() fails with ENOENT if the pathname (Rc_name) is an empty string.
This integrates well into the existing code, and makes write_rcfile()
work without a change.

Also, it makes the code in configs_read() easier to follow: only set and
use p_home if safe, and only set Rc_name if safe (in all the other cases
it is the empty string, and the fopen() calls fail). Plus, check for
snprintf() truncation (and if it happens, reset Rc_name to the empty
string).

Important note: top.1 should probably be updated, since it mentions the
fallback to the current working directory.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
e877d4f4c4 top: Fix double-fclose() in configs_read().
It happens only if RCFILE_NOERR is defined (it is not, by default).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
8476e6f4f9 pmap: Fix extended mode in one_proc().
Check the return value of sscanf() to make sure that all input items are
properly initialized.

In extended mode (x_option), one_proc() loads the values of start and
perms during one iteration of the while loop, and displays them during
one of the following iterations, but start and perms are variables local
to the while loop: move them out of the while loop, to the beginning of
the function.

Also, display a mapping only if cp2 is properly initialized; otherwise
(for example), mappings that do not belong to a selected range are
displayed, and with a NULL mapping name:

$ pmap -x -A 6FFF00000000,7FFF00000000 $$
...
Address           Kbytes     RSS   Dirty Mode  Mapping
000055b3d1e9b000       0     912       0  r-xp (null)
000055b3d2194000       0      16      16  r--p (null)
000055b3d2198000       0      36      36  rw-p (null)
...
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
6e4eade3d4 pmap: Plug mem- and fd-leak in one_proc(). 2018-05-19 07:32:22 +10:00
Qualys Security Advisory
32e57dbb88 pmap: Remove dead code in mapping_name().
If "cp = strrchr(mapbuf_b, '/')" then this function returns, and
otherwise there is no '/' in mapbuf_b and "cp = strchr(mapbuf_b, '/')"
is always false: remove this second block, since it is never entered.
Also, constify a few things in this function.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
c9241d85ac pmap: Harden one_proc().
Replace sprintf() with snprintf().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
737fbff0e6 pmap: Check sscanf() in discover_shm_minor().
Need at least 6 items ("inode" is unused).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
8c84870d83 pmap: Fix output format of VmFlags.
In the headers, the space was misplaced; for example, "pmap -XX $$"
outputs "VmFlagsMapping" (without a space). Use justify_print() instead
of printf().

There was also an extra space in the output, because vmflags[] (from the
"VmFlags:" line) always ends with a space. Overwriting this last space
with a null byte fixes this misalignment.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
62de3a2aa7 pmap: Prevent buffer overflow in sscanf().
vmflags[] is a 27*(2+1)=81 char array, but there are 30 flags now (not
27), and even with 27 flags this was an off-by-one overflow (the kernel
always outputs a flag with "%c%c ", so the last +1 is for a space, not
for the terminating null byte). Protect vmflags[] with a maximum field
width, as in the surrounding sscanf() calls.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
0d9d0a5206 pmap: Always check the return value of fgets().
Otherwise "the contents of the array remain unchanged and a null pointer
is returned" or "the array contents are indeterminate and a null pointer
is returned".
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
6f82fa2b04 pmap: Fix parsing error in config_read().
$ echo '[' > crash
$ pmap -C crash $$
Segmentation fault (core dumped)
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
c6e427d22e pmap: Prevent integer overflow in main().
Unlikely to ever happen, but just in case.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
db2f011895 pmap: Plug memory leak in range_arguments().
Also, simplify the code slightly (but functionally equivalent). Check
the return value of xstrdup() only once (yes, it can return NULL).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
14758ebc8f 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).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
94eebb03b5 proc/readproc.c: Prevent integer overflows in readproctab*().
If an integer overflow is about to be reached, call xalloc_err_handler()
(since it would have been caught by calloc() or reallocarray()) and then
exit(): these integer overflows are far from reachable, with the current
PID_MAX_LIMIT (2^22), so if they are there is something very wrong going
on. Note: we check the n_*alloc variables against INT_MAX even when they
are size_t because they are later stored as int in a struct proc_data_t.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
a013f6e020 proc/readproc.c: Fix double-free()s in readtask().
If QUICK_THREADS is not defined (it is not by default, but most
distributions enable it) and task_dir_missing is true (only on very old
kernels), then readtask() forgets to reset some of the struct proc_t t's
members, which later results in double-free()s in free_acquired().

For now, we simply synchronized the list of members to be reset with the
list of members freed in free_acquired().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
1539c13507 proc/readproc.c: Fix use-after-free in readproctab2().
The memset() in the PROC_LOOSE_TASKS loop leaves a struct proc_t
uninitialized (the one at data+n_used), which leads to a use-after-free.

ps calls readproctab2(), but only if !TF_loose_tasks, and this U-A-F is
triggered only if PROC_LOOSE_TASKS, so there seems to be no vulnerable
call in the procps package itself (other users of the libprocps may be
vulnerable, though).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
a4d82a2c2c proc/readproc.c: Harden openproc().
Replace xmalloc() with xcalloc().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
19849a45e0 proc/readproc.c: Harden get_proc_stats().
Replace sprintf() with snprintf().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
1b8ec51013 proc/readproc.c: Harden simple_nextpid().
Replace memcpy+strcpy with snprintf.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
263c0ebdd8 proc/readproc.c: Harden fill_cgroup_cvt().
Check the return value of snprintf(), otherwise dst may point
out-of-bounds when it reaches the end of the dst_buffer (the snprintf()
always returns 1 in that case, even if there is not enough space left),
and vMAX becomes negative and is passed to snprintf() as a size_t.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
6939463606 proc/readproc.c: Harden vectorize_this_str().
This detects an integer overflow of "strlen + 1", prevents an integer
overflow of "tot + adj + (2 * pSZ)", and avoids calling snprintf with a
string longer than INT_MAX. Truncate rather than fail, since the callers
do not expect a failure of this function.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
39dcf47bc8 proc/readproc.c: Harden read_unvectored().
1/ Prevent an out-of-bounds write if sz is 0.

2/ Limit sz to INT_MAX, because the return value is an int, not an
unsigned int (and because if INT_MAX is equal to SSIZE_MAX, man 2 read
says "If count is greater than SSIZE_MAX, the result is unspecified.")

3/ Always null-terminate dst (unless sz is 0), because a return value of
0 because of an open() error (for example) is indistinguishable from a
return value of 0 because of an empty file.

4/ Use an unsigned int for i (just like n), not an int.

5/ Check for snprintf() truncation.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
36c350f07c proc/readproc.c: Fix bugs and overflows in file2strvec().
Note: this is by far the most important and complex patch of the whole
series, please review it carefully; thank you very much!

For this patch, we decided to keep the original function's design and
skeleton, to avoid regressions and behavior changes, while fixing the
various bugs and overflows. And like the "Harden file2str()" patch, this
patch does not fail when about to overflow, but truncates instead: there
is information available about this process, so return it to the caller;
also, we used INT_MAX as a limit, but a lower limit could be used.

The easy changes:

- Replace sprintf() with snprintf() (and check for truncation).

- Replace "if (n == 0 && rbuf == 0)" with "if (n <= 0 && tot <= 0)" and
  do break instead of return: it simplifies the code (only one place to
  handle errors), and also guarantees that in the while loop either n or
  tot is > 0 (or both), even if n is reset to 0 when about to overflow.

- Remove the "if (n < 0)" block in the while loop: it is (and was) dead
  code, since we enter the while loop only if n >= 0.

- Rewrite the missing-null-terminator detection: in the original
  function, if the size of the file is a multiple of 2047, a null-
  terminator is appended even if the file is already null-terminated.

- Replace "if (n <= 0 && !end_of_file)" with "if (n < 0 || tot <= 0)":
  originally, it was equivalent to "if (n < 0)", but we added "tot <= 0"
  to handle the first break of the while loop, and to guarantee that in
  the rest of the function tot is > 0.

- Double-force ("belt and suspenders") the null-termination of rbuf:
  this is (and was) essential to the correctness of the function.

- Replace the final "while" loop with a "for" loop that behaves just
  like the preceding "for" loop: in the original function, this would
  lead to unexpected results (for example, if rbuf is |\0|A|\0|, this
  would return the array {"",NULL} but should return {"","A",NULL}; and
  if rbuf is |A|\0|B| (should never happen because rbuf should be null-
  terminated), this would make room for two pointers in ret, but would
  write three pointers to ret).

The hard changes:

- Prevent the integer overflow of tot in the while loop, but unlike
  file2str(), file2strvec() cannot let tot grow until it almost reaches
  INT_MAX, because it needs more space for the pointers: this is why we
  introduced ARG_LEN, which also guarantees that we can add "align" and
  a few sizeof(char*)s to tot without overflowing.

- Prevent the integer overflow of "tot + c + align": when INT_MAX is
  (almost) reached, we write the maximal safe amount of pointers to ret
  (ARG_LEN guarantees that there is always space for *ret = rbuf and the
  NULL terminator).
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
ccf8de0874 proc/readproc.c: Harden file2str().
1/ Replace sprintf() with snprintf() (and check for truncation).

2/ Prevent an integer overflow of ub->siz. The "tot_read--" is needed to
avoid an off-by-one overflow in "ub->buf[tot_read] = '\0'". It is safe
to decrement tot_read here, because we know that tot_read is equal to
ub->siz (and ub->siz is very large).

We believe that truncation is a better option than failure (implementing
failure instead should be as easy as replacing the "tot_read--" with
"tot_read = 0").
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
344f6d3c0e proc/readproc.c: Harden stat2proc().
1/ Use a "size_t num" instead of an "unsigned num" (also, do not store
the return value of sscanf() into num, it was unused anyway).

2/ Check the return value of strchr() and strrchr().

3/ Never jump over the terminating null byte with "S = tmp + 2".
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
20269a4129 proc/readproc.c: Harden supgrps_from_supgids().
1/ Prevent an integer overflow of t.

2/ Avoid an infinite loop if s contains characters other than comma,
spaces, +, -, and digits.

3/ Handle all possible return values of snprintf().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
6fb2bbaa0d proc/readproc.c: Harden status2proc().
1/ Do not read past the terminating null byte when hashing the name.

2/ S[x] is used as an index, but S is "char *S" (signed) and hence may
index the array out-of-bounds. Bit-mask S[x] with 127 (the array has 128
entries).

3/ Use a size_t for j, not an int (strlen() returns a size_t).

Notes:

- These are (mostly) theoretical problems, because the contents of
  /proc/PID/status are (mostly) trusted.

- The "name" member of the status_table_struct has 8 bytes, and
  "RssShmem" occupies exactly 8 bytes, which means that "name" is not
  null-terminated. This is fine right now, because status2proc() uses
  memcmp(), not strcmp(), but it is worth mentioning.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
27e45cf43b proc/readproc.c: Fix the unhex() function.
This function is unused (SIGNAL_STRING is defined by default, and if it
is not, procps does not compile -- for example, there is no "outbuf" in
help_pr_sig()) but fix it anyway. There are two bugs:

- it accepts non-hexadecimal characters (anything >= 0x30);

- "(c - (c>0x57) ? 0x57 : 0x30)" is always equal to 0x57.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
920b0ada70 proc/sysinfo.c: Ensure null-termination in getstat().
There was a "buff[BUFFSIZE-1] = 0;" but there may be garbage between
what is read() (less than BUFFSIZE-1 bytes) and this null byte. Reuse
the construct from the preceding getrunners().
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
015669383f ps/sortformat.c: Avoid "sep_loc + 1" when sep_loc is NULL. 2018-05-19 07:32:22 +10:00
Qualys Security Advisory
bb89dad867 ps/sortformat.c: Handle large width in aix_format_parse().
Unlikely to ever happen, since it would imply a very large string, but
better safe than sorry.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
cde22815af ps/sortformat.c: Catch negative width in format_parse().
The existing strspn() check guarantees that the string contains no '-'
but atoi() does not catch errors, especially not integer overflows.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
db00f54f4a ps/sortformat.c: Double-check chars in verify_short_sort().
To avoid an out-of-bounds access at checkoff[tmp]. The strspn() at the
beginning of the function protects against it already, but double-check
this in case of some future change.
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
afca7eee75 ps/display.c: Fix "move process-only flags to the process".
Use "proc |= (task & PROC_ONLY)" not "proc |= (task &~ PROC_ONLY)".
2018-05-19 07:32:22 +10:00
Qualys Security Advisory
2e4a594221 ps/display.c: Always exit from signal_handler().
Right now, "we _exit() anyway" is not always true: for example, the
default action for SIGURG is to ignore the signal, which means that
"kill(getpid(), signo);" does not terminate the process. Call _exit()
explicitly, in this case (rather than exit(), because the terminating
kill() calls do not call the functions registered with atexit() either).
2018-05-19 07:32:21 +10:00
Qualys Security Advisory
7dd7bdb09f ps/output.c: Always null-terminate outbuf in show_one_proc().
Before "strlen(outbuf)", if one of the pr_*() functions forgot to do it.
This prevents an out-of-bounds read in strlen(), and an out-of-bounds
write in "outbuf[sz] = '\n'". Another solution would be to replace
strlen() with strnlen(), but this is not used anywhere else in the
code-base and may not exist in all libc's.
2018-05-19 07:32:21 +10:00
Qualys Security Advisory
db25d0375a ps/output.c: Protect outbuf in various pr_*() functions.
pr_bsdstart(): Replace "strcpy(outbuf," with "snprintf(outbuf, COLWID,"
(which is used in all surrounding functions). (side note: the fact that
many pr_*() functions simply return "snprintf(outbuf, COLWID," justifies
the "amount" checks added to show_one_proc() by the "ps/output.c:
Replace strcpy() with snprintf() in show_one_proc()." patch)

pr_stime(): Check the return value of strftime() (in case of an error,
"the contents of the array are undefined").

help_pr_sig(): Handle the "len < 8" case, otherwise "sig+len-8" may
point outside the sig string.

pr_context(): Handle the empty string case, or else "outbuf[len-1]"
points outside outbuf.
2018-05-19 07:32:21 +10:00
Qualys Security Advisory
14e0247ea5 ps/output.c: Enforce a safe range for max_rightward.
Enforce a maximum max_rightward of OUTBUF_SIZE-1, because it is used in
constructs such as "snprintf(outbuf, max_rightward+1," (we could remove
the extra check at the beginning of forest_helper() now, but we decided
to leave it, as a precaution and reminder).

The minimum max_rightward check is not strictly needed, because it is
unsigned. However, we decided to add it anyway:

- most of the other variables are signed;

- make it visually clear that this case is properly handled;

- ideally, the minimum max_rightward should be 1, not 0 (to prevent
  integer overflows such as "max_rightward-1"), but this might change
  the behavior/output of ps, so we decided against it, for now.

Instead, we fixed the only function that overflows if max_rightward is
0. Also, enforce the same safe range for max_leftward, although it is
never used throughout the code-base.
2018-05-19 07:32:21 +10:00