[PATCH 0/2] [arptables] Fixes for problems found by static analysis.

[PATCH 0/2] [arptables] Fixes for problems found by static analysis.

[Jiri Popelka]

We analyzed the arptables-0.4.4 code with Coverity - tool for static analysis.
Each patch contains relevant part of the Coverity scan output.

Jaromír Končický (2):
  Error: STRING_NULL
  Error: STRING_OVERFLOW

 userspace/arptables/arptables.c              | 13 ++++++++-----
 userspace/arptables/libarptc/libarptc_incl.c | 16 ++++++++++------
 2 files changed, 18 insertions(+), 11 deletions(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] Error: STRING_NULL

[Jiri Popelka]

From: Jaromír Končický <jkoncick@redhat.com>

arptables-v0.0.4/arptables.c:1671: string_null_argument: Function "read(int, void *, size_t)" does not terminate string "*ret".
arptables-v0.0.4/arptables.c:1675: string_null: Passing unterminated string "ret" to "strlen(char const *)", which expects a null-terminated string.
---
 userspace/arptables/arptables.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/userspace/arptables/arptables.c b/userspace/arptables/arptables.c
index 5535ab2..8ef445a 100644
--- a/userspace/arptables/arptables.c
+++ b/userspace/arptables/arptables.c
@@ -1668,10 +1668,12 @@ static char *get_modprobe(void)
 
 	ret = malloc(1024);
 	if (ret) {
-		switch (read(procfile, ret, 1024)) {
+		int read_bytes = read(procfile, ret, 1024);
+		switch (read_bytes) {
 		case -1: goto fail;
 		case 1024: goto fail; /* Partial read.  Wierd */
 		}
+		ret[read_bytes] = '\0';
 		if (ret[strlen(ret)-1]=='\n')
 			ret[strlen(ret)-1]=0;
 		close(procfile);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] Error: STRING_OVERFLOW

[Jiri Popelka]

From: Jaromír Končický <jkoncick@redhat.com>

Error: STRING_OVERFLOW
arptables-v0.0.4/arptables.c:1273: fixed_size_dest: You might overrun the 8192 byte fixed-size string "buf" by copying the return value of "mask_to_dotted(struct in_addr const *)" without checking the length.

Error: STRING_OVERFLOW
arptables-v0.0.4/arptables.c:1297: fixed_size_dest: You might overrun the 8192 byte fixed-size string "buf" by copying the return value of "mask_to_dotted(struct in_addr const *)" without checking the length.

Error: STRING_OVERFLOW
arptables-v0.0.4/libarptc/libarptc_incl.c:360: fixed_size_dest: You might overrun the 32 byte fixed-size string "(h->cache_chain_heads + h->cache_num_chains).name" by copying "arpt_get_target(e)->data" without checking the length.

Error: STRING_OVERFLOW
arptables-v0.0.4/libarptc/libarptc_incl.c:371: fixed_size_dest: You might overrun the 32 byte fixed-size string "(h->cache_chain_heads + h->cache_num_chains).name" by copying "h->hooknames[builtin - 1U]" without checking the length.

Error: STRING_OVERFLOW
arptables-v0.0.4/libarptc/libarptc_incl.c:213: fixed_size_dest: You might overrun the 32 byte fixed-size string "h->entries.name" by copying "tablename" without checking the length.
arptables-v0.0.4/libarptc/libarptc_incl.c:213: parameter_as_source: Note: This defect has an elevated risk because the source argument is a parameter of the current function.

Error: STRING_OVERFLOW
arptables-v0.0.4/libarptc/libarptc_incl.c:212: fixed_size_dest: You might overrun the 32 byte fixed-size string "h->info.name" by copying "tablename" without checking the length.
arptables-v0.0.4/libarptc/libarptc_incl.c:212: parameter_as_source: Note: This defect has an elevated risk because the source argument is a parameter of the current function.

Error: STRING_OVERFLOW
arptables-v0.0.4/arptables.c:2065: fixed_size_dest: You might overrun the 30 byte fixed-size string "target->t->u.user.name" by copying "jumpto" without checking the length.
---
 userspace/arptables/arptables.c              |  9 +++++----
 userspace/arptables/libarptc/libarptc_incl.c | 16 ++++++++++------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/userspace/arptables/arptables.c b/userspace/arptables/arptables.c
index 8ef445a..4da6fea 100644
--- a/userspace/arptables/arptables.c
+++ b/userspace/arptables/arptables.c
@@ -1270,7 +1270,7 @@ print_firewall(const struct arpt_entry *fw,
 			sprintf(buf, "%s", addr_to_dotted(&(fw->arp.src)));
 		else
 			sprintf(buf, "%s", addr_to_anyname(&(fw->arp.src)));
-		strcat(buf, mask_to_dotted(&(fw->arp.smsk)));
+		strncat(buf, mask_to_dotted(&(fw->arp.smsk)), sizeof(buf) - strlen(buf) -1);
 		printf("-s %s ", buf);
 	}
 
@@ -1294,7 +1294,7 @@ after_devsrc:
 			sprintf(buf, "%s", addr_to_dotted(&(fw->arp.tgt)));
 		else
 			sprintf(buf, "%s", addr_to_anyname(&(fw->arp.tgt)));
-		strcat(buf, mask_to_dotted(&(fw->arp.tmsk)));
+		strncat(buf, mask_to_dotted(&(fw->arp.tmsk)),  sizeof(buf) - strlen(buf) -1);
 		printf("-d %s ", buf);
 	}
 
