[PATCH] ebtables: Possible problems found by static analysis of code

[PATCH] ebtables: Possible problems found by static analysis of code

[Jiri Popelka]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

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


[-- Attachment #2: ebtables-v2.0.9-2-Coverity-forward_null.patch --]
[-- Type: text/plain, Size: 1598 bytes --]

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;

[-- Attachment #3: ebtables-v2.0.9-2-Coverity-no_effect.patch --]
[-- Type: text/plain, Size: 1614 bytes --]

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;

[-- Attachment #4: ebtables-v2.0.9-2-Coverity-overrun_static.patch --]
[-- Type: text/plain, Size: 783 bytes --]

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

[-- Attachment #5: ebtables-v2.0.9-2-Coverity-resource_leak.patch --]
[-- Type: text/plain, Size: 1625 bytes --]

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++;

[-- Attachment #6: ebtables-v2.0.9-2-Coverity-uninit.patch --]
[-- Type: text/plain, Size: 1157 bytes --]

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

[-- Attachment #7: ebtables-v2.0.9-2-Coverity-use_after_free.patch --]
[-- Type: text/plain, Size: 590 bytes --]

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)

Re: [PATCH] ebtables: Possible problems found by static analysis of code

[Bart De Schuymer]

Op 14/06/2011 12:32, Jiri Popelka schreef:
> 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.
>

Thanks,

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

cheers,
Bart

-- 
Bart De Schuymer
www.artinalgorithms.be

Re: [PATCH] ebtables: Possible problems found by static analysis of code

[Bart De Schuymer]

On 15-06-11 22:37, Bart De Schuymer wrote:
> Op 14/06/2011 12:32, Jiri Popelka schreef:
>> 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.
>>
>
> Thanks,
>
> I'll have a closer look at them later this week.
>
> cheers,
> Bart
>

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