Bug fix discovered with the libfiu fault injection tag
"fko_get_username_init" combined with valgrind analysis. This bug
is only triggered after a valid authenticated and decrypted SPA
packet is sniffed by fwknopd:
==11181== Conditional jump or move depends on uninitialised value(s)
==11181== at 0x113B6D: incoming_spa (incoming_spa.c:707)
==11181== by 0x11559F: process_packet (process_packet.c:211)
==11181== by 0x5270857: ??? (in /usr/lib/x86_64-linux-gnu/libpcap.so.1.4.0)
==11181== by 0x114BCC: pcap_capture (pcap_capture.c:270)
==11181== by 0x10F32C: main (fwknopd.c:195)
==11181== Uninitialised value was created by a stack allocation
==11181== at 0x113476: incoming_spa (incoming_spa.c:294)
When validating access.conf stanzas make sure that one of
GPG_REMOTE_ID or GPG_FINGERPRINT_ID is specified whenever GnuPG
signatures are to be verified for incoming SPA packets. Signature
verification is the default, and can only be disabled with
GPG_DISABLE_SIG but this is NOT recommended.
Add a new GPG_FINGERPRINT_ID variable to the access.conf file
so that full GnuPG fingerprints can be required for incoming SPA packets
in addition to the appreviated GnuPG signatures listed in GPG_REMOTE_ID.
From the test suite, an example fingerprint is
GPG_FINGERPRINT_ID 00CC95F05BC146B6AC4038C9E36F443C6A3FAD56
This commit fixes the following leak found by valgrind:
==6241== 568 bytes in 1 blocks are still reachable in loss record 1 of 1
==6241== at 0x4C2A2DB: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6241== by 0x551537A: __fopen_internal (iofopen.c:73)
==6241== by 0x118C8E: parse_access_file (access.c:1143)
==6241== by 0x10F134: main (fwknopd.c:250)
Using the 'fiu-run' fault injection binary, a couple of cases were
turned up with libfko does not properly check the strdup() return value.
This commit fixes these issues, and here is an illustration of the stack
trace for one such issue:
Core was generated by `../client/.libs/fwknop -A tcp/22 -a 127.0.0.2 -D
127.0.0.1 --get-key local_spa.'.
Program terminated with signal 11, Segmentation fault.
#0 __strnlen_sse2 () at ../sysdeps/x86_64/multiarch/../strnlen.S:34
34 ../sysdeps/x86_64/multiarch/../strnlen.S: No such file or directory.
(gdb) where
#0 __strnlen_sse2 () at ../sysdeps/x86_64/multiarch/../strnlen.S:34
#1 0x00007effa38189bc in _rijndael_encrypt (enc_key_len=<optimized out>, enc_key=<optimized out>, ctx=0x7effa5945750) at fko_encryption.c:141
#2 fko_encrypt_spa_data (ctx=0x7effa5945750, enc_key=<optimized out>, enc_key_len=<optimized out>) at fko_encryption.c:605
#3 0x00007effa381a2d6 in fko_spa_data_final (ctx=0x7effa5945750, enc_key=enc_key@entry=0x7fff3ff4aa10 "fwknoptest", enc_key_len=<optimized out>, hmac_key=hmac_key@entry=0x7fff3ff4aaa0 "", hmac_key_len=0) at fko_funcs.c:489
#4 0x00007effa405f2fb in main (argc=<optimized out>, argv=<optimized out>) at fwknop.c:449
This commit changes how fko_new() deals with FKO context initialization
to not set ctx->initval back to zero (uninitialized) imediately after
calling each fko_set_... function and before checking the fko_set_... return
value. The reason for this change is that fko_destroy() checks for
context initialization via ctx->initval before calling free() against
any heap allocated context member. So, if fko_set_... returns an error,
fko_destroy() (previous to this commit) would have no opportunity to
free such members.
This bug was found with fault injection testing provided by libfiu
together with valgrind. Specifically the following test suite command
exposes the problem (from the test/ directory):
./test-fwknop.pl --enable-complete --include "fault injection.*libfko"
In the resulting output/2.test file valgrind reports the following:
==27941== LEAK SUMMARY:
==27941== definitely lost: 264 bytes in 1 blocks
==27941== indirectly lost: 28 bytes in 3 blocks
==27941== possibly lost: 0 bytes in 0 blocks
==27941== still reachable: 1,099 bytes in 12 blocks
==27941== suppressed: 0 bytes in 0 blocks
After this commit is applied, this changes to:
==7137== LEAK SUMMARY:
==7137== definitely lost: 0 bytes in 0 blocks
==7137== indirectly lost: 0 bytes in 0 blocks
==7137== possibly lost: 0 bytes in 0 blocks
==7137== still reachable: 1,099 bytes in 12 blocks
==7137== suppressed: 0 bytes in 0 blocks
Note that 'definitely lost' in valgrind output means there is a real
memory leak that needs to be fixed whereas 'still reachable' is most
likely not a real problem according to:
http://valgrind.org/docs/manual/faq.html#faq.deflost
