ebtables: Possible problems found by static analysis of code

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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)

Op 14/06/2011 12:32, Jiri Popelka schreef:

Thanks,

I'll have a closer look at them later this week.

cheers,
Bart

Bart De Schuymer
www.artinalgorithms.be

I've taken all reported issues into account. The changes are in the latest CVS. See below for my comments on your submitted patches:

1. forward_null:

diff 1: no issue, next can only be NULL in the first iteration (unless there is a bug). Added an assertion just in case. diff 2: possible issue. If repl->counters is NULL, then repl->nentries is 0 too, though. Depends
on the implementation of fread. Committed an altered patch (note that the submitted patch wrongly uses repr instead of repl)

2. no_effect:
removed the negative value checks instead of checking on ULONG_MAX as the submitted patch did.

3. overrun_static:
first error: non-issue: part of the API between ebtables userspace and kernel
second error: non-issue: part of the API between ebtables userspace and kernel: only the initialized elements are needed by the kernel

4. resource_leak:
applied

5. uninit:
applied

6. use_after_free:
added a different fix (the submitted patch made things worse): cc->type should be set to CNT_DEL when the original cc->type != CNT_ADD

Thanks again for your time and interest,
Bart

Bart De Schuymer
www.artinalgorithms.be