Bug fix to ensure that proper bounds are enforced when importing digest
cache files from previous fwknopd executions. This bug
was discovered through fuzzing with American Fuzzy Lop (AFL) as driven
by the test/afl/fuzzing-wrappers/server-digest-cache.sh wrapper.
Previous to this fix, fwknopd could be made to crash through a malicious
digest cache file (normally in /var/run/fwknop/digest.cache) upon
initial import.
This commit updates all authorship and copyright information to include a
standard header that references the AUTHORS and CREDITS file. This standard
header was written by the Debian legal team at the request of Franck Joncourt.
This commit fixes a crash if the replay digest init() routine fails - fwknopd
attempted to make use of replay tracking anyway. The crash was discovered
during testing fwknopd with an AppArmor enforce policy deployed. The
following stack trace shows the crash (taken before the previous static
function commit):
Program received signal SIGSEGV, Segmentation fault.
__strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
31 ../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
(gdb) where
#0 __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
#1 0x00007f59cabd8b26 in add_replay_file_cache (opts=opts@entry=0x7fff3eaa0bb0, digest=digest@entry=0x0) at replay_cache.c:516
#2 0x00007f59cabd8cf5 in add_replay (opts=opts@entry=0x7fff3eaa0bb0, digest=digest@entry=0x0) at replay_cache.c:472
#3 0x00007f59cabd62eb in incoming_spa (opts=0x7fff3eaa0bb0) at incoming_spa.c:536
#4 0x00007f59ca56164e in ?? () from /usr/lib/x86_64-linux-gnu/libpcap.so.0.8
#5 0x00007f59cabd7175 in pcap_capture (opts=opts@entry=0x7fff3eaa0bb0) at pcap_capture.c:269
#6 0x00007f59cabd3d4d in main (argc=5, argv=0x7fff3eaa1458) at fwknopd.c:314
This commit is a follow up to Ryman's report (#85) of a potential timing attack
that could be leveraged against fwknop when strncmp() is used to compare HMAC
digests. All strncmp() calls that do similar things have been replaced with a
new constant_runtime_cmp() function that mitigates this problem.
Lots of places in the code were already using {0} to initialize stack char
arrays, but memset() was being used as well. This commit removes all
unnecessary memset() calls against char arrays that are already initialized
via {0} (which sets all members to zero for such arrays).
This commit fixes a minor memory leak for the digest cache file path in
--rotate-digest-cache mode in the replay_cache_init() function. The leak was
caught by valgrind, and a new test was added to the test suite for it. Here
is the valgrind warning:
==29021== 21 bytes in 1 blocks are definitely lost in loss record 2 of 2
==29021== at 0x4C2B3F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29021== by 0x1103AA: replay_cache_init (replay_cache.c:96)
==29021== by 0x10BB8C: main (fwknopd.c:254)
[client+server] Applied patch from Franck Joncourt to remove unnecessary
chmod() call when creating client rc file and server replay cache file.
The permissions are now set appropriately via open(), and at the same
time this patch fixes a potential race condition since the previous code
used fopen() followed by chmod().
Applied patch from Vlad Glagolev to fix ndbm/gdbm usage when --disable-file-cache
is used for the autoconf configure script. This functionality was broken in
be4193d734850fe60f14a26b547525ea0b9ce1e9 through improper handling of #define
macros from --disable-file-cache.
- [client+server] Fernando Arnaboldi from IOActive found that strict
filesystem permissions for various fwknop files are not verified. Added
warnings whenever permissions are not strict enough, and ensured that
files created by the fwknop client and server are only set to user
read/write.
- [client] Fernando Arnaboldi from IOActive found a local buffer overflow
in --last processing with a maliciously constructed ~/.fwknop.run file.
This has been fixed with proper validation of .fwknop.run arguments.
This change ensures that we only cache replay digests for those SPA packets
that actually decrypt. Not doing this would have allowed an attacker to
potentially fill up digest cache space with digests for garbage packets.
This commit fixes a bug where the same encryption key used for two stanzas in
the access.conf file would result in access requests that matched the second
stanza to always be treated as a replay attack. This has been fixed for
the fwknop-2.0.1 release, and was reported by Andy Rowland. Now the fwknopd
server computes the SHA256 digest of raw incoming payload data before
decryption, and compares this against all previous hashes. Previous to this
commit, fwknopd would add a new hash to the replay digest list right after
the first access.conf stanza match, so when SPA packet data matched the
second access.conf stanza a matching replay digest would already be there.
The two memory leaks were found with the test suite running in
--enable-valgrind mode - here are the relevant error messages:
For fwknopd server GPG clean up:
==345== 9 bytes in 1 blocks are definitely lost in loss record 2 of 2
==345== at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==345== by 0x52F6B81: strdup (strdup.c:43)
==345== by 0x10FA57: add_string_list_ent (access.c:308)
==345== by 0x110513: parse_access_file (access.c:387)
==345== by 0x10B5FB: main (fwknopd.c:193)
For fwknop client rc file processing:
==8045== 568 bytes in 1 blocks are still reachable in loss record 12 of 12
==8045== at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==8045== by 0x50A53AA: __fopen_internal (iofopen.c:76)
==8045== by 0x10C3FF: process_rc (config_init.c:446)
==8045== by 0x10C8F6: config_init (config_init.c:671)
==8045== by 0x10AC9E: main (fwknop.c:62)
There is also a new clean_exit() function that makes it easier to ensure that
resources are deallocated upon existing.
This commit fixes the following gcc warning on freebsd systems:
replay_cache.c: In function 'replay_file_cache_init':
replay_cache.c:312: warning: format '%ld' expects type 'long int *', but argument 9 has type 'time_t *'
Bug fix to ensure that the digest.cache file gets created at fwknopd init time
so fwknopd does not throw the following error:
Error opening digest cache file. Incoming digests will not be remembered.
Replay warnings now include port and protocol information. Here is an example:
SPA Packet from IP: 127.0.0.1 received.
Replay detected from source IP: 127.0.0.1
Destination proto/port: 17/62201
Original source IP: 127.0.0.1
Original dst proto/port: 17/62201
Entry created: 08/17/11 21:06:07
First replay: 08/17/11 21:06:32
Last replay: 08/17/11 21:06:45
Replay count: 7
Upon fwknopd shutdown, a new function free_replay_list() is now called in order
to free heap allocated memory dedicated to SPA digest tracking. Without this
fix, valgrind reports the following (some output snipped):
valgrind --leak-check=full ./server/.libs/fwknopd -f -i lo -P "udp port 62201"
==30864== 431 (48 direct, 383 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17
==30864== at 0x4C27480: calloc (vg_replace_malloc.c:467)
==30864== by 0x407CB7: replay_check_file_cache (replay_cache.c:461)
==30864== by 0x407B69: replay_check (replay_cache.c:413)
==30864== by 0x405813: incoming_spa (incoming_spa.c:363)
==30864== by 0x406275: pcap_capture (pcap_capture.c:223)
==30864== by 0x40317D: main (fwknopd.c:297)
Added the source port and protocol fields to valid SPA packets in the digest
cache. This can help to discover replay trends. The format of the digest
file cache is now:
<digest> <proto> <src_ip> <src_port> <dst_ip> <dst_port> <time>
At init time fwknopd will read in the digest cache file into the in-memory
linked list of digests for SPA replay detection. This commit starts on this
code, but the file format does not yet include destination IP addresses
(to be added in an upcoming commit).
When not using gdbm/ndbm support (the default now), fwknopd implements a linked
list of SPA packet digests for replay attack detection along with writing
digest data in ascii text down to disk (in the CONF_DIGEST_FILE file).
If fwknopd is compiled with --disable-file-cache to the ./configure script
then it will assume that the default filename is "digest_db.cache" for the
digest cache. If the file cache method is used (this is the default), then
"digest.cache" is the default filename. A new variable DIGEST_DB_FILE in
the fwknopd.conf file controls the digest filename if gdbm/ndbm support is
required.
This change starts on support for a simple file-based cache mechanism
for tracking SPA digests. This removes the libgdbm/libndbm dependency
by default, but it can be re-enabled with the --disable-file-cache
argument to the ./configure script.
