Commit Graph

2256 Commits

Author SHA1 Message Date
Qualys Security Advisory
5f654f143a 0088-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".

Signed-off-by: Craig Small <csmall@enc.com.au>
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
45f81ef706 0087-pmap: Fix parsing error in config_read().
$ echo '[' > crash
$ pmap -C crash $$
Segmentation fault (core dumped)

Signed-off-by: Craig Small <csmall@enc.com.au>
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
bf409f92fd 0086-pmap: Prevent integer overflow in main().
Unlikely to ever happen, but just in case.

Signed-off-by: Craig Small <csmall@enc.com.au>
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
105ab093b8 0085-pmap.c: 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).

Adapted slightly to remove goto and leave the format of checks the same.
A lot of the fixes were already in newlib, caught by coverity

References:
 commit 25f655891f

Signed-off-by: Craig Small <csmall@enc.com.au>
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
a016a43b53 0027-skill: Prevent multiple overflows in ENLIST().
First problem: saved_argc was used to calculate the size of the array,
but saved_argc was never initialized. This triggers an immediate heap-
based buffer overflow:

$ skill -c0 -c0 -c0 -c0
Segmentation fault (core dumped)

Second problem: saved_argc was not the upper bound anyway, because one
argument can ENLIST() several times (for example, in parse_namespaces())
and overflow the array as well.

Third problem: integer overflow of the size of the array.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
3f75d105b9 0026-skill: Fix double-increment of pid_count.
No need to "pid_count++;" because "ENLIST(pid," does it already. Right
now this can trigger a heap-based buffer overflow.

Also, remove the unneeded "pid_count = 0;" (it is static, and
skillsnice_parse() is called only once; and the other *_count variables
are not initialized explicitly either).
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
18efff2a1f 0024-skill: Always NULL-terminate argv.
The memmove() itself does not move the NULL-terminator, because nargs is
decremented first. Copy how skill_sig_option() does it: decrement nargs
last, and remove the "if (nargs - i)" (we are in "while (i < nargs)").
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
fa7ea000ce 0023-skill: Fix getline() usage.
man getline: "If *lineptr is set to NULL and *n is set 0 before the
call, then getline() will allocate a buffer for storing the line. This
buffer should be freed by the user program even if getline() failed."
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
c704460c7d 0022-skill: Simplify the kill_main() loop.
Right now the "loop=0; break;" is never reached.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
f69d54f05b 0021-pwdx: Fix a misleading comment.
It sounds like an off-by-one, but the code itself is correct.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
73008f26ea 0020-pidof: Prevent integer overflows with grow_size().
Note: unlike "size" and "omit_size", "path_alloc_size" is not multiplied
by "sizeof(struct el)" but the checks in grow_size() allow for a roughly
100MB path_alloc_size, which should be more than enough for readlink().
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
4abe4a51a0 0019-pidof: Do not memleak pidof_root if multiple -c options. 2018-06-23 21:59:14 +10:00
Qualys Security Advisory
55fac85bf0 0018-pidof: Do not skip the NULL terminator in cmdline.
This should never happen (cmdline[0] should always be non-NULL), but
just in case.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
9d59bd6fc9 0017-pidof: Get the arg1 base name with get_basename().
Same as program_base, cmd_arg0base, and exe_link_base.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
cd8499f5f0 0015-tload: Prevent integer overflows of ncols, nrows, and scr_size.
Also, use xerrx() instead of xerr() since errno is not set.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
44d5a5689c 0014-tload: Prevent a buffer overflow when row equals nrows.
When max_scale is very small, scale_fact is very small, row is equal to
nrows, p points outside screen, and the write to *p is out-of-bounds.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
44593c5e99 0013-tload: Use snprintf() instead of sprintf(). 2018-06-23 21:59:14 +10:00
Qualys Security Advisory
39c3542754 0012-tload: Call longjmp() 1 instead of 0.
Do it explicitly instead of the implicit "longjmp() cannot cause 0 to be
returned. If longjmp() is invoked with a second argument of 0, 1 will be
returned instead."
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
fdc2af1114 0011-tload: Use standard names instead of numbers. 2018-06-23 21:59:14 +10:00
Qualys Security Advisory
27fbb21128 0009-uptime: Check the return value of various functions. 2018-06-23 21:59:14 +10:00
Qualys Security Advisory
df0e1a13ab 0007-pgrep: Always null-terminate the cmd*[] buffers.
Otherwise, man strncpy: "If there is no null byte among the first n
bytes of src, the string placed in dest will not be null-terminated."
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
8e6d11a928 0006-pgrep: Initialize the cmd*[] stack buffers.
Otherwise (for example), if the (undocumented) opt_echo is set, but not
opt_long, and not opt_longlong, and not opt_pattern, there is a call to
xstrdup(cmdoutput) but cmdoutput was never initialized:

