Commit Graph

325 Commits

Author SHA1 Message Date
Jim Warner
80057ec06f 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>
2018-06-23 22:03:56 +10:00
Jim Warner
1fe5a5415f 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>
2018-06-23 22:03:56 +10:00
Jim Warner
2b7779fdeb 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>
2018-06-23 22:03:56 +10:00
Jim Warner
e691f75b81 top: let's honor <Esc> key on color mapping screen too
Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-23 22:03:56 +10:00
Jim Warner
291d98ee50 top: make rcfile duplicate fields check more efficient
Jeeze, there was no need to employ *both* strchr() and
strrchr() when ensuring fields hadn't been duplicated.

So let's avoid one of those function calls completely.

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-23 22:03:56 +10:00
Jim Warner
949a3c9c2e 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
. introduced 256 color support
commit cf057d2fe5

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-06-13 22:11:29 +10:00
Jim Warner
9ea85bc653 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-13 22:11:28 +10:00
Jim Warner
9dd7a97c4a 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-13 22:11:28 +10:00
Jim Warner
42f0a341ba 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-13 22:11:28 +10:00
Jim Warner
cf057d2fe5 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-07 21:35:59 +10:00
Jim Warner
fe23360520 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-05-31 20:28:32 +10:00
Jim Warner
434530a038 top: prevent buffer overruns in 'inspection_utility()'
When a Qualys patch was reverted as being unwarranted,
1 specific problem their patch had, in fact, prevented
was re-introduced. This patch corrects that oversight.

Reference(s):
. qualys patch revert
commit c502678715

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-31 20:28:32 +10:00
Jim Warner
a42742b0df 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-05-31 20:28:32 +10:00
Jim Warner
c699f2e26d 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):
. increased 'comm' length
commit 2cfdbbe897

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:48:13 +10:00
Jim Warner
607edc2064 top: eliminate a couple of warnings of -Wunused-result
Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:47:58 +10:00
Jim Warner
de807a639f top: ensure sane rcfile values for the remaining stuff
This will protect some remaining rcfile variables from
a possible manual editing of top's configuration file.

