Segfault under OpenBSD 5.6/i386

[vaygr]

I've found an issue in knockd after upgrading from OpenBSD 5.5 to 5.6 on i386. It started to crash on every second knock try.

Backtrace is here (running with '-D' flag):

----
2014-10-19 00:09:08: XXp: 195.9.XXX.XXX:36396 -> 85.30.XXX.XXX:M 74 bytes
2014-10-19 00:09:08: XXp: 195.9.XXX.XXX:38268 -> 85.30.XXX.XXX:N 60 bytes
2014-10-19 00:09:08: XXp: 195.9.XXX.XXX:34122 -> 85.30.XXX.XXX:P 74 bytes
[New process 4729]
0
2014-10-19 00:09:09: tcp: 195.9.XXX.XXX:36398 -> 85.30.XXX.XXX:M 74 bytes

removing successful knock attempt (195.9.XXX.XXX)

Program received signal SIGSEGV, Segmentation fault.
sniff (arg=0x0, hdr=0x7fd27000, packet=0x7fd27012 "") at src/knockd.c:1444
1444                    attempt = (knocker_t*)lp->data;
(gdb) bt
#0  sniff (arg=0x0, hdr=0x7fd27000, packet=0x7fd27012 "") at src/knockd.c:1444
#1  0x0a3a52cd in pcap_read (p=0x8440a000, cnt=-1, callback=0x1ab20550 <sniff>, user=0x0) at /usr/src/lib/libpcap/pcap-bpf.c:188
#2  0x0a3a3bed in pcap_dispatch (p=0x8440a000, cnt=-1, callback=0x1ab20550 <sniff>, user=0x0) at /usr/src/lib/libpcap/pcap.c:59
#3  0x1ab2114d in main (argc=536921060, argv=0x81944200) at src/knockd.c:270
----

After some digging I've found these lines:

list_free(lp);
continue;

But shouldn't we use 'break' here to prevent further reference to already freed 'lp' (and therefore 'lp->next') in the next iteration of 'for' cycle:

for(lp = attempts; lp; lp = lp->next)

after we totally free 'lp' recursively in list.c:

if(list->next != NULL) {
        list_free(list->next);
}

? Or did I miss/misunderstand something?

knock is compiled by GCC 4.2.1 with these flags: '-g -Wall -pedantic -fno-exceptions -O2 -pipe'.

[vaygr]

As of testing with default config on Linux (x86_64), Valgrind returns this:

2014-10-21 20:19:02: tcp: 10.0.1.3:34245 -> 10.0.1.6:7000 60 bytes
removing successful knock attempt (10.0.1.3)
==16857== Invalid read of size 8
==16857==    at 0x4047A4: sniff (knockd.c:1442)
==16857==    by 0x4E322A1: ??? (in /usr/lib/libpcap.so.1.2.1)
==16857==    by 0x401D71: main (knockd.c:270)
==16857==  Address 0x53deda0 is 16 bytes inside a block of size 24 free'd
==16857==    at 0x4C24492: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16857==    by 0x4047A3: sniff (knockd.c:1475)
==16857==    by 0x4E322A1: ??? (in /usr/lib/libpcap.so.1.2.1)
==16857==    by 0x401D71: main (knockd.c:270)
==16857==

which exactly points knockd.c:1442 to:

        for(lp = attempts; lp; lp = lp->next) {

and knockd.c:1475 to:

                        list_free(lp);

in source release 0.7.

[skyglobe]

I've added some comments in the code and I tried to fix a possible NULL pointer free. Unfortunately I don't have a machine with OpenBSD 5.6 installed (I will fix this soon). Can you please check if my changes had some (hopefully positive) effect?

[vaygr]

nope, it stopped working after the first attempt. No crashes, as well no activity.

[vaygr]

oh, wait, no. I missed the log entry. It crashes exactly the same way -- on the 2nd attempt.

[skyglobe]

Ok, it seems that this bug cannot be replicated on Linux so I have to set up a box with OpenBSD 5.6 and start hacking!

2014-12-05 12:34 GMT+01:00 Vlad Glagolev notifications@github.com:

oh, wait, no. I missed the log entry. It crashes exactly the same way -- on the 2nd attempt.

— Reply to this email directly or view it on GitHub.

[skyglobe]

I still don't have a physical box to try knockd so I've worked with OpenBSD 5.6 RELEASE under VirtualBox. Please tell me if it works now. By the way: I've had to add an ifdef __linux/endif and another include to compile knockd under OpenBSD. So far Debian Stable compiles everything but I'll be glad to ear from users of other OSes and/or other GNU/Linux distros if everything's fine.

[vaygr]

Oh, sorry, I've forgotten to mention that. But instead I used this diff:

--- src/knockd.c.orig   Tue Jun 10 09:42:04 2014
+++ src/knockd.c        Fri Nov 21 14:09:44 2014
@@ -32,12 +32,12 @@
 #include <sys/socket.h>
 #include <netinet/in_systm.h>
 #include <netinet/in.h>
-#include <netinet/if_ether.h>
 #include <netinet/ip.h>
 #include <netinet/tcp.h>
 #include <netinet/udp.h>
 #include <netinet/ip_icmp.h>
 #include <net/if.h>
+#include <netinet/if_ether.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <sys/ioctl.h>

and shouldn't we use DLT_PPP instead of ifdef'ing DLT_LINUX_SLL? Or it's absolutely another device on the OpenBSD platform?

[vaygr]

p.s. confirming the above patch resolves the issue

[skyglobe]

Thanks for the diff: I have changed the order of the includes and I've fixed a possible memory leak. Right now I'm unable to compile everything on OpenBSD (my laptop is too old to handle a VM) but tomorrow I'll check again and if everything's ok I'll issue a pull request.

[airwoflgh]

I've bundled the fixes under Issue #16 into my fork.