Right now, if read() returns less than 127 bytes (the most likely case),
the end of the "string" buf will contain garbage from the stack, because
buf is always null-terminated at a fixed offset 127. This is especially
bad because the next operation is a strrchr().
Also, make sure that the whole /proc/PID/stat file is read, otherwise
its parsing may be unsafe (the strrchr() may point into user-controlled
data, comm). This should never happen with the current file format (comm
is very short), but be safe, just in case.
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.
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."
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."
This is one of the worst issues that we found: if the strlen() of one of
the cmdline arguments is greater than INT_MAX (it is possible), then the
"int bytes" could wrap around completely, back to a very large positive
int, and the next strncat() would be called with a huge number of
destination bytes (a stack-based buffer overflow).
Fortunately, every distribution that we checked compiles its procps
utilities with FORTIFY, and the fortified strncat() detects and aborts
the buffer overflow before it occurs.
This patch also fixes a secondary issue: the old "--bytes;" meant that
cmdline[sizeof (cmdline) - 2] was never written to if the while loop was
never entered; in the example below, "ff" is the uninitialized byte:
((exec -ca `python3 -c 'print("A" * 131000)'` /usr/bin/cat < /dev/zero) | sleep 60) &
pgrep -a -P "$!" 2>/dev/null | hexdump -C
00000000 31 32 34 36 30 20 41 41 41 41 41 41 41 41 41 41 |12460 AAAAAAAAAA|
00000010 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 |AAAAAAAAAAAAAAAA|
*
00001000 41 41 41 41 ff 0a 31 32 34 36 32 20 73 6c 65 65 |AAAA..12462 slee|
00001010 70 20 36 30 0a |p 60.|
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")
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.
memset()ing task and subtask inside their loops prevents free_acquired()
(in readproc() and readtask()) from free()ing their contents (especially
cmdline and environ).
Our solution is not perfect, because we still memleak the very last
cmdline/environ, but select_procs() is called only once, so this is not
as bad as it sounds.
It would be better to leave subtask in its block and call
free_acquired() after the loop, but this function is static (not
exported).
The only other solution is to use freeproc(), but this means replacing
the stack task/subtask with xcalloc()s, thus changing a lot of code in
pgrep.c (to pointer accesses).
Hence this imperfect solution for now.
sig.c had this odd logic where on non-Hurd systems it would undefine
SIGLOST. Fine for Hurd or amd64 Linux systems. Bad for a sparc which
has SIGLOST defined *and* is not Hurd.
Just check its defined, its much simpler.
If pgrep is run with a non-program name match and there are
no matches, it segfaults.
The testsuite thinks zero bytes sent, and zero bytes sent
because the program crashed is the same :/
References:
commit 1aacf4af7fhttps://bugs.debian.org/894917
Signed-off-by: Craig Small <csmall@enc.com.au>
Update NEWS with the version
Add library API change into NEWS
Update c:r:a for library to 7:0:1
This means the current and age are incremented, so old programs can
use new library but not vice-versa as they won't have the numa*
functions.
pidof will miss scripts that are run a certain way due to how
they appear in procfs. This is just a note to say it might miss
them.
References:
procps-ng/procps#17
Hurd doesn't have HOST_NAME_MAX, neither does Solaris.
An early fix just checked for this value and used 64 instead.
This change uses sysconf which is the correct method, possibly until
this compiles on some mis-behaving OS which doesn't have this value.
References:
commit e564ddcb01procps-ng/procps#54
