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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
- 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).
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).
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()).
Several of these buffer overflows can actually be triggered (through the
configuration file for example): in config_file(), inspection_utility(),
and show_special().
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()."
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.
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.
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.
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.
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>
As it turns out, that Ukrainian 'demo' text supporting
the '=' command was 152 bytes long, up from an English
version of 80 bytes. Unfortunately, the buffer used to
format all such strings was insufficient at 128 bytes.
Depending on the width of one's terminal, some strange
result could be experienced when a multi-byte sequence
was truncated. So, this just makes that buffer bigger.
Signed-off-by: Jim Warner <james.warner@comcast.net>
After wrestling with extra wide characters, supporting
languages like zh_CN, sometimes default/minimum column
widths might force a truncation of translated headers.
So, this commit explores one way that such truncations
could be avoided. It is designed so as to have minimal
impact on existing code, ultimately affecting just one
function. But it's off by default via its own #define.
Signed-off-by: Jim Warner <james.warner@comcast.net>
When I recently added extra wide character support for
locales like zh_CN, I didn't worry about some overhead
associated with the new calls to 'mbtowc' & 'wcwidth'.
That's because such overhead was usually incurred with
user interactions, not a normal iterative top display.
There was, however, one area where this overhead would
impact the normal iterative top mode - that's with the
Summary display. So I peeked at the glibc source code.
As it turns out, the costs of executing those 'mbtowc'
and 'wcwidth' functions were not at all insignificant.
So, this patch will avoid them in the vast majority of
instances, while still enabling extra wide characters.
Signed-off-by: Jim Warner <james.warner@comcast.net>
There is (should be) no justification for changing the
width of the percentage columns (%CPU, %MEM) depending
on the BOOST_PERCNT #define. So this patch will ensure
that both columns are fixed at their former maximum 5.
Signed-off-by: Jim Warner <james.warner@comcast.net>
With the documentation update in the commit referenced
below, we should also account for such threads as they
will already be represented in the task/thread totals.
[ and do it in a way that might avoid future changes ]
Reference(s):
commit a238a687ce
Signed-off-by: Jim Warner <james.warner@comcast.net>
