From 020a4d08937a68d00d8a17717783bcbb6eb489f7 Mon Sep 17 00:00:00 2001 From: Peter Haag Date: Fri, 22 Dec 2017 12:09:56 +0100 Subject: [PATCH] Fix potential memory leaks in nfpcapd --- ChangeLog | 3 +++ bin/flowtree.c | 8 -------- bin/flowtree.h | 5 ----- bin/ipfix.c | 2 +- bin/ipfrag.c | 15 +++++++++++++-- bin/ipfrag.h | 9 +++------ bin/netflow_v9.c | 2 +- bin/nfpcapd.c | 4 ++-- bin/pcaproc.c | 49 ++++++++++++++++++++---------------------------- 9 files changed, 43 insertions(+), 54 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4f0eff6..2e338ad 100755 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,6 @@ +2017-12-22 +- Fix potential memory leaks in nfpcapd + 2017-12-21 - Fix wrong offset calculation if unknown options are found - Add x-late src/dst ip aggregation, if compiled with NSEL support diff --git a/bin/flowtree.c b/bin/flowtree.c index 97afd00..1c5bc8f 100644 --- a/bin/flowtree.c +++ b/bin/flowtree.c @@ -110,7 +110,6 @@ struct FlowNode *node; memset((void *)node, 'A', sizeof(struct FlowNode)); node->left = NULL; node->right = NULL; - node->data = NULL; node->memflag = NODE_IN_USE; dbg_printf("New node: %llx\n", (unsigned long long)node); return node; @@ -162,11 +161,6 @@ void Free_Node(struct FlowNode *node) { abort(); } - if ( node->data ) { - free(node->data); - node->data = NULL; - } - dbg_assert(node->left == NULL); dbg_assert(node->right == NULL); @@ -248,8 +242,6 @@ struct FlowNode *node, *nxt; for (node = RB_MIN(FlowTree, FlowTree); node != NULL; node = nxt) { nxt = RB_NEXT(FlowTree, FlowTree, node); RB_REMOVE(FlowTree, FlowTree, node); - if ( node->data ) - free(node->data); } free(FlowElementCache); FlowElementCache = NULL; diff --git a/bin/flowtree.h b/bin/flowtree.h index 7010b95..a140e5a 100644 --- a/bin/flowtree.h +++ b/bin/flowtree.h @@ -92,11 +92,6 @@ struct FlowNode { uint32_t packets; // summed up number of packets uint32_t bytes; // summed up number of bytes - // flow payload -#define DATABLOCKSIZE 256 - uint32_t DataSize; // max size of data buffer - void *data; // start of data buffer - struct FlowNode *rev_node; struct latency_s { uint64_t client; diff --git a/bin/ipfix.c b/bin/ipfix.c index 111acba..3e7b397 100644 --- a/bin/ipfix.c +++ b/bin/ipfix.c @@ -1220,7 +1220,7 @@ ipfix_template_record_t *ipfix_template_record; } // End of Process_ipfix_template_withdraw static inline void Process_ipfix_option_templates(exporter_ipfix_domain_t *exporter, void *option_template_flowset, FlowSource_t *fs) { -void *DataPtr; +uint8_t *DataPtr; uint32_t size_left, size_required, i; // uint32_t nr_scopes, nr_options; uint16_t id, field_count, scope_field_count, offset, sampler_id_length; diff --git a/bin/ipfrag.c b/bin/ipfrag.c index 7be4a15..abb0c84 100644 --- a/bin/ipfrag.c +++ b/bin/ipfrag.c @@ -1,4 +1,5 @@ /* + * Copyright (c) 2017, Peter Haag * Copyright (c) 2016, Peter Haag * Copyright (c) 2014, Peter Haag * All rights reserved. @@ -77,6 +78,7 @@ static void Remove_node(struct IPFragNode *node); RB_GENERATE(IPFragTree, IPFragNode, entry, IPFragNodeCMP); static IPFragTree_t *IPFragTree; +static uint32_t NumFragments; static int IPFragNodeCMP(struct IPFragNode *e1, struct IPFragNode *e2) { uint32_t *a = &e1->src_addr; @@ -119,6 +121,7 @@ struct IPFragNode *node; node->holes->next = NULL; node->holes->first = 0; node->holes->last = IP_MAXPACKET; + NumFragments++; return node; @@ -136,6 +139,7 @@ hole_t *hole, *h; if (free_data) free(node->data); free(node); + NumFragments--; } // End of Free_node @@ -156,6 +160,7 @@ int IPFragTree_init(void) { return 0; } RB_INIT(IPFragTree); + NumFragments = 0; dbg_printf("IPFrag key len: %lu\n", KEYLEN); return 1; } // End of IPFragTree_init @@ -172,6 +177,8 @@ struct IPFragNode *node, *nxt; free(IPFragTree); IPFragTree = NULL; + NumFragments = 0; + } // End of IPFragTree_free void *IPFrag_tree_Update(uint32_t src, uint32_t dst, uint32_t ident, uint32_t *length, uint32_t ip_off, void *data) { @@ -203,14 +210,14 @@ int found_hole; if ( last > IP_MAXPACKET ) { LogError("Fragment assembly error: last > IP_MAXPACKET"); - LogError("Fraget assembly: first: %u, last: %u, MF: %u\n", first, last, more_fragments); + LogError("Fragment assembly: first: %u, last: %u, MF: %u\n", first, last, more_fragments); return NULL; } // last fragment - sets max offset found_hole = 0; max = more_fragments == 0 ? last : 0; - dbg_printf("Fraget assembly: first: %u, last: %u, MF: %u\n", first, last, more_fragments); + dbg_printf("Fragment assembly: first: %u, last: %u, MF: %u\n", first, last, more_fragments); while (hole) { uint16_t hole_last; if ( max ) { @@ -307,3 +314,7 @@ int found_hole; } } // End of IPFrag_tree_Update + +uint32_t IPFragEntries() { + return NumFragments; +} // End of IPFragEntries diff --git a/bin/ipfrag.h b/bin/ipfrag.h index ca2dcb9..159f0fe 100644 --- a/bin/ipfrag.h +++ b/bin/ipfrag.h @@ -1,4 +1,5 @@ /* + * Copyright (c) 2017, Peter Haag * Copyright (c) 2014, Peter Haag * All rights reserved. * @@ -26,12 +27,6 @@ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE * POSSIBILITY OF SUCH DAMAGE. * - * $Author: phaag $ - * - * $Id: ipfrag.h 40691 2014-03-03 11:24:22Z phaag $ - * - * $LastChangedRevision: 40691 $ - * */ typedef struct hole_s { @@ -72,3 +67,5 @@ void IPFragTree_free(void); void *IPFrag_tree_Update(uint32_t src, uint32_t dst, uint32_t ident, uint32_t *length, uint32_t ip_off, void *data); +uint32_t IPFragEntries(void); + diff --git a/bin/netflow_v9.c b/bin/netflow_v9.c index 7c1e9d9..be96590 100644 --- a/bin/netflow_v9.c +++ b/bin/netflow_v9.c @@ -1335,7 +1335,7 @@ int i; } // End of Process_v9_templates static inline void Process_v9_option_templates(exporter_v9_domain_t *exporter, void *option_template_flowset, FlowSource_t *fs) { -void *option_template, *p; +uint8_t *option_template, *p; uint32_t size_left, nr_scopes, nr_options, i; uint16_t id, scope_length, option_length, offset, sampler_id_length; uint16_t offset_sampler_id, offset_sampler_mode, offset_sampler_interval, found_sampler; diff --git a/bin/nfpcapd.c b/bin/nfpcapd.c index b01ffb3..9c97258 100644 --- a/bin/nfpcapd.c +++ b/bin/nfpcapd.c @@ -762,9 +762,9 @@ int err, done; UpdateBooks(fs->bookkeeper, t_start, 512*fstat.st_blocks); } - LogInfo("Ident: '%s' Flows: %llu, Packets: %llu, Bytes: %llu, Max Flows: %u", + LogInfo("Ident: '%s' Flows: %llu, Packets: %llu, Bytes: %llu, Max Flows: %u, Fragments: %u", fs->Ident, (unsigned long long)nffile->stat_record->numflows, (unsigned long long)nffile->stat_record->numpackets, - (unsigned long long)nffile->stat_record->numbytes, NumFlows); + (unsigned long long)nffile->stat_record->numbytes, NumFlows, IPFragEntries()); // reset stats fs->bad_packets = 0; diff --git a/bin/pcaproc.c b/bin/pcaproc.c index 2bc846d..6a291b6 100644 --- a/bin/pcaproc.c +++ b/bin/pcaproc.c @@ -494,6 +494,7 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; // data is sitting on a defragmented IPv4 packet memory region // REDO loop could result in a memory leak, if again IP is fragmented // XXX memory leak to be fixed + LogError("Fragmentation memory leak triggered!"); } ip = (struct ip *)(data + offset); // offset points to end of link layer @@ -570,13 +571,24 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; inet_ntop(AF_INET, &ip->ip_dst, s2, sizeof(s2))); // IPv4 defragmentation - if ( (ip_off & IP_MF) || frag_offset ) { + if ( (ip_off & IP_MF) || frag_offset ) { uint16_t ip_id = ntohs(ip->ip_id); +#ifdef DEVEL + if ( frag_offset == 0 ) + printf("Fragmented packet: first segement: ip_off: %u, frag_offset: %u\n", ip_off, frag_offset); + if (( ip_off & IP_MF ) && frag_offset ) + printf("Fragmented packet: middle segement: ip_off: %u, frag_offset: %u\n", ip_off, frag_offset); + if (( ip_off & IP_MF ) == 0 ) + printf("Fragmented packet: last segement: ip_off: %u, frag_offset: %u\n", ip_off, frag_offset); +#endif // fragmented packet defragmented = IPFrag_tree_Update(ip->ip_src.s_addr, ip->ip_dst.s_addr, ip_id, &payload_len, ip_off, payload); - if ( defragmented == NULL ) + if ( defragmented == NULL ) { // not yet complete + dbg_printf("Fragmentation not yet completed\n"); return; + } + dbg_printf("Fragmentation assembled\n"); // packet defragmented - set payload to defragmented data payload = defragmented; } @@ -609,25 +621,16 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; if ( UDPlen < 8 ) { LogError("UDP payload legth error: %u bytes < 8\n", UDPlen); - if ( defragmented ) { - free(defragmented); - defragmented = NULL; - } Free_Node(Node); - return; + break; } uint32_t size_udp_payload = ntohs(udp->uh_ulen) - 8; if ( (bytes == payload_len ) && (payload_len - sizeof(struct udphdr)) != size_udp_payload ) { LogError("UDP payload legth error: Expected %u, have %u bytes\n", size_udp_payload, (payload_len - (unsigned)sizeof(struct udphdr))); - - if ( defragmented ) { - free(defragmented); - defragmented = NULL; - } Free_Node(Node); - return; + break; } payload = payload + sizeof(struct udphdr); payload_len -= sizeof(struct udphdr); @@ -653,12 +656,8 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; if ( payload_len < size_tcp ) { LogError("TCP header length error: len: %u < size TCP header: %u", payload_len, size_tcp); pcap_dev->proc_stat.short_snap++; - if ( defragmented ) { - free(defragmented); - defragmented = NULL; - } Free_Node(Node); - return; + break; } payload = payload + size_tcp; @@ -731,12 +730,8 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; if ( payload_len < size_inner_ip ) { LogError("IPIP tunnel header length error: len: %u < size inner IP: %u", payload_len, size_inner_ip); pcap_dev->proc_stat.short_snap++; - if ( defragmented ) { - free(defragmented); - defragmented = NULL; - } Free_Node(Node); - return; + break; } offset = 0; data = payload; @@ -760,12 +755,8 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; if ( payload_len < gre_hdr_size ) { LogError("GRE tunnel header length error: len: %u < size GRE hdr: %u", payload_len, gre_hdr_size); pcap_dev->proc_stat.short_snap++; - if ( defragmented ) { - free(defragmented); - defragmented = NULL; - } Free_Node(Node); - return; + break; } dbg_printf("GRE proto encapsulation: type: 0x%x\n", ethertype); @@ -791,9 +782,9 @@ pkt->vlans[pkt->vlan_count].pcp = (p[0] >> 5) & 7; } if ( defragmented ) { - LogError("Defragmented buffer not freed for proto %u", proto); free(defragmented); defragmented = NULL; + dbg_printf("Defragmented buffer freed for proto %u", proto); }