Linux C Port Knock Implementation

Question

Compile with: gcc -std=gnu11 -o "portknock" "portknock.c" -lpcap

I did this as a learning experience, and it actually ended up being something useful. I made a port-knocking spawned reverse TTY with it, and it could easily be made into a port knocking firewall daemon.

#include <stdio.h>
#include <stdlib.h>
#include <pcap.h>
#include <netinet/if_ether.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/ip.h>
#include <netinet/ip6.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <time.h>
#include <string.h>
#include <linux/kd.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>
struct knock {
 struct sockaddr* addr;
 int prog;
};
struct knock** np = NULL;
size_t nps = 0;
uint16_t kxs[] = { 5900, 80, 88, 82, 86 };
size_t kxsn = 5;
int main() {
 if (getuid() != 0) {
 printf("Port Knocking requires root!\n");
 }
 char errbuf[PCAP_ERRBUF_SIZE];
 char *dev = pcap_lookupdev(errbuf);
 if (dev == NULL) {
 printf("pcap_lookupdev %s\n", errbuf);
 exit(1);
 }
 pcap_t* descr = pcap_open_live(dev, 8192, 0, 100, errbuf);
 if (descr == NULL) {
 printf("pcap_open_live %s\n", errbuf);
 exit(1);
 }
 struct pcap_pkthdr hdr;
 struct pcap_pkthdr* hdrp = &hdr;
 const u_char* packet;
 int ret;
 while (1) {
 ret = pcap_next_ex(descr, &hdrp, &packet);
 if (ret < 0) {
 printf("pcap receive error, %s\n", pcap_geterr(descr));
 exit(1);
 } else if (ret == 0) {
 //printf("timeout!\n");
 continue;
 }
 const struct ether_header *eptr = (struct ether_header *) packet;
 uint16_t et = ntohs(eptr->ether_type);
 u_int size_ip;
 struct sockaddr* from;
 struct sockaddr_in ip4;
 struct sockaddr_in6 ip6;
 if (et == ETHERTYPE_IP) {
 const struct iphdr *ip = (struct iphdr*) (packet + ETHER_HDR_LEN);
 size_ip = ip->ihl * 4;
 if (size_ip < 20) {
 printf("bad ip header size: %i\n", size_ip);
 continue;
 }
 if (ip->protocol != 6) continue;
 from = (struct sockaddr*) &ip4;
 ip4.sin_family = AF_INET;
 ip4.sin_addr.s_addr = ip->saddr;
 } else if (et == ETHERTYPE_IPV6) {
 const struct ip6_hdr *ip = (struct ip6_hdr*) (packet + ETHER_HDR_LEN);
 size_ip = 40;
 uint8_t et = ip->ip6_ctlun.ip6_un1.ip6_un1_nxt;
 if (et == 0 || et == 60 || et == 43 || et == 44 || et == 51 || et == 50 || et == 60 || et == 135) {
 struct ip6_ext *ext = (struct ip6_ext*) (packet + ETHER_HDR_LEN + size_ip);
 size_ip += 8 + (ext->ip6e_len * 8);
 et = ext->ip6e_nxt;
 while (et == 0 || et == 60 || et == 43 || et == 44 || et == 51 || et == 50 || et == 60 || et == 135) {
 size_ip += 8 + (ext->ip6e_len * 8);
 ext = (struct ip6_ext*) (packet + ETHER_HDR_LEN + size_ip);
 }
 if (et == 59) continue;
 } else if (et == 59) continue;
 if (et != 6) continue;
 from = (struct sockaddr*) &ip6;
 ip6.sin6_family = AF_INET6;
 ip6.sin6_addr = ip->ip6_src;
 //printf("ipv6 size: %u\n", size_ip);
 } else continue;
 const struct tcphdr *tcp = (struct tcphdr*) (packet + ETHER_HDR_LEN + size_ip);
 u_int size_tcp = tcp->th_off * 4;
 if (size_tcp < 20) {
 printf("bad tcp header size: %i\n", size_tcp);
 continue;
 }
 //const u_char* data = (u_char *) (packet + ETHER_HDR_LEN + size_ip + size_tcp);
 if ((tcp->th_flags & TH_SYN) == TH_SYN) {
 //char* add;
 //char ip6a[64];
 //if (from->sa_family == AF_INET) {
 // add = inet_ntoa(ip4.sin_addr);
 //} else if (from->sa_family == AF_INET6) {
 // add = inet_ntop(AF_INET6, &ip6.sin6_addr, ip6a, 64);
 //}
 uint16_t dport = ntohs(tcp->th_dport);
 //uint16_t sport = ntohs(tcp->th_sport);
 //printf("%s sent a syn to port dest = %u, source = %u\n", add, dport, sport);
 struct knock* cn = NULL;
 for (int i = 0; i < nps; i++) {
 if (np[i]->addr->sa_family == AF_INET && from->sa_family == AF_INET) {
 if (((struct sockaddr_in*) (np[i]->addr))->sin_addr.s_addr == ip4.sin_addr.s_addr) {
 cn = np[i];
 }
 }
 }
 int cp = cn == NULL ? 0 : cn->prog;
 if (dport == kxs[cp]) {
 if (++cp == kxsn) {
 printf("knock complete!\n");
 //do knock stuff, ie accept a connection from an IP, by interfacing with libiptc
 } else {
 printf("knock progress = %i!\n", cp);
 if (cn == NULL) {
 cn = malloc(sizeof(struct knock));
 if (cn == NULL) {
 printf("out of memory!\n");
 exit(1);
 }
 cn->prog = 1;
 size_t s = from->sa_family == AF_INET ? sizeof(ip4) : sizeof(ip6);
 cn->addr = malloc(s);
 if (cn->addr == NULL) {
 printf("out of memory!\n");
 exit(1);
 }
 memcpy(cn->addr, from, s);
 if (np == NULL) {
 np = malloc(sizeof(struct knock*));
 nps = 0;
 } else {
 np = realloc(np, sizeof(struct knock*) * (nps + 1));
 }
 if (np == NULL) {
 printf("out of memory!\n");
 exit(1);
 }
 np[nps++] = cn;
 } else {
 cn->prog++;
 }
 }
 }
 }
 }
 exit(0);
 return EXIT_SUCCESS;
}

