Fixed fwknopd memory leak, several other fixes and updates
authorMichael Rash <mbr@cipherdyne.org>
Fri, 4 Nov 2011 02:15:19 +0000 (22:15 -0400)
committerMichael Rash <mbr@cipherdyne.org>
Fri, 4 Nov 2011 02:15:19 +0000 (22:15 -0400)
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.

13 files changed:
doc/fwknopd.man.asciidoc
server/access.c
server/access.h
server/cmd_opts.h
server/config_init.c
server/fwknopd.conf
server/fwknopd_common.h
server/incoming_spa.c
server/pcap_capture.c
server/process_packet.c
server/utils.c
server/utils.h
test/test-fwknop.pl

index b89503e..f25a9d6 100644 (file)
@@ -146,9 +146,9 @@ FWKNOPD.CONF VARIABLES
 ~~~~~~~~~~~~~~~~~~~~~~
 This section list the more prominent configuration variables used by
 *fwknopd*.  It is not a complete list.  There are directives for the type
-of firewall used by *fwknopd* (i.e. _iptables_ or _ipfw_).  You will want
-to make sure to check these to make sure they have appropriate values.  See
-the 'fwknopd.conf' file for the full list and corresponding details.
+of firewall used by *fwknopd* (i.e. _iptables_, _ipfw_, or _pf_).  You will
+want to make sure to check these to make sure they have appropriate values.
+See the 'fwknopd.conf' file for the full list and corresponding details.
 
 *PCAP_INTF* '<interface>'::
     Specify the ethernet interface on which *fwknopd* will sniff packets.
@@ -203,7 +203,7 @@ the 'fwknopd.conf' file for the full list and corresponding details.
     In some situations, internal systems may not have a route back out for
     the source address of the incoming connection, so it is necessary to
     also apply SNAT rules so that the internal systems see the IP of the
-    internal interface where *fwknopd* is running.  
+    internal interface where *fwknopd* is running.
 
 *SNAT_TRANSLATE_IP* '<ip_address>'::
     Specify the IP address for SNAT.  This functionality is only enabled
@@ -238,9 +238,9 @@ the 'fwknopd.conf' file for the full list and corresponding details.
     the '$HOME/.gnupg' directory of the user running *fwknopd* (most
     likely root).
 
-*LOCALE* '<locale>':: 
+*LOCALE* '<locale>'::
     Set the locale (via the LC_ALL variable).  This can be set to override
