Hello,
we had analyzed the ebtables-v2.0.9-2 code with Coverity.
Coverity is commercial enterprise level tool for
static analysis (analysis based only on compiling of sources,
not based on running of binary) of the code.
As a result I have the following patches that should fix some possible problems.
There's a respective part(s) of the Coverity error log in each patch.
You could also find this link useful:
https://www.securecoding.cert.org/confluence/display/seccode/Coverity+Prevent
With regards,
Jiri Popelka
Error: FORWARD_NULL
ebtables-v2.0.9-2/communication.c:305: var_compare_op: Comparing "next" to null implies that "next" might be null.
ebtables-v2.0.9-2/communication.c:316: var_deref_op: Dereferencing null variable "next".
Error: FORWARD_NULL
ebtables-v2.0.9-2/communication.c:632: assign_zero: Assigning: "repl->counters" = 0.
ebtables-v2.0.9-2/communication.c:634: var_deref_model: Passing null variable "(char *)repl->counters" to function "fread", which dereferences it.
diff -up ebtables-v2.0.9-2/communication.c.forward_null ebtables-v2.0.9-2/communication.c
--- ebtables-v2.0.9-2/communication.c.forward_null 2010-02-03 22:17:45.000000000 +0100
+++ ebtables-v2.0.9-2/communication.c 2011-06-01 18:20:52.844295103 +0200
@@ -309,6 +309,8 @@ void ebt_deliver_counters(struct ebt_u_r
if (chainnr == u_repl->num_chains)
break;
}
+ if (next == NULL)
+ ebt_print_error("ebt_deliver_counters: next == NULL");
if (cc->type == CNT_NORM) {
/* 'Normal' rule, meaning we didn't do anything to it
* So, we just copy */
@@ -636,9 +638,9 @@ static int retrieve_from_file(char *file
!= repl->entries_size ||
fseek(file, sizeof(struct ebt_replace) + repl->entries_size,
SEEK_SET)
- || fread((char *)repl->counters, sizeof(char),
+ || (repr->counters && fread((char *)repl->counters, sizeof(char),
repl->nentries * sizeof(struct ebt_counter), file)
- != repl->nentries * sizeof(struct ebt_counter)) {
+ != repl->nentries * sizeof(struct ebt_counter))) {
ebt_print_error("File %s is corrupt", filename);
free(entries);
repl->entries = NULL;
Error: NO_EFFECT
ebtables-v2.0.9-2/extensions/ebt_nflog.c:83: unsigned_compare: This less-than-zero comparison of an unsigned value is never true. "i < 0U".
ebtables-v2.0.9-2/extensions/ebt_nflog.c:95: unsigned_compare: This less-than-zero comparison of an unsigned value is never true. "i < 0U".
ebtables-v2.0.9-2/extensions/ebt_nflog.c:107: unsigned_compare: This less-than-zero comparison of an unsigned value is never true. "i < 0U".
diff -up ebtables-v2.0.9-2/extensions/ebt_nflog.c.no_effect ebtables-v2.0.9-2/extensions/ebt_nflog.c
--- ebtables-v2.0.9-2/extensions/ebt_nflog.c.no_effect 2010-02-03 22:17:45.000000000 +0100
+++ ebtables-v2.0.9-2/extensions/ebt_nflog.c 2011-06-01 18:29:41.058691515 +0200
@@ -80,7 +80,7 @@ static int nflog_parse(int c, char **arg
i = strtoul(optarg, &end, 10);
if (*end != '\0')
ebt_print_error2("--nflog-group must be a number!");
- if (i < 0)
+ if (i == ULONG_MAX)
ebt_print_error2("--nflog-group can not be negative");
info->group = i;
break;
@@ -92,7 +92,7 @@ static int nflog_parse(int c, char **arg
i = strtoul(optarg, &end, 10);
if (*end != '\0')
ebt_print_error2("--nflog-range must be a number!");
- if (i < 0)
+ if (i == ULONG_MAX)
ebt_print_error2("--nflog-range can not be negative");
info->len = i;
break;
@@ -104,7 +104,7 @@ static int nflog_parse(int c, char **arg
i = strtoul(optarg, &end, 10);
if (*end != '\0')
ebt_print_error2("--nflog-threshold must be a number!");
- if (i < 0)
+ if (i == ULONG_MAX)
ebt_print_error2
("--nflog-threshold can not be negative");
info->threshold = i;
Error: OVERRUN_STATIC
ebtables-v2.0.9-2/communication.c:378: assignment: Assigning: "optlen" = "u_repl->nentries * sizeof (struct ebt_counter) /*16*/ + sizeof (struct ebt_replace) /*120*/".
ebtables-v2.0.9-2/communication.c:387: overrun-buffer-arg: Overrunning struct type struct ebt_replace of size 120 bytes by passing it to a function which indexes it with argument "optlen" at byte position 135.
Error: UNINIT
ebtables-v2.0.9-2/communication.c:288: var_decl: Declaring variable "repl" without initializer.
ebtables-v2.0.9-2/communication.c:387: uninit_use_in_call: Using uninitialized value "repl": field "repl".entries is uninitialized when calling "setsockopt".
I'm not sure whether this is really a problem and how to properly treat it,
so there's actually no patch here :-)
Error: RESOURCE_LEAK
ebtables-v2.0.9-2/extensions/ebt_among.c:191: alloc_fn: Calling allocation function "new_wormhash".
ebtables-v2.0.9-2/extensions/ebt_among.c:87: alloc_fn: Storage is returned from allocation function "malloc".
ebtables-v2.0.9-2/extensions/ebt_among.c:87: var_assign: Assigning: "result" = "malloc(size)".
ebtables-v2.0.9-2/extensions/ebt_among.c:92: noescape: Variable "result" is not freed or pointed-to in function "memset".
ebtables-v2.0.9-2/extensions/ebt_among.c:94: return_alloc: Returning allocated memory "result".
ebtables-v2.0.9-2/extensions/ebt_among.c:191: var_assign: Assigning: "workcopy" = storage returned from "new_wormhash(1024)".
ebtables-v2.0.9-2/extensions/ebt_among.c:205: leaked_storage: Variable "workcopy" going out of scope leaks the storage it points to.
ebtables-v2.0.9-2/extensions/ebt_among.c:210: leaked_storage: Variable "workcopy" going out of scope leaks the storage it points to.
diff -up ebtables-v2.0.9-2/extensions/ebt_among.c.resource_leak ebtables-v2.0.9-2/extensions/ebt_among.c
--- ebtables-v2.0.9-2/extensions/ebt_among.c.resource_leak 2010-02-03 22:17:45.000000000 +0100
+++ ebtables-v2.0.9-2/extensions/ebt_among.c 2011-06-01 19:19:46.886113499 +0200
@@ -202,11 +202,13 @@ static struct ebt_mac_wormhash *create_w
if (read_until(&pc, ":", token, 2) < 0
|| token[0] == 0) {
ebt_print_error("MAC parse error: %.20s", anchor);
+ free(workcopy);
return NULL;
}
mac[i] = strtol(token, &endptr, 16);
if (*endptr) {
ebt_print_error("MAC parse error: %.20s", anchor);
+ free(workcopy);
return NULL;
}
pc++;
Error: UNINIT
ebtables-v2.0.9-2/communication.c:59: var_decl: Declaring variable "chain_offsets" without initializer.
ebtables-v2.0.9-2/communication.c:69: alloc_fn: Assigning: "chain_offsets" = "(unsigned int *)malloc(u_repl->num_chains * sizeof (unsigned int) /*4*/)", which is allocated but not initialized.
ebtables-v2.0.9-2/communication.c:169: uninit_use: Using uninitialized value "*chain_offsets".
diff -up ebtables-v2.0.9-2/communication.c.uninit ebtables-v2.0.9-2/communication.c
--- ebtables-v2.0.9-2/communication.c.uninit 2010-02-03 22:17:45.000000000 +0100
+++ ebtables-v2.0.9-2/communication.c 2011-06-01 19:36:52.192295410 +0200
@@ -66,7 +66,10 @@ static struct ebt_replace *translate_use
new->nentries = u_repl->nentries;
new->num_counters = u_repl->num_counters;
new->counters = sparc_cast u_repl->counters;
- chain_offsets = (unsigned int *)malloc(u_repl->num_chains * sizeof(unsigned int));
+ chain_offsets = (unsigned int *)calloc(u_repl->num_chains, sizeof(unsigned int));
+ if (!chain_offsets)
+ ebt_print_memory();
+
/* Determine size */
for (i = 0; i < u_repl->num_chains; i++) {
if (!(entries = u_repl->chains[i]))
Error: USE_AFTER_FREE
ebtables-v2.0.9-2/libebtc.c:408: freed_arg: "free" frees "cc".
ebtables-v2.0.9-2/libebtc.c:410: deref_after_free: Dereferencing freed pointer "cc".
diff -up ebtables-v2.0.9-2/libebtc.c.use_after_free ebtables-v2.0.9-2/libebtc.c
--- ebtables-v2.0.9-2/libebtc.c.use_after_free 2010-02-03 22:17:45.000000000 +0100
+++ ebtables-v2.0.9-2/libebtc.c 2011-06-01 19:53:36.764736526 +0200
@@ -407,7 +407,6 @@ void ebt_delete_cc(struct ebt_cntchanges
cc->next->prev = cc->prev;
free(cc);
}
- cc->type = CNT_DEL;
}
void ebt_empty_chain(struct ebt_u_entries *entries)
