Commit Graph

2448 Commits

Author SHA1 Message Date
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
Jim Warner
fa96f3e5dc top: sanitized some potentially corrupt 'Inspect' data
This guards against rcfile 'Inspect' entries which may
include non-printable characters. While this shouldn't
occur, we have no real control over those crazy users.

[ and, while such data can't be used maliciously, it ]
[ does adversely impact such a user's screen display ]

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
34feb6183a top: prevent buffer overruns in 'inspection_utility()'
For our master branch, a Qualys patch referenced below
was reverted as being unwarranted. That original patch
was not applied in this branch so there was no revert.

However, there was 1 specific problem their patch had,
in fact, prevented. Thus, this patch now addresses it.

Reference(s):
. original qualys patch
0109-top-Protect-scat-from-buffer-overflows.patch

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
59c8c1c86c top: add another field sanity check in 'config_file()'
Until the Qualys security audit I had never considered
it a possibility that some malicious person might edit
the top config file to achieve some nefarious results.

And while the Qualys approach tended to concentrate on
the symptoms from such an effort, subsequent revisions
more properly concentrated on startup and that rcfile.

This commit completes those efforts with 1 more field.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
8ce5f080a2 top: don't mess with groff line length in man document
I've long since forgotten why the attempt to influence
groff line lengths was made. However, I did receive an
email regarding problems formatting postscript output.

Hopefully this patch will eliminate any such problems.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
50ae55ef10 library: avoid problems involving 'supgid' mishandling
Following that patch referenced below, the top SUPGRPS
field would produce a segmentation fault and ps SUPGRP
would often show "(null)". Such problems resulted from
some faulty logic in the status2proc() routine dealing
with 'Groups' (supgid) which served as a source field.

For many processes the original code produced an empty
string which prevented conversion to the expected "-".
Moreover, prior to release 3.3.15 such an empty string
will become 0 after strtol() which pwcache_get_group()
translates to 'root' yielding very misleading results.

So, now we'll check for empty '/proc/#/status/Groups:'
fields & consistently provide a "-" value for callers.

[ we'll also protect against future problems in that ]
[ new qualys logic by always ensuring valid 'supgrp' ]
[ pointers - logic which revealed our original flaw! ]

Reference(s):
. original qualys patch
0071-proc-readproc.c-Harden-supgrps_from_supgids.patch

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
80e210d38e library: refactor #define FALSE_THREADS dependent code
This refactor was done in response to the Qualys patch
referenced below, which deals with some 'readeither()'
flaws under the master branch. Under our newlib branch
those flaws mostly disappear since the function is now
private. But without a redesign the #define is broken.

When the #define FALSE_THREADS is active, some special
strings showing "[ duplicate ENUM ]" will appear under
each child thread. Note that the real reason for those
appearing isn't being exercised, only their mechanics.

In reality, they only show when a user duplicates such
enums in a results stack & only 1 instance can own it.

Reference(s):
. original qualys patch
0084-proc-readproc.c-Work-around-a-design-flaw-in-readeit.patch
. QUICK_THREADS became FALSE_THREADS
commit c546d9dd44

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
97d078a9af library: clean up some miscellaneous compiler warnings
Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-09 21:35:20 +10:00
Jim Warner
2c6a480cc8 top: just respond to the increased command name length
The command name for running tasks is displayed by top
in a variable length field, so the increase from 16 to
64 bytes was not a problem. However, there's one place
where top is sensitive to length - insp_view_choice().

So, this patch just bumps a buffer used to display it.

Reference(s):
. master branch increase to 64
commit 2cfdbbe897

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