-    the default system locale. 
+    the default system locale.
 
 *ENABLE_SPA_OVER_HTTP* '<Y/N>'::
     Allow *fwknopd* to acquire SPA data from HTTP requests (generated with
@@ -256,10 +256,24 @@ the 'fwknopd.conf' file for the full list and corresponding details.
      accept incoming TCP request.  This server only accepts the
      request.  It does not otherwise communicate. This is only to allow the
      incoming SPA over TCP packet which is detected via PCAP. The connection
-     is closed after 1 second regardless. 
+     is closed after 1 second regardless.
      Note that fwknopd still only gets its data via pcap, so the filter
      defined by PCAP_FILTER needs to be updated to include this TCP port.
 
+*PCAP_DISPATCH_COUNT* '<count>'::
+    Sets the number of packets that are processed when the *pcap_dispatch()*
+    call is made.  The default is zero, since this allows *fwknopd* to process
+    as many packets as possible in the corresponding callback where the SPA
+    handling routine is called for packets that pass a set of prerequisite
+    checks.  However, if *fwknopd* is running on a platform with an old
+    version of libpcap, it may be necessary to change this value to a positive
+    non-zero integer.  More information can be found in the *pcap_dispatch(3)*
+    man page.
+
+*PCAP_LOOP_SLEEP* '<microseconds'::
+    Sets the number of microseconds to passed as an argument to usleep() in
+    the pcap loop.  The default is 10000, or 1/10th of a second.
+
 *TCPSERV_PORT* '<port>'::
     Set the port number that the ``dummy'' TCP server listens on. This server
     is only spawned when ``ENABLE_TCP_SERVER'' is set to ``Y''.
@@ -290,7 +304,7 @@ directive starts a new stanza.
     This defines the source address from which the SPA packet will be
     accepted.  The string ``ANY'' is also accepted if a valid SPA packet
     should be honored from any source IP.  Every authorization stanza in
-    'access.conf' definition must start with the ``SOURCE'' keyword. 
+    'access.conf' definition must start with the ``SOURCE'' keyword.
     Networks should be specified in CIDR notation (e.g. ``192.168.10.0/24''),
     and individual IP addresses can be specified as well.  Also, multiple
     IP's and/or networks can be defined as a comma separated list (e.g.
index 7b5204f..3eae40e 100644 (file)
@@ -438,12 +438,13 @@ free_acc_string_list(acc_string_list_t *stl)
 /* Free any allocated content of an access stanza.
  *
  * NOTE: If a new access.conf parameter is created, and it is a string
- *       value, it also needs to be added to the list if items to check
+ *       value, it also needs to be added to the list of items to check
  *       and free below.
 */
 static void
 free_acc_stanza_data(acc_stanza_t *acc)
 {
+
     if(acc->source != NULL)
     {
         free(acc->source);
@@ -468,6 +469,9 @@ free_acc_stanza_data(acc_stanza_t *acc)
     if(acc->cmd_exec_user != NULL)
         free(acc->cmd_exec_user);
 
+    if(acc->require_username != NULL)
+        free(acc->require_username);
+
     if(acc->gpg_home_dir != NULL)
         free(acc->gpg_home_dir);
 
@@ -516,11 +520,8 @@ expand_acc_ent_lists(fko_srv_options_t *opts)
     }
 }
 
-/* Take an index and a string value. malloc the space for the value
- * and assign it to the array at the specified index.
-*/
-static void
-acc_stanza_init(fko_srv_options_t *opts)
+void
+free_acc_stanzas(fko_srv_options_t *opts)
 {
     acc_stanza_t    *acc, *last_acc;
 
@@ -541,6 +542,20 @@ acc_stanza_init(fko_srv_options_t *opts)
     return;
 }
 
+/* Wrapper for free_acc_stanzas(), we may put additional initialization
+ * code here.
+*/
+static void
+acc_stanza_init(fko_srv_options_t *opts)
+{
+    /* Free any resources first (in case of reconfig). Assume non-NULL
+     * entry needs to be freed.
+    */
+    free_acc_stanzas(opts);
+
+    return;
+}
+
 /* Add a new stanza bay allocating the required memory at the required
  * location, yada-yada-yada.
 */
@@ -828,7 +843,7 @@ parse_access_file(fko_srv_options_t *opts)
         {
             add_acc_string(&(curr_acc->gpg_remote_id), val);
         }
-        else 
+        else
         {
             fprintf(stderr,
                 "*Ignoring unknown access parameter: '%s' in %s\n",
@@ -863,6 +878,7 @@ parse_access_file(fko_srv_options_t *opts)
 
     /* Expand our the expandable fields into their respective data buckets.
     */
+
     expand_acc_ent_lists(opts);
 
     /* Make sure default values are set where needed.
index 210fd93..b21b3dd 100644 (file)
@@ -42,6 +42,7 @@ int acc_check_port_access(acc_stanza_t *acc, char *port_str);
 int acc_check_gpg_remote_id(acc_stanza_t *acc, const char *gpg_id);
 void dump_access_list(const fko_srv_options_t *opts);
 void expand_acc_port_list(acc_port_list_t **plist, char *plist_str);
+void free_acc_stanzas(fko_srv_options_t *opts);
 void free_acc_port_list(acc_port_list_t *plist);
 
 #endif /* ACCESS_H */
index 59b2879..68e59bb 100644 (file)
@@ -44,6 +44,8 @@ static char *config_map[NUMBER_OF_CONFIG_ENTRIES] = {
     "PCAP_INTF",
     "ENABLE_PCAP_PROMISC",
     "PCAP_FILTER",
+    "PCAP_DISPATCH_COUNT",
+    "PCAP_LOOP_SLEEP",
     "MAX_SNIFF_BYTES",
     "ENABLE_SPA_PACKET_AGING",
     "MAX_SPA_PACKET_AGE",
index 75c49de..349f486 100644 (file)
@@ -30,6 +30,7 @@
 */
 #include "fwknopd_common.h"
 #include "config_init.h"
+#include "access.h"
 #include "cmd_opts.h"
 #include "utils.h"
 #include "log_msg.h"
@@ -102,6 +103,8 @@ free_configs(fko_srv_options_t *opts)
 {
     int i;
 
+    free_acc_stanzas(opts);
+
     for(i=0; i<NUMBER_OF_CONFIG_ENTRIES; i++)
         if(opts->config[i] != NULL)
             free(opts->config[i]);
@@ -282,6 +285,18 @@ validate_options(fko_srv_options_t *opts)
         set_config_entry(opts, CONF_ENABLE_PCAP_PROMISC,
             DEF_ENABLE_PCAP_PROMISC);
 
+    /* The packet count argument to pcap_dispatch()
+    */
+    if(opts->config[CONF_PCAP_DISPATCH_COUNT] == NULL)
+        set_config_entry(opts, CONF_PCAP_DISPATCH_COUNT,
+            DEF_PCAP_DISPATCH_COUNT);
+
+    /* Microseconds to sleep between pcap loop iterations
+    */
+    if(opts->config[CONF_PCAP_LOOP_SLEEP] == NULL)
+        set_config_entry(opts, CONF_PCAP_LOOP_SLEEP,
+            DEF_PCAP_LOOP_SLEEP);
+
     /* PCAP Filter.
     */
     if(opts->config[CONF_PCAP_FILTER] == NULL)
index a2edde7..a9a6046 100644 (file)
 #
 #ENABLE_DIGEST_PERSISTENCE   Y;
 
+# Sets the number of packets that are processed when the *pcap_dispatch()*
+# call is made.  The default is zero, since this allows *fwknopd* to process
+# as many packets as possible in the corresponding callback where the SPA
+# handling routine is called for packets that pass a set of prerequisite
+# checks.  However, if *fwknopd* is running on a platform with an old
+# version of libpcap, it may be necessary to change this value to a positive
+# non-zero integer.  More information can be found in the *pcap_dispatch(3)*
+# man page.
+#PCAP_DISPATCH_COUNT            0;
+
+# Sets the number of microseconds to passed as an argument to usleep() in
+# the pcap loop.  The default is 10000, or 1/10th of a second.
+#PCAP_LOOP_SLEEP                10000;
+
 # Allow SPA clients to request access to services through an iptables
 # firewall instead of just to it (i.e. access through the FWKNOP_FORWARD
 # chain instead of the INPUT chain).
index 1af650c..e26a2fb 100644 (file)
@@ -85,6 +85,8 @@
 #define DEF_INTERFACE                   "eth0"
 #define DEF_ENABLE_PCAP_PROMISC         "N"
 #define DEF_PCAP_FILTER                 "udp port 62201"
+#define DEF_PCAP_DISPATCH_COUNT         "0"
+#define DEF_PCAP_LOOP_SLEEP             "10000" /* a tenth of a second */
 #define DEF_ENABLE_SPA_PACKET_AGING     "Y"
 #define DEF_MAX_SPA_PACKET_AGE          "120"
 #define DEF_ENABLE_DIGEST_PERSISTENCE   "Y"
@@ -162,6 +164,8 @@ enum {
     CONF_PCAP_INTF,
     CONF_ENABLE_PCAP_PROMISC,
     CONF_PCAP_FILTER,
+    CONF_PCAP_DISPATCH_COUNT,
+    CONF_PCAP_LOOP_SLEEP,
     CONF_MAX_SNIFF_BYTES,
     CONF_ENABLE_SPA_PACKET_AGING,
     CONF_MAX_SPA_PACKET_AGE,
index 92ba03e..7e65549 100644 (file)
@@ -56,35 +56,24 @@ preprocess_spa_data(fko_srv_options_t *opts, const char *src_ip)
     int      pkt_data_len = spa_pkt->packet_data_len;
     int      i;
 
-    /* At this point, we can reset the packet data length to 0.  This our
+    /* At this point, we can reset the packet data length to 0.  This is our
      * indicator to the rest of the program that we do not have a current
      * spa packet to process (after this one that is).
     */
     spa_pkt->packet_data_len = 0;
 
-    /* Expect the data to be at least the minimum required size.
-    */
-    if(pkt_data_len < MIN_SPA_DATA_SIZE)
-        return(SPA_MSG_LEN_TOO_SMALL);
-
-    /* Detect and parse out SPA data from an HTTP reqest. If the SPA data
+    /* Detect and parse out SPA data from an HTTP request. If the SPA data
      * starts with "GET /" and the user agent starts with "Fwknop", then
      * assume it is a SPA over HTTP request.
     */
-    if(strncasecmp(ndx, "GET /", 5) == 0
+    if(strncasecmp(opts->config[CONF_ENABLE_SPA_OVER_HTTP], "N", 1) == 0
+      && strncasecmp(ndx, "GET /", 5) == 0
       && strstr(ndx, "User-Agent: Fwknop") != NULL)
     {
         /* This looks like an HTTP request, so let's see if we are
          * configured to accept such request and if so, find the SPA
          * data.
         */
-        if(strncasecmp(opts->config[CONF_ENABLE_SPA_OVER_HTTP], "N", 1) == 0)
-        {
-            log_msg(LOG_WARNING,
-                "HTTP request from %s detected, but not enabled.", src_ip
-            );
-            return(SPA_MSG_HTTP_NOT_ENABLED);
-        }
 
         /* Now extract, adjust (convert characters translated by the fwknop
          * client), and reset the SPA message itself.
@@ -102,25 +91,16 @@ preprocess_spa_data(fko_srv_options_t *opts, const char *src_ip)
                 *ndx = '+';
             else if(*ndx == '_') /* Convert '_' to '/' */
                 *ndx = '/';
-            /* Make sure it is a valid base64 char. */
-            else if(!(isalnum(*ndx) || *ndx == '/' || *ndx == '+' || *ndx == '='))
-                return(SPA_MSG_NOT_SPA_DATA);
 
             ndx++;
         }
     }
-    else
-    {
-        /* Make sure the data is valid Base64-encoded characters
-        */
-        ndx = (char *)spa_pkt->packet_data;
-        for(i=0; i<pkt_data_len; i++)
-        {
-            if(!(isalnum(*ndx) || *ndx == '/' || *ndx == '+' || *ndx == '='))
-                return(SPA_MSG_NOT_SPA_DATA);
-            ndx++;
-        }
-    }
+
+    /* Require base64-encoded data
+    */
+    if(! is_base64(spa_pkt->packet_data, pkt_data_len))
+        return(SPA_MSG_NOT_SPA_DATA);
+
 
     /* --DSS:  Are there other checks we can do here ??? */
 
@@ -206,7 +186,12 @@ incoming_spa(fko_srv_options_t *opts)
     */
     res = preprocess_spa_data(opts, spadat.pkt_source_ip);
     if(res != FKO_SUCCESS)
+    {
+        if(opts->verbose > 1)
+            log_msg(LOG_INFO, "preprocess_spa_data() returned error %i: '%s' for incoming packet.",
+                res, get_errstr(res));
         return(res);
+    }
 
     log_msg(LOG_INFO, "SPA Packet from IP: %s received.", spadat.pkt_source_ip);
 
index d3dd77e..ebdbc93 100644 (file)
@@ -33,7 +33,6 @@
 #include "fwknopd_common.h"
 #include "pcap_capture.h"
 #include "process_packet.h"
-#include "incoming_spa.h"
 #include "sig_handler.h"
 #include "fw_util.h"
 #include "log_msg.h"
@@ -221,24 +220,18 @@ pcap_capture(fko_srv_options_t *opts)
                 got_signal = 0;
         }
 
-        res = pcap_dispatch(pcap, 1, (pcap_handler)&process_packet, (unsigned char *)opts);
+        res = pcap_dispatch(pcap, atoi(opts->config[CONF_PCAP_DISPATCH_COUNT]),
+            (pcap_handler)&process_packet, (unsigned char *)opts);
 
-        /* If there was a packet and it was processed without error, then
-         * keep going.
+        /* Count processed packets
         */
-        if(res > 0 && opts->spa_pkt.packet_data_len > 0)
+        if(res > 0)
         {
-            res = incoming_spa(opts);
-
-            if(res != 0 && opts->verbose > 1)
-                log_msg(LOG_INFO, "incoming_spa returned error %i: '%s' for incoming packet.",
-                    res, get_errstr(res));
-
-            /* Count this packet since it has at least one byte of payload
-             * data - we use this as a comparison for --packet-limit regardless
+            /* Count the set of processed packets (pcap_dispatch() return
+             * value) - we use this as a comparison for --packet-limit regardless
              * of SPA packet validity at this point.
             */
-            opts->packet_ctr++;
+            opts->packet_ctr += res;
             if (opts->packet_ctr_limit && opts->packet_ctr >= opts->packet_ctr_limit)
             {
                 log_msg(LOG_WARNING,
@@ -281,7 +274,7 @@ pcap_capture(fko_srv_options_t *opts)
         check_firewall_rules(opts);
 
 #if FIREWALL_IPFW
-        /* Purge expired rules that no longer have any corresponding 
+        /* Purge expired rules that no longer have any corresponding
          * dynamic rules.
         */
         if(opts->fw_config->total_rules > 0)
@@ -295,7 +288,7 @@ pcap_capture(fko_srv_options_t *opts)
         }
 #endif
 
-        usleep(10000);
+        usleep(atoi(opts->config[CONF_PCAP_LOOP_SLEEP]));
     }
 
     pcap_close(pcap);
index 59b1ad6..1ef96d5 100644 (file)
@@ -36,6 +36,7 @@
 #include "fwknopd_common.h"
 #include "netinet_common.h"
 #include "process_packet.h"
+#include "incoming_spa.h"
 #include "utils.h"
 
 void
@@ -174,13 +175,19 @@ process_packet(unsigned char *args, const struct pcap_pkthdr *packet_header,
      * data.
     */
 
-    /* Truncate the data if it is too long.  This most likely means it is not
-     * a valid SPA packet anyway.
+    /* Expect the data to be at least the minimum required size.  This check
+     * will weed out a lot of things like small TCP ACK's if the user has a
+     * permissive pcap filter
+    */
+    if(pkt_data_len < MIN_SPA_DATA_SIZE)
+        return;
+
+    /* Expect the data to not be too large
     */
     if(pkt_data_len > MAX_SPA_PACKET_LEN)
-        pkt_data_len = MAX_SPA_PACKET_LEN;
+        return;
 
-    /* Put the data in our 1-entry queue.
+    /* Copy the packet for SPA processing
     */
     strlcpy((char *)opts->spa_pkt.packet_data, (char *)pkt_data, pkt_data_len+1);
     opts->spa_pkt.packet_data_len = pkt_data_len;
@@ -190,6 +197,8 @@ process_packet(unsigned char *args, const struct pcap_pkthdr *packet_header,
     opts->spa_pkt.packet_src_port = src_port;
     opts->spa_pkt.packet_dst_port = dst_port;
 
+    incoming_spa(opts);
+
     return;
 }
 
index 49a070c..e145ba8 100644 (file)
@@ -160,4 +160,25 @@ is_valid_dir(const char *path)
     return(1);
 }
 
+/* Determine if a buffer contains only characters from the base64
+ * encoding set
+*/
+int
+is_base64(const unsigned char *buf, unsigned short int len)
+{
+    unsigned short int  i;
+    int                 rv = 1;
+
+    for(i=0; i<len; i++)
+    {
+        if(!(isalnum(buf[i]) || buf[i] == '/' || buf[i] == '+' || buf[i] == '='))
+        {
+            rv = 0;
+            break;
+        }
+    }
+
+    return rv;
+}
+
 /***EOF***/
index 4ddf26b..09584e1 100644 (file)
@@ -60,6 +60,7 @@
 */
 void hex_dump(const unsigned char *data, const int size);
 char* dump_ctx(fko_ctx_t ctx);
+int is_base64(const unsigned char *buf, unsigned short int len);
 int is_valid_dir(const char *path);
 
 size_t strlcat(char *dst, const char *src, size_t siz);
index c95464f..1682237 100755 (executable)
@@ -1536,11 +1536,6 @@ sub server_ignore_small_packets() {
         $rv = 0;
     }
 
-    unless (&file_find_regex([qr/Not\senough\sdata/],
-            $server_test_file)) {
-        $rv = 0;
-    }
-
     return $rv;
 }