Jim Warner [Thu, 21 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: a few tweaks for those scrolling (mostly) changes
This patch just addresses some edge cases with respect
to 'unseen' tasks. Given the ability to preserve other
filters in the rcfile, it's entirely possible the very
first task(s) may not be visible at top startup. Also,
when switching between windows ('a'/'w') we should try
to always position its row #1 on some visible process.
Lastly, a window might have *NO* visible tasks at all.
Therefore, protect 'window_hlp' from an infinite loop.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: normalize vertical scrolling for hidden processes
To my knowledge, nobody has ever complained about some
anomalies when scrolling vertically if tasks should be
hidden from view. This can happen with the user filter
('u/U') or other filter ('o/O') features. And although
some tasks are not shown, they still impact scrolling.
This is most apparent when that scroll coordinates msg
is on ('C') & up/down arrow keys used (vs. pgup/pgdn).
Now that we can collapse/expand forked children, there
is a potential for yet more of those hidden processes.
So this commit normalizes vertical scrolling providing
an expected behavior. In other words, the up/down keys
skip the unseen tasks to reposition on a visible task.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactor the 'scroll coordinates' message support
This patch is simply preparation for upcoming vertical
scrolling enhancements. With those changes, it will be
impossible to predict what the beginning task position
should be at the time the message is currently issued.
This patch will allow such a message to be shown after
the individual windows' tasks have all been displayed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
The issue cited below really dealt with preserving the
'Other filter' criteria in the rcfile. But as an aside
the htop 'F6' feature (collapsed children) was raised.
I took that as an implied challenge and decided to try
implementing a similar feature in top. So, this commit
will now provide a brand new forest view toggle ('^V')
which will be used to collapse/expand forked children.
[ this patch will also lead to additional patches in ]
[ support of more rational vertical scrolling, since ]
[ many more tasks might now be hidden in some window ]
Reference(s):
. where this secondary issue was raised
https://gitlab.com/procps-ng/procps/issues/99
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: to prepare for collapse, move forest view support
In anticipation of a new collapsible child feature, we
will have to make some forest view variables available
to that 'keys_task()' function. This commit just moves
the forest view logic ahead of tertiary input support.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: 'other filters' saved with config file, pgm logic
Well, after the rearranging and refactoring, all those
active 'other filter' entries for each window will now
be preserved in the user's configuration file via 'W'.
For raising the issue below, thanks to Marco Ippolito.
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactored some more peripheral 'inspect' support
These modifications are being made now in anticipation
of some coming 'other filter' config file changes. Our
entries must be written last to the rc file since that
is where the users have been told to 'echo' additions.
Therefore, that 'config_insp' function must be adapted
to anticipate a passed buffer that was already primed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactored some of that 'other filtering' support
If we are to support preserving 'other filter' entries
in the rcfile, then the current logic setting up those
osel entries for a WIN_t must be shareable for startup
and when interacting with a user. So, this commit just
repositions this current code in a shareable function.
[ along the way, we give the prior guy a proper name ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: reposition some of that 'other filtering' support
When we get around to saving that 'Other Filter' stuff
in the rcfile, we'll need access to the Fieldstab plus
the justify_pad() function. So this commit repositions
two 'osel' functions in anticipation of adding 1 more.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Wed, 20 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: refactor some of that configuration files support
The 'config_file()' function was getting a little long
in the tooth, so this commit simply renames/rearranges
some stuff anticipating 'other filters' in the rcfile.
Signed-off-by: Jim Warner <james.warner@comcast.net>
0121-w: Clamp maxcmd to the MIN/MAX_CMD_WIDTH range.
The current checks allow out-of-range values (for example, if
getenv/atoi returns ~-2GB, maxcmd becomes ~+2GB after the subtraction).
This is not a security problem, none of this is under an attacker's
control.
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:
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.
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.
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.
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".
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:
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.
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).
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)").
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."
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().
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."
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:
(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")
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.
Jim Warner [Thu, 7 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
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>
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>
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.
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).
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>
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>
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>
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>
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>
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>
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>
Jim Warner [Wed, 6 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
Jim Warner [Wed, 6 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
Jim Warner [Tue, 5 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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.
Jim Warner [Sun, 3 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
Jim Warner [Sun, 3 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
Jim Warner [Sun, 3 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
Jim Warner [Sun, 3 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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>
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>
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 c546d9dd4409ee11cd466c99a820a3b5dadfe3f4
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sat, 2 Jun 2018 05:00:00 +0000 (00:00 -0500)]
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.
Jim Warner [Sat, 2 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: Prevent out-of-bounds writes in PUFF(). __Tweaked
This commit moves some overhead to the Batch mode path
where it's needed. And given the new 'else if' test we
can delete some now redundant logic in the other path.
Reference(s):
. original qualys patch
0117-top-Prevent-out-of-bounds-writes-in-PUFF.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch addresses a potential (but unlikely) buffer
overflow by reducing, if necessary, a memcpy length by
3 bytes to provide for an eol '\0' and 2 unused buffer
positions which also might receive the '\0' character.
[ note to future analysis tool: just because you see ]
[ binary data being manipulated in the routine, that ]
[ doesn't mean such function was passed binary data! ]
Reference(s):
. original qualys patch
0116-top-Fix-out-of-bounds-read-write-in-show_special.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sat, 2 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: prevent buffer overflow potential in all routines
Whereas an original patch (referenced below) addressed
some symptoms related to manually edited config files,
this solution deals with root causes. And it goes much
beyond any single top field by protecting all of top's
fields. Henceforth, a duplicated field is not allowed.
Reference(s):
. original qualys patch
0114-top-Prevent-buffer-overflow-in-calibrate_fields.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Rather than validate the window's 'sortindx' each time
it was referenced (as was done in the patch below), we
now ensure the validity just once when the config file
is read. Thereafter, a running top will police bounds.
Reference(s):
. original qualys patch
0102-top-Check-sortindx.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sat, 2 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: other graph_cpus, graph_mems, and summ_mscale fix
This patch replaces an original patch referenced below
(omitted under this branch). We now validate variables
'graph_cpus', 'graph_mems' and 'summ_mscale' just once
at startup. Thereafter, top enforces the proper range.
[ we afford the same treatment to that 'task_mscale' ]
[ variable, which was ignored in the original patch. ]
Reference(s):
. original qualys patch
0099-top-Check-graph_cpus-graph_mems-and-summ_mscale.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
Jim Warner [Sat, 2 Jun 2018 05:00:00 +0000 (00:00 -0500)]
top: Do not default to the cwd in configs_r... Tweaked
While it's only documented (so far) in commit text and
an occasional email I've tried to maintain some coding
standards primarily for reference/navigation purposes.
They also served, I felt, as useful mental challenges.
Someday I will get around to formerly documenting them
but in the meantime here are the ones for this commit:
. functions are grouped into logical (i hope) sections
. functions & sections are ordered to avoid prototypes
. function names are alphabetical within every section
. all functions & sections must be referenced in top.h
This patch just attempts to honor the above standards,
while also covering this new behavior in the man page.
[ please note that the net result of these 2 patches ]
[ is simply to avoid pathname truncations should our ]
[ limit of 1024 be exceeded. they do not have a role ]
[ in solving the 'local privilege escalation' issue. ]
[ and we can never prevent a user from setting their ]
[ HOME var to a directory writable by some attacker! ]
[ the only real protection for that CVE-2018-1122 is ]
[ those soon to be enhanced rcfile integrity checks, ]
[ achieved through several of the following patches. ]
Reference(s):
. original qualys patch
0097-top-Do-not-default-to-the-cwd-in-configs_read.patch
Signed-off-by: Jim Warner <james.warner@comcast.net>
1/ Because snprintf() returns "the number of characters (excluding the
terminating null byte) which would have been written to the final string
if enough space had been available", _eol may point past the end of _str
and write out-of-bounds (in Batch mode).
2/ _eol is never checked against _str, so "while (*(--_eol) == ' ');"
may point _eol below _str and write out-of-bounds (in Batch mode).
3/ Sanity-check Pseudo_row to protect the strcpy().
- Make sure i is at least 1 before "i - 1" and "--i".
- Initialize endpflg (to 0, as it was originally, since it is static)
before the "for" loop (the "break" may leave endpflg uninitialized,
for example).
The safety of the critical function task_show() depends on the sanity of
Screen_cols. Just copy the tests on w_cols to Screen_cols (from the same
function adj_geometry()).
projects / procps-ng / log