sleep 60 & echo "$!" > pidfile
env -i LD_DEBUG=`perl -e 'print "A" x 131000'` pkill -e -c -F pidfile | xxd
...
000001c0: 4141 4141 4141 4141 4141 4141 4141 4141  AAAAAAAAAAAAAAAA
000001d0: 4141 4141 4141 4141 fcd4 e6bd e47f 206b  AAAAAAAA...... k
000001e0: 696c 6c65 6420 2870 6964 2031 3230 3931  illed (pid 12091
000001f0: 290a 310a                                ).1.
[1]+  Terminated              sleep 60

(the LD_DEBUG is just a trick to fill the initial stack with non-null
bytes, to show that there is uninitialized data from the stack in the
output; here, an address "fcd4 e6bd e47f")
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
ac85587773 0005-pgrep: Simplify the match_*() functions. 2018-06-23 21:59:14 +10:00
Qualys Security Advisory
8e3e77910d 0004-pgrep: Replace buf+1 with buf in read_pidfile().
Unless we missed something, this makes it unnecessarily difficult to
read/audit.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
9cfc1b8c1f 0003-pgrep: Replace ints with longs in strict_atol().
atol() means long, and value points to a long.
2018-06-23 21:59:14 +10:00
Qualys Security Advisory
7c9a7d7cfe 0002-pgrep: Prevent integer overflow of list size.
Not exploitable (not under an attacker's control), but still a potential
non-security problem. Copied, fixed, and used the grow_size() macro from
pidof.c.

Signed-off-by: Craig Small <csmall@enc.com.au>
2018-06-23 21:59:14 +10:00
Jim Warner
80f9815f5f top: a tweak to the recent 256-color termninal support
We now use the actual terminfo 'max_colors' value with
the 'color mapping' screen, not that hard coded '256'.

Reference(s):
https://gitlab.com/procps-ng/procps/issues/96

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
5705866fb6 top: treat all of those vertical scroll keys uniformly
When not displaying all tasks (the 'i' toggle is off),
the concept of vertical scrolling has no real meaning.

However, only 2 keys (up/down) impacting that vertical
position were currently being disabled with this mode.

This patch will extend such treatment to the following
additional vertical impact keys: pgup,pgdn,home & end.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
19ec80bd41 top: force return to row 1 for thread mode transitions
This program does a good job of policing that vertical
scrolled position, ensuring that total tasks are never
exceeded. However, during transitions from thread mode
to normal task mode (the 'H' toggle) that wasn't true.

And while there was no real harm done, it did make the
use of up/down arrow keys "appear" disabled especially
if that scroll message was not displayed ('C' toggle).

This patch simply forces a return to row #1 whenever a
user toggles that display between thread & task modes.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
c3323bdb68 top: fix 'iokey()' flaw preventing proper translations
As it turns out, the very first entry in the 'iokey()'
tinfo_tab was preventing the proper translation of the
simulated PgUp/PgDn keys (ctrl+meta+k/j). Ignoring the
tortured history behind the most recent change to that
entry, this patch restores the previous value and once
again properly translates these particular keystrokes.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
6cf89d5065 0067-ps/sortformat.c: Avoid "sep_loc + 1" when sep_loc is NULL. 2018-06-09 21:45:38 +10:00
Qualys Security Advisory
f212e552d7 0066-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.

---------------------------- adapted for newlib branch
. now uses 'xmalloc' vs. unchecked stdlib 'malloc'
. the member 'need' was removed from 'format_node'

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
281ac43e35 0065-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-06-09 21:45:38 +10:00
Qualys Security Advisory
aad2b13690 0064-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-06-09 21:45:38 +10:00
Qualys Security Advisory
7bd4f0b6d7 0062-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-06-09 21:45:38 +10:00
Qualys Security Advisory
6f7d610621 0061-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.

---------------------------- adapted for newlib branch
. adapted via 'patch' without rejections

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
c5bbe00770 0060-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.

---------------------------- adapted for newlib branch
. logic is quite different with 'stacks' vs. 'proc_t'

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
0bd44bc76e 0059-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.

---------------------------- adapted for newlib branch
. adapted via 'patch' without rejections

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
d9eb77bd68 0058-ps/output.c: Replace strcpy() with snprintf() in show_one_proc().
This strcpy() should normally not overflow outbuf, but names can be
overridden (via -o). Also, check "amount" in all cases.

---------------------------- adapted for newlib branch
. we don't use that 'likely/unlikely' crap in newlib

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
15c7fa7aba 0057-ps/output.c: Remove the page_shift variable.
It is static and not used anywhere.

---------------------------- adapted for newlib branch
. limited to whitespace/formatting differences

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
70351dd781 0056-ps/output.c: Check return value of mmap() in init_output().
We decided not to check the return value of the mprotect() calls,
because they are not vital to the operation of ps.

---------------------------- adapted for newlib branch
. many formatting/whitespace differences

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
5d7e7cf468 0055-ps/display.c: Harden show_tree().
1/ Do not go deeper than the size of forest_prefix[], to prevent a
buffer overflow (sizeof(forest_prefix) is roughly 128K, but the maximum
/proc/sys/kernel/pid_max is 4M). (actually, we go deeper, but we stop
adding bytes to forest_prefix[])

2/ Always null-terminate forest_prefix[] at the current level.

---------------------------- adapted for newlib branch
. logic is quite different with 'stacks' vs. 'proc_t'
. a commented out 'debug' line was no longer present

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
67c1ec4685 0054-ps/output.c: Fix outbuf overflows in pr_args() etc.
Because there is usually less than OUTBUF_SIZE available at endp.

---------------------------- adapted for newlib branch
. logic is quite different with 'stacks' vs. 'proc_t'
. ps no longer deals with the library 'FILL...' flags

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
43c4d553e6 0053-ps/output.c: Harden forest_helper().
This patch solves several problems:

1/ Limit the number of characters written (to outbuf) to OUTBUF_SIZE-1
(-1 for the null-terminator).

2/ Always null-terminate outbuf at q.

3/ Move the "rightward" checks *before* the strcpy() calls.

4/ Avoid an integer overflow in these checks (e.g., rightward-4).
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
9caf95c0bd 0052-ps/output.c: Handle negative snprintf() return value.
May happen if strlen(src) > INT_MAX for example. This patch prevents
escaped_copy() from increasing maxroom and returning -1 (= number of
bytes consumed in dst).

---------------------------- adapted for newlib branch
. formerly applied to proc/escape.c
. function was moved to ps/output.c

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Qualys Security Advisory
f44fe715bd 0048-ps/output.c: Make sure all escape*() arguments are safe.
The SECURE_ESCAPE_ARGS() macro solves several potential problems
(although we found no problematic calls to the escape*() functions in
procps's code-base, but had to thoroughly review every call; and this is
library code):

1/ off-by-one overflows if the size of the destination buffer is 0;

2/ buffer overflows if this size (or "maxroom") is negative;

3/ integer overflows (for example, "*maxcells+1");

4/ always null-terminate the destination buffer (unless its size is 0).

---------------------------- adapted for newlib branch
. formerly applied to proc/escape.c
. function was moved to ps/output.c

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
f0b245c794 ps: move other initialization code after setREL macros
While the previous patch concerned an essential change
to avoid dereferencing those NULL pointers, this patch
could be considered optional. For consistency, it just
puts all initialization logic after the setREL macros.

[ plus along the way some inter-function spacing was ]
[ standardized with just a single blank line between ]

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
26ab9ae143 ps: ensure functions not called prior to setREL macros
Under newlib design, ps must loop though all potential
print functions so as to gather the appropriate enum's
while establishing the 'relative' equivalent. The keys
to the setREL/chkREL macros are a NULL 'outbuf' param.

It's imperative that no other functions be called with
that NULL value. Unfortunately, several instances were
found where this was violated. They are now corrected!

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
292a4dd6fa ps/output.c: eliminate one irritating compiler warning
Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:45:38 +10:00
Jim Warner
4d9e4ac4f6 top: provide the means to exploit a 256-color terminal
With the Qualys security audit, we began to harden our
treatment of the top rcfile. In particular, the values
read were checked so as to prevent some malicious user
from editing it in order to achieve an evil objective.

However when it came to colors I was surprised to find
that at least one user edited the rcfile for 256-color
support. Unfortunately, our new checks prevented this.

So this commit will provide the means to exploit those
extra colors with no need to manually edit the rcfile.

Reference(s):
https://gitlab.com/procps-ng/procps/issues/96

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00