• Der Kommissar: Why do you have et == 60 twice on the et == 0 || et == 60 || et == 43 || et == 44 || et == 51 || et == 50 || et == 60 || et == 135 lines?
• JavaProphet: @EBrown I was implementing a list of IPv6 addon headers, and I probably read the same value twice.

Answer 1

• Magic numbers

  I think it is not a coincidence that kxsn is 5 and kxs has 5 elements. A c idiom is

   size_t kxsn = sizeof(kxs)/sizeof(kxs[0]);
  

  It would be very helpful to explain (preferably via named constants) what do values of kxs signify. Same goes for

   et == 0 || et == 60 || et == 43 || et == 44 || et == 51 || et == 50 || et == 60 || et == 135
  

  (who are those guys?).

• Naming

  is honestly meaningless. np, cn, cp convey no information to the reader.

• Memory is never reclaimed

  Allocated memory is never freed. The daemon is doomed to run out of memory (and before that it will bring the whole system to the crawl).

• What about AF_INET6?

  AF_INET6 packets just clog the np array. Their addresses would never be found in np, so a cn will always be allocated anew and added to np. If I understand correctly, 5 times per connection.

• Functions

  As @looserdroog mentioned, logics must be separated into functions. It is not a matter of repeated code, but about code cohesion and separation of responsibilities. Besides, a function is something you may give a nice explanatory name to.

  For example, the body of a main loop may look like

   while (1) {
   get_packet(descr, &hdrp, &packet);
   struct sockaddr * from = parse_address(packet, &size_ip);
   if (from == NULL) continue;
   struct tcphdr * tcp = get_tcp_header(packet, size_ip);
   if (tcp == NULL || !is_syn(tcp)) continue;
   uint16_t dport = tcp->th_dport;
   struct knock * knock = find_knock(known_knocks, from);
   if (knock == NULL) {
   knock = allocate_knock(from);
   add_knock(knock, known_knocks);
   }
   if (dport == kxs[knock->progress]) {
   knock->progress++;
   if (knock_completed(knock)) {
   do_stuff(knock);
   }
   }
   }
  

• JavaProphet: Does sizeof(array) actually work? I thought it wouldn't because sizeof is compiletime. The values of ET are IPv6 extra header types that carry further headers(ie non transport-layer protocol TCP/UDP). I get your point though. Thanks for all the critique, it's much appreciated.
• vnp: @JavaProphet The array (and hence its size) is known at compile time. The sizeof()/sizeof() trick is widely known and very useful as long as the array is seen as array (meaning it didn't decay into a pointer when passed to function).

Answer 2

Pick one:

exit(0);
return EXIT_SUCCESS;

Aside from that, I don't really know about socket programming so I can't critique what you're doing in there. But it would almost certainly be an improvement to separate out the logic into several smaller functions. It is very advantageous to be able to view an entire function on one screen. It can be more easily comprehended as a single unit. As a side-benefit, it naturally reduces the number of indentation levels you need.

The main function should only

• process command-line arguments
• call initialization functions
• call "workhorse" function

Now it may be that your "workhorse" here still contains most of the code, and maybe that's ok for what it needs to do. But remember C's primary means of abstraction is functions (and macros), so use them (but don't go crazy with macros).

Quoting advice once given to me,

Try and remove chunks into separate functions that

1. have a single entrance and exit
2. perform a single function well

pass parameters, don't use global data Nick Keighley

This advice is, of course, recursive. Thus it's a terrible definition for pedagogical purposes, but once you "get it", this advice sums it up nicely IMO. Functional abstraction is one of the most difficult aspects of any programming language because it has nothing to do with the language per se, but the conceptual tools you bring to the situation. Writing small functions helps you to better conceptualize the problem you're solving. And a more clear conceptualization will naturally express itself through simple atomic constructs combined in powerful ways.

This is part of the Unix/C philosophy that even a whole program should act like a "single function" so it can be composed with other programs via shell programming. So internally as well, there should be operations (functions) with names like open_connection, close_connection, send_payload_data. For inspiration take a look at "Literate Programming" which takes functional abstraction to a bizarre extreme. (Ideas are often easier to grasp from their more extreme manifestations.)

• JavaProphet: On the exit matter, that was an accident, I created a concise runnable from another project. How would you recommend I split it up into functions? I usually do by repetition, for example if I use the roughly equal code twice.
• luser droog: The best source I can come up with is the thread where I learned the true value if organizing code into (small) functions: embarrassing spaghetti code needs stylistic advice. Decomposition into (sub-) functions is a vital tool in both top-down and bottom-up design.