@@ -1796,7 +1796,7 @@ int do_command(int argc, char *argv[], char **table, arptc_handle_t *handle)
 				*table, arptc_strerror(errno));
 			}
 		}
-        }
+	}
 
 	memset(&fw, 0, sizeof(fw));
 	opts = original_opts;
@@ -2064,7 +2064,8 @@ int do_command(int argc, char *argv[], char **table, arptc_handle_t *handle)
 
 				target->t = fw_calloc(1, size);
 				target->t->u.target_size = size;
-				strcpy(target->t->u.user.name, jumpto);
+				strncpy(target->t->u.user.name, jumpto, sizeof(target->t->u.user.name));
+				target->t->u.user.name[sizeof(target->t->u.user.name)-1] = '\0';
 /*
 				target->init(target->t, &fw.nfcache);
 */
diff --git a/userspace/arptables/libarptc/libarptc_incl.c b/userspace/arptables/libarptc/libarptc_incl.c
index 2fa3d43..9c1aeac 100644
--- a/userspace/arptables/libarptc/libarptc_incl.c
+++ b/userspace/arptables/libarptc/libarptc_incl.c
@@ -209,8 +209,10 @@ alloc_handle(const char *tablename, unsigned int size, unsigned int num_rules)
 	h->counter_map = (void *)h
 		+ sizeof(STRUCT_TC_HANDLE)
 		+ size;
-	strcpy(h->info.name, tablename);
-	strcpy(h->entries.name, tablename);
+	strncpy(h->info.name, tablename, sizeof(h->info.name));
+	h->info.name[sizeof(h->info.name)-1] = '\0';
+	strncpy(h->entries.name, tablename, sizeof(h->entries.name));
+	h->entries.name[sizeof(h->entries.name)-1] = '\0';
 
 	return h;
 }
@@ -357,8 +359,9 @@ add_chain(STRUCT_ENTRY *e, TC_HANDLE_T h, STRUCT_ENTRY **prev)
 		h->cache_chain_heads[h->cache_num_chains-1].end
 			= *prev;
 
-		strcpy(h->cache_chain_heads[h->cache_num_chains].name,
-		       (const char *)GET_TARGET(e)->data);
+		strncpy(h->cache_chain_heads[h->cache_num_chains].name,
+		       (const char *)GET_TARGET(e)->data, TABLE_MAXNAMELEN-1);
+		h->cache_chain_heads[h->cache_num_chains].name[TABLE_MAXNAMELEN-1] = '\0';
 		h->cache_chain_heads[h->cache_num_chains].start
 			= (void *)e + e->next_offset;
 		h->cache_num_chains++;
@@ -368,8 +371,9 @@ add_chain(STRUCT_ENTRY *e, TC_HANDLE_T h, STRUCT_ENTRY **prev)
 			h->cache_chain_heads[h->cache_num_chains-1].end
 				= *prev;
 
-		strcpy(h->cache_chain_heads[h->cache_num_chains].name,
-		       h->hooknames[builtin-1]);
+		strncpy(h->cache_chain_heads[h->cache_num_chains].name,
+		       h->hooknames[builtin-1], TABLE_MAXNAMELEN-1);
+		h->cache_chain_heads[h->cache_num_chains].name[TABLE_MAXNAMELEN-1] = '\0';
 		h->cache_chain_heads[h->cache_num_chains].start
 			= (void *)e;
 		h->cache_num_chains++;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html