strlcat() needs to be informed about the actual size of the buffer. Two
calls simply used the size expected, thus potentially allowing
stack-based buffer overflows.
There is no direct security impact in this case, since the code affected
is on the client side, and the input comes from configuration
information.
In fwknop, the values generated using random() are only used for the ID
field of raw IP packets. As indicated in the corresponding comments,
this value does not really matter, and it does not really have to be
random at all.
However, it should not hurt to initialize the entropy pool before
generating random values. arc4random() would be a better choice, but it
is not portable across the range of systems currently supported by
fwknop.
execvp() is (usually) equivalent to execvpe(), without enforcing any
change to the environment. However, unlike execvp(), execvpe() is not
standardized by POSIX, and may therefore not be available nor detected
when configuring the project (like on NetBSD).
No place could be found in fwknop to be using execvpe() and changing the
environment. Therefore it seems only logical (and safer) to use execvp()
instead.
This also updates the tests to reflect this change.
Characters should be casted as unsigned before use in functions from
<ctype.h>. Otherwise the compiler treats 8-bit characters (eg UTF-8) as
negative values (since it expects signed integers) and they no longer
match the comparison tables. Worse, the character 0xff gets interpreted
as -1 (like EOF). In turn, it helps to explicitly cast the result as a
signed integer, since this is what is expected. Characters in the range
0x80-0xff do keep their original values.
See the manual page for ctype(3) for more details (eg from NetBSD)
A trio of checks were done two places, and the code path always went through both checks.
Since the program announced a fail and closed, the second set of checks could never be triggered.
