Check the return value of snprintf(), otherwise dst may point
out-of-bounds when it reaches the end of the dst_buffer (the snprintf()
always returns 1 in that case, even if there is not enough space left),
and vMAX becomes negative and is passed to snprintf() as a size_t.
This detects an integer overflow of "strlen + 1", prevents an integer
overflow of "tot + adj + (2 * pSZ)", and avoids calling snprintf with a
string longer than INT_MAX. Truncate rather than fail, since the callers
do not expect a failure of this function.
1/ Prevent an out-of-bounds write if sz is 0.
2/ Limit sz to INT_MAX, because the return value is an int, not an
unsigned int (and because if INT_MAX is equal to SSIZE_MAX, man 2 read
says "If count is greater than SSIZE_MAX, the result is unspecified.")
3/ Always null-terminate dst (unless sz is 0), because a return value of
0 because of an open() error (for example) is indistinguishable from a
return value of 0 because of an empty file.
4/ Use an unsigned int for i (just like n), not an int.
5/ Check for snprintf() truncation.
Note: this is by far the most important and complex patch of the whole
series, please review it carefully; thank you very much!
For this patch, we decided to keep the original function's design and
skeleton, to avoid regressions and behavior changes, while fixing the
various bugs and overflows. And like the "Harden file2str()" patch, this
patch does not fail when about to overflow, but truncates instead: there
is information available about this process, so return it to the caller;
also, we used INT_MAX as a limit, but a lower limit could be used.
The easy changes:
- Replace sprintf() with snprintf() (and check for truncation).
- Replace "if (n == 0 && rbuf == 0)" with "if (n <= 0 && tot <= 0)" and
do break instead of return: it simplifies the code (only one place to
handle errors), and also guarantees that in the while loop either n or
tot is > 0 (or both), even if n is reset to 0 when about to overflow.
- Remove the "if (n < 0)" block in the while loop: it is (and was) dead
code, since we enter the while loop only if n >= 0.
- Rewrite the missing-null-terminator detection: in the original
function, if the size of the file is a multiple of 2047, a null-
terminator is appended even if the file is already null-terminated.
- Replace "if (n <= 0 && !end_of_file)" with "if (n < 0 || tot <= 0)":
originally, it was equivalent to "if (n < 0)", but we added "tot <= 0"
to handle the first break of the while loop, and to guarantee that in
the rest of the function tot is > 0.
- Double-force ("belt and suspenders") the null-termination of rbuf:
this is (and was) essential to the correctness of the function.
- Replace the final "while" loop with a "for" loop that behaves just
like the preceding "for" loop: in the original function, this would
lead to unexpected results (for example, if rbuf is |\0|A|\0|, this
would return the array {"",NULL} but should return {"","A",NULL}; and
if rbuf is |A|\0|B| (should never happen because rbuf should be null-
terminated), this would make room for two pointers in ret, but would
write three pointers to ret).
The hard changes:
- Prevent the integer overflow of tot in the while loop, but unlike
file2str(), file2strvec() cannot let tot grow until it almost reaches
INT_MAX, because it needs more space for the pointers: this is why we
introduced ARG_LEN, which also guarantees that we can add "align" and
a few sizeof(char*)s to tot without overflowing.
- Prevent the integer overflow of "tot + c + align": when INT_MAX is
(almost) reached, we write the maximal safe amount of pointers to ret
(ARG_LEN guarantees that there is always space for *ret = rbuf and the
NULL terminator).
1/ Replace sprintf() with snprintf() (and check for truncation).
2/ Prevent an integer overflow of ub->siz. The "tot_read--" is needed to
avoid an off-by-one overflow in "ub->buf[tot_read] = '\0'". It is safe
to decrement tot_read here, because we know that tot_read is equal to
ub->siz (and ub->siz is very large).
We believe that truncation is a better option than failure (implementing
failure instead should be as easy as replacing the "tot_read--" with
"tot_read = 0").
1/ Use a "size_t num" instead of an "unsigned num" (also, do not store
the return value of sscanf() into num, it was unused anyway).
2/ Check the return value of strchr() and strrchr().
3/ Never jump over the terminating null byte with "S = tmp + 2".
1/ Prevent an integer overflow of t.
2/ Avoid an infinite loop if s contains characters other than comma,
spaces, +, -, and digits.
3/ Handle all possible return values of snprintf().
1/ Do not read past the terminating null byte when hashing the name.
2/ S[x] is used as an index, but S is "char *S" (signed) and hence may
index the array out-of-bounds. Bit-mask S[x] with 127 (the array has 128
entries).
3/ Use a size_t for j, not an int (strlen() returns a size_t).
Notes:
- These are (mostly) theoretical problems, because the contents of
/proc/PID/status are (mostly) trusted.
- The "name" member of the status_table_struct has 8 bytes, and
"RssShmem" occupies exactly 8 bytes, which means that "name" is not
null-terminated. This is fine right now, because status2proc() uses
memcmp(), not strcmp(), but it is worth mentioning.
This function is unused (SIGNAL_STRING is defined by default, and if it
is not, procps does not compile -- for example, there is no "outbuf" in
help_pr_sig()) but fix it anyway. There are two bugs:
- it accepts non-hexadecimal characters (anything >= 0x30);
- "(c - (c>0x57) ? 0x57 : 0x30)" is always equal to 0x57.
There was a "buff[BUFFSIZE-1] = 0;" but there may be garbage between
what is read() (less than BUFFSIZE-1 bytes) and this null byte. Reuse
the construct from the preceding getrunners().
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).
This solves several problems:
1/ outbuf[1] was written to, but not outbuf[0], which was left
uninitialized (well, SECURE_ESCAPE_ARGS() already fixes this, but do it
explicitly as well); we know it is safe to write one byte to outbuf,
because SECURE_ESCAPE_ARGS() guarantees it.
2/ If bytes was 1, the write to outbuf[1] was an off-by-one overflow.
3/ Do not call escape_str() with a 0 bufsize if bytes == overhead.
4/ Prevent various buffer overflows if bytes <= overhead.
Simply rearrange the old comparisons. The new comparisons are safe,
because we know from previous checks that:
1/ wlen > 0
2/ my_cells < *maxcells (also: my_cells >= 0 and *maxcells > 0)
3/ len > 1
4/ my_bytes+1 < bufsize (also: my_bytes >= 0 and bufsize > 0)
This should never happen, because wcwidth() is called only if iswprint()
returns nonzero. But belt-and-suspenders, and make it visually clear
(very important for the next patch).
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).
In the human_readable case; otherwise the strcat() that follows may
append bytes to the previous contents of buf.
Also, slightly enlarge buf, as it was a bit too tight.
Could also replace all sprintf()s with snprintf()s, but all the calls
here output a limited number of characters, so they should be safe.
Especially its "next" member: this is what caused the crash in "slabtop:
Reset slab_list if get_slabinfo() fails." (if parse_slabinfo*() fails in
sscanf(), for example, then curr is set to NULL but it is already linked
into the "list" and its "next" member was never initialized).
In proc/slab.c, functions parse_slabinfo20() and parse_slabinfo11(),
sscanf() might overflow curr->name, because "String input conversions
store a terminating null byte ('\0') to mark the end of the input; the
maximum field width does not include this terminator."
Add one byte to name[] for this terminator.
Otherwise this can truncate sizes on 64-bit platforms, and is one of the
reasons the integer overflows in file2strvec() are exploitable at all.
Also: catch potential integer overflow in xstrdup() (should never
happen, but better safe than sorry), and use memcpy() instead of
strcpy() (faster).
Warnings:
- in glibc, realloc(ptr, 0) is equivalent to free(ptr), but not here,
because of the ++size;
- here, xstrdup() can return NULL (if str is NULL), which goes against
the idea of the xalloc wrappers.
We were tempted to call exit() or xerrx() in those cases, but decided
against it, because it might break things in unexpected places; TODO?
This can disclose information from the stack, but is unlikely to have a
security impact in the context of the procps utilities:
user@debian:~$ w 2>&1 | xxd
00000000: a03c 79b7 1420 6661 696c 6564 2074 6f20 .<y.. failed to
00000010: 616c 6c6f 6361 7465 2033 3232 3137 3439 allocate 3221749
00000020: 3738 3020 6279 7465 7320 6f66 206d 656d 780 bytes of mem
00000030: 6f72 79 ory
Do not memleak "copy" in case of an error.
Do not use "sizeof(converted)" in snprintf(), since "converted" is a
"char *" (luckily, 8 >= sizeof(char *)). Also, remove "sizeof(char)"
which is guaranteed to be 1 by the C standard, and replace 8 with 12,
which is enough to hold any stringified int and does not consume more
memory (in both cases, the glibc malloc()ates a minimum-sized chunk).
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.
The procio functions that were in the library have been
moved into sysctl. sysctl is not linked to libprocps in
newlib and none of the other procps binaries would need
to read/write large data to the procfs.
References:
be6b048a41
to be able to read and write large buffers below /proc.
The buffers and file offsets are handled dynamically
on the required buffer size at read, that is lseek(2)
is used to determine this size. Large buffers at
write are split at a delimeter into pieces and also
lseek(2) is used to write each of them.
Signed-off-by: Werner Fink <werner@suse.de>
Since the value of number_of_signals is known at compile time, we can
use a compile-time check instead. This also adds SIGLOST for the Hurd,
uses the correct signal counts for the Hurd and FreeBSD, and only gives
a compile-time warning when compiled on an unknown platform that it does
not know whether the number of signals is correct.
There is no longer justification for placing a typedef
employed in overriding that alloc.h message handler in
the procps.h header file. So this commit just moves it
to the alloc.h header file itself where's it's needed!
[ gosh, sure wish i had thought to relocate this guy ]
[ when the changes in the 1st commit shown were made ]
Reference(s):
. most recent related changes
commit 18e5aecd2b
. place where it *should* have been relocated
commit 2865ded64e
Signed-off-by: Jim Warner <james.warner@comcast.net>
Way back in November of 2011, the library was equipped
with an overridable error message handler function. It
was done expressly for a program like top which alters
the tty. But that support was withdrawn shortly after.
This was all done in the lead up to v3.3.2. That's the
release where NLS support was added and it represented
a hectic time. In hindsight, the changes went too far.
So this commit, in a minimal fashion, restores ability
to address a potential fatal library error. After all,
any properly behaving library would never unilaterally
subject a caller to a stderr message and then an exit.
[ when exposing 1 variable in libprocps.sym, 2 other ]
[ existing symbols were repositioned alphabetically. ]
Reference(s):
. generalized library memory provisions
commit 7126cc4491
. top exploit library memory provisions
commit 88087ec5a5
. library xalloc type functions made private
commit 2865ded64e
. restored prior top memory logic
commit 05f5deb97c
Signed-off-by: Jim Warner <james.warner@comcast.net>
When supplying the -p command to uptime, it does not display any
sections where the value is less than 1; however, after a reboot, this
causes the command to just output "up". Showing 0 minutes when the
system has been up for less than a minute makes it clear a reboot just
occurred.
The combined results of merge request #49 without that
overhead plus distortion in this repository's history.
Prototyped-by: Wayne Porter <wporter82@gmail.com>
In response to that suggestion referenced below, these
changes allow display of task/thread level NUMA nodes.
Currently, only the 'top' program offers any NUMA type
support and it is limited to the Summary Area display.
With this commit both the 'top' and 'ps' programs will
be able to display NUMA nodes associated with threads.
[ this patch has been adapted from the newlib branch ]
[ and implemented so as to preserve the existing ABI ]
Reference(s):
https://gitlab.com/procps-ng/procps/issues/58
Signed-off-by: Jim Warner <james.warner@comcast.net>
Reference(s):
proc/readproc.c: In function 'statm2proc'
proc/readproc.c:627:9: warning: variable 'num' set but not used [-Wunused-but-set-variable]
ps/output.c: In function 'pr_context':
ps/output.c:1273:14: warning: unused variable 'tried_load' [-Wunused-variable]
ps/output.c:1272:16: warning: unused variable 'ps_is_selinux_enabled' [-Wunused-variable]
ps/output.c:1272:16: warning: 'ps_is_selinux_enabled' defined but not used [-Wunused-variable]
ps/output.c:1273:14: warning: 'tried_load' defined but not used [-Wunused-variable]
ps/output.c:1837:18: warning: 'shortsort_array_count' defined but not used [-Wunused-const-variable=]
ps/output.c:1803:18: warning: 'aix_array_count' defined but not used [-Wunused-const-variable=]
ps/parser.c: In function 'arg_type':
ps/parser.c:1098:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
ps/parser.c:1099:34: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
ps/sortformat.c: In function 'format_parse':
ps/sortformat.c:241:1: warning: label 'out' defined but not used [-Wunused-label]
ps/stacktrace.c:176:13: warning: 'stack_trace_sigsegv' defined but not used [-Wunused-function]
watch.c: In function 'process_ansi':
watch.c:234:5: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
watch.c:237:2: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
Signed-off-by: Jim Warner <james.warner@comcast.net>
The commit (merge) referenced below added some useless
trailing whitespace, and this patch will correct such.
[ this also updates the NEWS file for the buglet fix ]
Gosh, if folks cannot coax their editors into avoiding
such crap they should remove the '.sample' suffix from
their '.git/hooks/pre-commit.sample' file. Thereafter,
git itself will reject changes with whitespace errors.
Reference(s):
commit cc1f49aeba
Signed-off-by: Jim Warner <james.warner@comcast.net>
