jvinet / knock

A port-knocking daemon
http://www.zeroflux.org/projects/knock
GNU General Public License v2.0
549 stars 113 forks source link

Issue with uninitialized parameters in door structure for tcpflags #54

Closed TDFKAOlli closed 4 years ago

TDFKAOlli commented 6 years ago

Hi, I noticed a problem with knockd on my Lede router. I use the build for "official" version 0.7 there. I noticed that three configuration sections work fine for me but after the foruth one I see strange pcap filter and problems with setting them:

` Adding pcap expression for door 'NBlock': (dst host 192.168.2.102 and (((tcp dst port 50213 or 51113 or 50432) and tcp[tcpflags] & tcp-syn != 0)))

Adding pcap expression for door 'NFree': (dst host 192.168.2.102 and (((tcp dst port 50432 or 51113 or 50213) and tcp[tcpflags] & tcp-syn != 0)))

Adding pcap expression for door 'PBlock': (dst host 192.168.2.102 and (((tcp dst port 43653 or 32243 or 49433) and tcp[tcpflags] & tcp-syn != 0)))

Adding pcap expression for door 'PFree': (dst host 192.168.2.102 and (((tcp dst port 49433 or 32243 or 43653) and tcp[tcpflags] & tcp-fin and tcp[tcpflags] & tcp-syn != 0 and tcp[tcpflags] & tcp-rst and tcp[tcpflags] & tcp-psh and tcp[tcpflags] & tcp-urg ))) `

As it can be seen the last pcap filter is nonsense. Also all following ones are bad.

I did a quick check through the code and I have seen that when "door" is allocated in parseconfig() (here: https://github.com/jvinet/knock/blob/258a27e5a47809f97c2b9f2751a88c2f94aae891/src/knockd.c#L578), the tcpflags are not initialized. Later when a flag is found, the according tcp_flag varibale is set or unset. But when creating the pcap filter in generate_pcap_filter() it is checked if the flags are set to DONT_CARE. Anyhow as they are never initialized and never set, if not mentioned in the config, there is most likely garbage in the flags. This will add the string path seen in the pcap filter above (e.g. " tcp[tcpflags] & tcp-rst", but missing the "!= 0" or "= 0", as the value also isn't either "SET" or "UNSET".

Fix would be to add:

` door->flag_fin = DONT_CARE;

door->flag_syn = DONT_CARE;

door->flag_rst = DONT_CARE;

door->flag_psh = DONT_CARE;

door->flag_ack = DONT_CARE;

door->flag_urg = DONT_CARE;

`

to the initialization here: https://github.com/jvinet/knock/blob/258a27e5a47809f97c2b9f2751a88c2f94aae891/src/knockd.c#L587

I can create a merge request, but I have no way to test and build at the moment. If somebody else has a working build setup, maybe it can be added, quickly tested and a merge request be made.

Cheers, TDFKAOlli

TDFKAOlli commented 6 years ago

Fixed by pull request #55

TDFKAOlli commented 4 years ago

@jvinet Thanks for the merge! Good you are back on the project.