[ and correct two #error related boo-boos introduced ]
[ with the system default rcfile in the commit shown ]

Reference(s):
. introduced /etc/topdefaultrc
commit 3e6a208ae5

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:47:02 +10:00
Jim Warner
d34c6eb5b4 top: address 'show_special()' o-o-b read/write concern
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
commit ed8f6d9cc6

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:45:36 +10:00
Jim Warner
2fb269a5b0 top: Fix out-of-bounds read/write in show_... REVERTED
I'm reverting this patch to prepare for some alternate
solution. In that solution I will address point #1 but
point #2 is based on a wrong assumption. There will be
no binary data ever found in the 'glob' passed to this
show_special() function. It is now always simple text.

------------------------------------------------ original commit message
This patch fixes two problems:

1/ In the switch case 0, if sub_end is at the very end of lin[], the two
null-byte writes are off-by-two (a stack-based buffer overflow). Replace
this end-of-string "emulation" with an equivalent test on ch (and then
goto/break out of the loop).

2/ "sub_end += 2" jumps over the null-byte terminator in lin[] if the
line contains a raw (without a tilde) \001-\010 character. Detect such a
null-byte terminator and goto/break out of the loop.

Note: in the case of a raw \001-\010 character, the character at
"sub_end + 1" is never processed (it is skipped/jumped over); this is
not a security problem anymore (since 2/ was fixed), so we decided not
to change this behavior, for backward-compatibility.
------------------------------------------------------------------------

Reference(s):
. original qualys patch
0116-top-Fix-out-of-bounds-read-write-in-show_special.patch
commit ed8f6d9cc6

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:44:35 +10:00
Jim Warner
fdb58974e2 top: prevent buffer overflow potential in all routines
Whereas that original patch (since reversed) 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
commit c424a64331

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:44:11 +10:00
Jim Warner
72ddc1c33d top: Prevent buffer overflow in calibrate_... REVERTED
Here, again, we have an example of attacking a problem
by addressing the symptoms. And that assertion made in
the original commit message is true if only if someone
had manually (maliciously) edited the top config file.

So let's reverse the original patch & thus prepare for
a proper solution addressing the cause, not a symptom.

Reference(s):
. original qualys patch
0114-top-Prevent-buffer-overflow-in-calibrate_fields.patch
commit c424a64331

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:43:08 +10:00
Jim Warner
c502678715 top: Protect scat() from buffer overflows. ___REVERTED
The whole idea was to make top's 'scat' function small
and very quick, unlike that standard 'strcat' routine.

To achieve that end we ignore the potential for buffer
overruns and trust callers to provide adequate dest's.

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

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:42:36 +10:00
Jim Warner
59f39da852 top: concede integer overflow risks in procs_refresh()
This is as far as we need go with respect to the issue
of integer overflow addressed in that reference below.

That patch, of course, was reversed to prepare for us.

Reference(s):
. original qualys patch
0105-top-Prevent-integer-overflows-in-procs_refresh.patch
commit 131e5e2fe6

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:38:19 +10:00
Jim Warner
c9dfcdebdc top: Prevent integer overflows in procs_re... REVERTED
That patch referenced below is being reverted because:

. By design, no other top macro looks like a function.
Instead, they all contain some minimal capitalization.
The 'grow_by_size' macro stands out like a sore thumb.

. We would need to approach 400+ million tasks for for
the 1st addressed problem to produce integer overflow.

. And a 2nd check against SSIZE_MAX remains a mystery.

Me thinks a system on which top is running will suffer
ENOMEM before we need to worry about integer overflow.

Reference(s):
. original qualys patch
0105-top-Prevent-integer-overflows-in-procs_refresh.patch
commit 131e5e2fe6

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:37:29 +10:00
Jim Warner
3b2235c266 top: check sortindx risk exposure (not treat symptoms)
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
commit d5b8ac7139

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:24:36 +10:00
Jim Warner
fb8cee585f top: Check sortindx. _________________________REVERTED
Here's yet another example of dealing with a potential
problem at the symptom level, instead of addressing it
at the source. So, we will reverse that original patch
referenced below in preparation for a proper solution.

[ at the least, this ugly code should have used that ]
[ existing MAXTBL macro, making it a little prettier ]

Reference(s):
. original qualys patch
0102-top-Check-sortindx.patch
commit d5b8ac7139

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:23:57 +10:00
Jim Warner
53e2654726 top: other graph_cpus, graph_mems, and summ_mscale fix
This patch replaces an original patch referenced below
which has now been reversed. 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
commit cd8ba5670e

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:22:56 +10:00
Jim Warner
d1fbc5fbd4 top: Check graph_cpus, graph_mems, and sum... REVERTED
The variables graph_cpus, graph_mems & summ_mscale are
all well managed in a running top. They were, however,
each vulnerable to tampering via the rcfile. So rather
than continually addressing the symptoms, we'll attack
the root cause just once at startup in the next patch.

Reference(s):
. original qualys patch
0099-top-Check-graph_cpus-graph_mems-and-summ_mscale.patch
commit cd8ba5670e

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:19:35 +10:00
Jim Warner
e531c78140 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
commit b45c4803dd

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-05-19 21:17:59 +10:00
Qualys Security Advisory
ed8f6d9cc6 top: Fix out-of-bounds read/write in show_special().
This patch fixes two problems:

1/ In the switch case 0, if sub_end is at the very end of lin[], the two
null-byte writes are off-by-two (a stack-based buffer overflow). Replace
this end-of-string "emulation" with an equivalent test on ch (and then
goto/break out of the loop).

2/ "sub_end += 2" jumps over the null-byte terminator in lin[] if the
line contains a raw (without a tilde) \001-\010 character. Detect such a
null-byte terminator and goto/break out of the loop.

Note: in the case of a raw \001-\010 character, the character at
"sub_end + 1" is never processed (it is skipped/jumped over); this is
not a security problem anymore (since 2/ was fixed), so we decided not
to change this behavior, for backward-compatibility.
2018-05-19 07:33:15 +10:00
Qualys Security Advisory
6b8b102cf2 top: Harden calibrate_fields().
- 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).
2018-05-19 07:33:15 +10:00
Qualys Security Advisory
c424a64331 top: Prevent buffer overflow in calibrate_fields().
pflgsall[] can contain PFLAGSSIZ = 100 elements, each iteration of the
loop can write 3 elements to pflgsall[], and there are EU_MAXPFLGS = 58
iterations: a buffer overflow (it can be triggered via the configuration
file, for example, by filling "fieldscur" with the "sortindx" flag).
2018-05-19 07:33:15 +10:00
Qualys Security Advisory
a71ac048e6 top: Impose a minimum on Screen_cols.
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()).
2018-05-19 07:33:15 +10:00
Qualys Security Advisory
b4d068e624 top: Prevent integer overflow in adj_geometry(). 2018-05-19 07:33:15 +10:00
Qualys Security Advisory
23baeb1175 top: Limit Width_mode to SCREENMAX.
adj_geometry() limits to SCREENMAX too, but belt and suspenders, and
might as well tell the user about it.
2018-05-19 07:33:15 +10:00
Qualys Security Advisory
8ab8c1a469 top: Prevent integer overflows in config_file() and other_selection(). 2018-05-19 07:33:15 +10:00
Qualys Security Advisory
9c745975b2 top: Protect scat() from buffer overflows.
Several of these buffer overflows can actually be triggered (through the
configuration file for example): in config_file(), inspection_utility(),
and show_special().
2018-05-19 07:32:34 +10:00
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
Jim Warner
029a463172 top: show that truncation indicator ('+') consistently
With a little luck, this should be the final tweak for
our support of extra wide characters. Currently, those
characters don't always display the '+' indicator when
they've been truncated. Now, it should always be seen.

[ plus it's done a tad more efficiently via snprintf ]

Signed-off-by: Jim Warner <james.warner@comcast.net>
2018-03-03 17:52:43 +11:00