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 replaces the separately implemented client/server
is_base64() check with a single libfko function, and libfko itself now
uses it as well before prepending Rijndael or GnuPG base64 encoded
prefixes.
[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().
- [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.
Added --key-gen to allow KEY_BASE64 and HMAC_KEY_BASE64 keys to be created from
reading random data from /dev/random. These keys can be placed within server
access.conf files and corresponding client .fwknoprc files for SPA
communications. The HMAC key is not used yet with this commit, but that is
coming.
This commit does several things. First, a memory leak in fwknopd has been
fixed by ensuring to free access.conf stanzas. This bug was found with the
new test suite running in --enable-valgrind mode. Here is what some of the
valgrind output looked like to find the leak:
==19217== 11 bytes in 1 blocks are indirectly lost in loss record 3 of 5
==19217== at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==19217== by 0x52F6B81: strdup (strdup.c:43)
==19217== by 0x10FC8B: add_acc_string (access.c:49)
==19217== by 0x1105C8: parse_access_file (access.c:756)
==19217== by 0x10B79B: main (fwknopd.c:194)
==19217==
==19217== 16 bytes in 1 blocks are indirectly lost in loss record 4 of 5
==19217== at 0x4C27480: calloc (vg_replace_malloc.c:467)
==19217== by 0x10FEC0: add_source_mask (access.c:88)
==19217== by 0x110100: expand_acc_source (access.c:191)
==19217== by 0x1104B0: parse_access_file (access.c:500)
==19217== by 0x10B79B: main (fwknopd.c:194)
==19217==
==19217== 183 (152 direct, 31 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 5
==19217== at 0x4C27480: calloc (vg_replace_malloc.c:467)
==19217== by 0x1103E4: parse_access_file (access.c:551)
==19217== by 0x10B79B: main (fwknopd.c:194)
==19217==
==19217== LEAK SUMMARY:
==19217== definitely lost: 152 bytes in 1 blocks
==19217== indirectly lost: 31 bytes in 3 blocks
==19217== possibly lost: 0 bytes in 0 blocks
==19217== still reachable: 8 bytes in 1 blocks
==19217== suppressed: 0 bytes in 0 blocks
Second, this commit changes how fwknopd acquires packet data with
pcap_dispatch() - packets are now processed within the callback function
process_packet() that is provided to pcap_dispatch(), the global packet
counter is incremented by the return value from pcap_dispatch() (since this is
the number of packets processed per pcap loop), and there are two new
fwknopd.conf variables PCAP_DISPATCH_COUNT and PCAP_LOOP_SLEEP to control the
number of packets that pcap_dispatch() should process per loop and the number
of microseconds that fwknopd should sleep per loop respectively. Without this
change, it was fairly easy to cause fwknopd to miss packets by creating bursts
of packets that would all be processed one at time with the usleep() delay
between each. For fwknopd deployed on a busy network and with a permissive
pcap filter (i.e. something other than the default that causes fwknopd to look
at, say, TCP ACK's), this change should help.
Third, the criteria that a packet must reach before data copying into the
buffer designed for SPA processing has been tightened. A packet less than
/greater than the minimum/maximum expected sizes is ignored before data is
copied, and the base64 check is done as well.
Added the 'const' qualifier to function prototype variables where possible.
In addition, reduced some functions to file-scope with 'static' where possible.
Also made a few minor changes to remove extra whitespace, and fixed a bug
in create_fwknoprc() to ensure the new fwknoprc filehandle is closed.
This commit fixes several compiler warnings like the following (now that -Wall
is the default):
config_init.h:68: warning: ‘cmd_opts’ defined but not used
