| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
The -b/--size handler checks the lower bound after casting the value to
unsigned long while checking the upper bound as a signed comparison. A
negative argument such as "-b -65536" therefore satisfies both checks.
The value is then truncated to an undersized icmp_data_size, which later
serves as the size of the ICMP send buffer, so building the packet
overflows that buffer.
Compare the size as a signed long against both bounds so negative values
are rejected.
Reported-by: Christopher Kreft <Email@ChristopherKreft.de>
|
|
The threshold parser starts a pointer at the last character of the
string and walks it backwards until it reaches the second character.
This assumes the string is at least two characters long. For a
single-character threshold such as "-w 1" or "-c 1", the pointer
underflows past the start of the string and keeps dereferencing memory
out of bounds.
Beyond the out-of-bounds reads, an out-of-bounds write can occur if a
stray '%' or ',' byte happens to turn up while scanning backwards. In
that case, the parser writes a NUL byte at that out-of-bounds address
(the ',' case additionally re-reads forward from there via strtoul(3)).
Only run the descending scan when the string has at least two
characters, leaving the behaviour for all valid thresholds unchanged.
Reported-by: Christopher Kreft <Email@ChristopherKreft.de>
|
|
The number of -H hosts is counted into an unsigned short, so supplying
more than 65535 hosts wraps the counter. The subsequent calloc(3) then
allocates an undersized hosts array while the later parsing loop still
writes one entry per host, overflowing the heap buffer. As
process_arguments() runs before we drop privileges via setuid(getuid()),
this happens while still running as root on setuid-root installs.
Guard both places where the counter is incremented and bail out with a
usage error once 65535 hosts are reached, rather than wrapping silently.
Reported-by: Christopher Kreft <Email@ChristopherKreft.de>
|
|
OpenBSD's pledge(2) system call allows the current process to
self-restrict itself, being reduced to promised pledges. For example,
unless a process says it wants to write to files, it is not allowed to
do so any longer.
This change starts by calling pledge(2) in some network-facing checks,
removing the more dangerous privileges, such as executing other files.
My initial motivation came from check_icmp, being installed as a setuid
binary and (temporarily) running with root privileges. There, the
pledge(2) calls result in check_icmp to only being allowed to interact
with the network and to setuid(2) to the calling user later on.
Afterwards, I went through my most commonly used monitoring plugins
directly interacting with the network. Thus, I continued with
pledge(2)-ing check_curl - having a huge codebase and all -,
check_ntp_time, check_smtp, check_ssh, and check_tcp.
For most of those, the changes were quite similar: start with
network-friendly promises, parse the configuration, give up file access,
and proceed with the actual check.
|
|
Within np_extra_opts, the ini parser expects a valid progname as the
default section to select a configuration section in the ini file.
However, within the check_icmp codebase, the progname is being populated
directly after the np_extra_opts call, being a null pointer before.
$ ./check_icmp --extra-opts=@foo.ini
Segmentation fault (core dumped)
> #0 strlen () at /usr/src/lib/libc/arch/amd64/string/strlen.S:125
> #1 0x000003989615d032 in _libc_strdup (str=Variable "str" is not available.) at /usr/src/lib/libc/string/strdup.c:44
> #2 0x000003966f751b74 in np_get_defaults (locator=0x73ede1e538ea "@foo.ini", default_section=0x0) at parse_ini.c:91
> #3 0x000003966f7518ce in np_extra_opts (argc=0x73ede1e5369c, argv=0x73ede1e53728, plugin_name=0x0) at extra_opts.c:98
> #4 0x000003966f74165a in main (argc=1, argv=0x0) at check_icmp.c:832
The progname variable is set within the process_arguments function,
requiring the already enriched arguments from np_extra_opts. Thus, I
moved the progname detection out of this function, directly before the
np_extra_opts call. This pattern does already exists in check_tcp.
I briefly looked for similar issues in other plugins, but found none.
|
|
* check_icmp: prevent segfault on OpenBSD
This commit adds a sanity check for sockets in
check_icmp.
Previously FD_ISSET segfaulted when a socket value was
-1 (on OpenBSD). The changes here add an explicit
check whether the socket is -1 (and therefore not
set).
---------
Co-authored-by: Lorenz Kästle <lorenz.kaestle@netways.de>
|
|
|
|
The refactoring in eafee9c3f91879afa82749fa1d8cd2b0b53a5d5c missed the
part within "#if defined(SIOCGIFADDR)" in get_ip_address.
|
|
The ioctl(2) call within "#if defined(SIOCGIFADDR)" requires the include.
|
|
|
|
|
|
This reverts commit 612d261cbf467c82f0dcc0ed63e7584d91f194c4.
|
|
This reverts commit 52338c3423d4b10d2bbeec7a120c7dd2a98fa092.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
This commit switches check_icmp from getopt to getopt_long
to provide long options too and (most importantly) homogenize
option parsing between the different plugins.
|
|
|
|
|
|
|
|
The timeout option was redundant in that the runtime
of check_icmp was always limited by the input parameters
and therefore timeout gets removed with this commit to
avoid that confusion.
The rest of the signal handlings was removed too, since
the added complexity does not provide sufficient returns.
If check_icmp gets a signal, it now dies like most other
programs instead of trying to save some things and return a
(arguably wrong) result.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
