This commit completes the conversion to the strtol() wrapper function in order
to remove all atoi() calls. In addition, variable max values are enforced
using more broadly defined RCHK_* values.
This commit replaces a few additional atoi() calls with the strtol() wrapper
function, and also fixes a bug where access SOURCE IP/mask combinations would
not be accepted when the string length was a long as something like
'123.123.123.123/255.255.255.255'.
[libfko] Added the ability to maintain backwards compatibility with the
now deprecated "zero padding" strategy in AES mode that was a hold over
from the old perl fwknop implementation. This enables the backwards
compatiblity tests to continue to pass in the test suite.
Fernando Arnaboldi from IOActive found several conditions in
which the server did not properly throw out maliciously constructed
variables in the access.conf file. This has been fixed along with new
fuzzing tests in the test suite.
- [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.
- [server] Fernando Arnaboldi from IOActive found several DoS/code
execution vulnerabilities for malicious fwknop clients that manage to
get past the authentication stage (so a such a client must be in
possession of a valid access.conf encryption key). These vulnerbilities
manifested themselves in the handling of malformed access requests, and
both the fwknopd server code along with libfko now perform stronger input
validation of access request data. These vulnerabilities affect
pre-2.0.3 fwknop releases.
- [test suite] Added a new fuzzing capability to ensure proper server-side
input validation. Fuzzing data is constructed with modified fwknop
client code that is designed to emulate malicious behavior.
For GPG mode, added a new access.conf variable "GPG_ALLOW_NO_PW" to make it
possible to leverage a server-side GPG key pair that has no associated
password. This comes in handy when a system requires the user to leverage
gpg-agent / pinentry which can present a problem in automated environments as
required by the fwknopd server. Now, it might seem like a problem to remove
the passphrase from a GPG key pair, but it's important to note that simply
doing this is little worse than storing the passphrase in the clear on disk
anyway in the access.conf file. Further, this link help provides additional
detail:
http://www.gnupg.org/faq/GnuPG-FAQ.html#how-can-i-use-gnupg-in-an-automated-environment
Now that encryptions keys and hmac keys may be acquired from /dev/random with
--key-gen (and base64 encoded), they may contain NULL bytes. This emphasizes
the need to not leverage code that assumes C-style strings when making use of
key information.
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.
fwknopd access stanzas can have both Rijndael and GnuPG keys, so this
commit fixes a bug where any gpg info would force only gpg decryption
attempts even if a Rijndael key is provided in the stanza.
This commit causes fwknopd to exit whenever an invalid SOURCE entry is seen
such as ":ANY". Previous to this commit, valgrind threw the following errors
with ":ANY" as an access.conf SOURCE entry:
Invalid read of size 8
at 0x117695: free_acc_source_list (access.c:512)
by 0x1177E3: free_acc_stanza_data (access.c:564)
by 0x117C67: free_acc_stanzas (access.c:654)
by 0x10E32E: free_configs (config_init.c:106)
by 0x10D085: main (fwknopd.c:376)
Address 0x5a80658 is 8 bytes inside a block of size 16 free'd
at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x116AE0: add_source_mask (access.c:255)
by 0x116D57: expand_acc_source (access.c:303)
by 0x117A82: expand_acc_ent_lists (access.c:620)
by 0x119570: parse_access_file (access.c:1043)
by 0x10C77E: main (fwknopd.c:193)
Invalid free() / delete / delete[] / realloc()
at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x1176A8: free_acc_source_list (access.c:514)
by 0x1177E3: free_acc_stanza_data (access.c:564)
by 0x117C67: free_acc_stanzas (access.c:654)
by 0x10E32E: free_configs (config_init.c:106)
by 0x10D085: main (fwknopd.c:376)
Address 0x5a80650 is 0 bytes inside a block of size 16 free'd
at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x116AE0: add_source_mask (access.c:255)
by 0x116D57: expand_acc_source (access.c:303)
by 0x117A82: expand_acc_ent_lists (access.c:620)
by 0x119570: parse_access_file (access.c:1043)
by 0x10C77E: main (fwknopd.c:193)
HEAP SUMMARY:
in use at exit: 8 bytes in 1 blocks
total heap usage: 1,659 allocs, 1,659 frees, 238,310 bytes allocated
This is a significant update to allow AES encryption modes to be selected on a
per-key basis. For now, only ECB and CBC (recommended) modes are supported.
The default is ECB modes in order to maintain backwards compatibility with the
older perl version of fwknop and the Crypt::CBC CPAN module. This will likely
be changed to use CBC mode by default because of its better security
properties.
In the access.conf file on the server side, there is a new configuration
variable "ENCRYPTION_MODE" that controls the mode for the corresponding AES
key. On the client side, a new command line argument "--encryption-mode"
controls how the client encrypts SPA packets.
This commit adds a new configuration variable "FORCE_NAT" to the access.conf
file:
For any valid SPA packet, force the requested connection to be NAT'd
through to the specified (usually internal) IP and port value. This is
useful if there are multiple internal systems running a service such as
SSHD, and you want to give transparent access to only one internal system
for each stanza in the access.conf file. This way, multiple external
users can each directly access only one internal system per SPA key.
This commit also implements a few minor code cleanups.
This commit does two major things:
1) Two new access.conf variables are added "ACCESS_EXPIRE" and
"ACCESS_EXPIRE_EPOCH" to allow access stanzas to be expired without having
to modify the access.conf file and restart fwknopd.
2) Allow an access stanza that matches the SPA source address to not
automatically short circuit other stanzas if there is an error (such as when
there are multiple encryption keys involved and an incoming SPA packet is
meant for, say, the second stanza and the first therefore doesn't allow
proper decryption).
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 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.
This commit makes it easier to determine exactly which commands fwknopd
runs in --verbose mode when interacting with the underlying firewall.
This commit also adds --verbose --verbose mode to the test suite.
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.
Added new command line options --fw-list-all and --fw-flush to allow all
firewall rules to be displayed including those not created by fwknopd, and
allow all firewall rules created by fwknopd to be deleted.
Also switched -D config dump output to stdout.
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
