[iptables PATCH 0/6] A series of covscan-indicated fixes

[iptables PATCH 0/6] A series of covscan-indicated fixes

[Phil Sutter]

I had to dig through a huge coverity tool report, these are the things I
found worth fixing.

Phil Sutter (6):
  xtables-restore: Avoid access of uninitialized data
  extensions: time: Avoid undefined shift
  extensions: cluster: Avoid undefined shift
  libxtables: Avoid buffer overrun in xtables_compatible_revision()
  xtables-translate: Guard strcpy() call in xlate_ifname()
  extensions: among: Check call to fstat()

 extensions/libebt_among.c    | 6 +++++-
 extensions/libxt_cluster.c   | 2 +-
 extensions/libxt_time.c      | 2 +-
 iptables/xtables-restore.c   | 2 +-
 iptables/xtables-translate.c | 5 ++---
 libxtables/xtables.c         | 3 ++-
 6 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.24.0

[iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data

[Phil Sutter]

When flushing, 'buffer' is not written to prior to checking its first
byte's value. Therefore it needs to be initialized upon declaration.

Fixes: 09cb517949e69 ("xtables-restore: Improve performance of --noflush operation")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-restore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index dd907e0b8ddd5..63cc15cee9621 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -281,7 +281,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p)
 {
 	struct nft_xt_restore_state state = {};
-	char preload_buffer[PREBUFSIZ] = {}, buffer[10240], *ptr;
+	char preload_buffer[PREBUFSIZ] = {}, buffer[10240] = {}, *ptr;
 
 	if (!h->noflush) {
 		nft_fake_cache(h);
-- 
2.24.0

[iptables PATCH 2/6] extensions: time: Avoid undefined shift

[Phil Sutter]

Value 1 is signed 32-bit by default and left-shifting that by 31 is
undefined. Fix this by marking the value as unsigned.

Fixes: ad326ef9f734a ("Add the libxt_time iptables match")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/libxt_time.c b/extensions/libxt_time.c
index 5a8cc5de13031..d001f5b7f448f 100644
--- a/extensions/libxt_time.c
+++ b/extensions/libxt_time.c
@@ -330,7 +330,7 @@ static void time_print_monthdays(uint32_t mask, bool human_readable)
 
 	printf(" ");
 	for (i = 1; i <= 31; ++i)
-		if (mask & (1 << i)) {
+		if (mask & (1u << i)) {
 			if (nbdays++ > 0)
 				printf(",");
 			printf("%u", i);
-- 
2.24.0

[iptables PATCH 3/6] extensions: cluster: Avoid undefined shift

[Phil Sutter]

Value 1 is signed 32-bit by default and left-shifting that by 31 is
undefined. Fix this by marking the value as unsigned.

Fixes: 64a0e09894e52 ("extensions: libxt_cluster: Add translation to nft")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_cluster.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/libxt_cluster.c b/extensions/libxt_cluster.c
index c9c35ee22e3df..d164bf6960166 100644
--- a/extensions/libxt_cluster.c
+++ b/extensions/libxt_cluster.c
@@ -156,7 +156,7 @@ static int cluster_xlate(struct xt_xlate *xl,
 		xt_xlate_add(xl, "%s %u seed 0x%08x ", jhash_st,
 				info->total_nodes, info->hash_seed);
 		for (node = 0; node < 32; node++) {
-			if (info->node_mask & (1 << node)) {
+			if (info->node_mask & (1u << node)) {
 				if (needs_set == 0) {
 					xt_xlate_add(xl, "{ ");
 					needs_set = 1;
-- 
2.24.0

[iptables PATCH 4/6] libxtables: Avoid buffer overrun in xtables_compatible_revision()

[Phil Sutter]

The function is exported and accepts arbitrary strings as input. Calling
strcpy() without length checks is not OK.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 libxtables/xtables.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 895f6988eaf57..777c2b08e9896 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -856,7 +856,8 @@ int xtables_compatible_revision(const char *name, uint8_t revision, int opt)
 
 	xtables_load_ko(xtables_modprobe_program, true);
 
-	strcpy(rev.name, name);
+	strncpy(rev.name, name, XT_EXTENSION_MAXNAMELEN - 1);
+	rev.name[XT_EXTENSION_MAXNAMELEN - 1] = '\0';
 	rev.revision = revision;
 
 	max_rev = getsockopt(sockfd, afinfo->ipproto, opt, &rev, &s);
-- 
2.24.0

[iptables PATCH 5/6] xtables-translate: Guard strcpy() call in xlate_ifname()

[Phil Sutter]

The function potentially fed overlong strings to strcpy(). Given that
everything needed to avoid this is there, reorder code a bit to prevent
those inputs, too.

Fixes: 0ddd663e9c167 ("iptables-translate: add in/out ifname wildcard match translation to nft")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-translate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index a42c60a3b64c6..77a186b905d73 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -32,14 +32,13 @@
 void xlate_ifname(struct xt_xlate *xl, const char *nftmeta, const char *ifname,
 		  bool invert)
 {
+	int ifaclen = strlen(ifname);
 	char iface[IFNAMSIZ];
-	int ifaclen;
 
-	if (ifname[0] == '\0')
+	if (ifaclen < 1 || ifaclen >= IFNAMSIZ)
 		return;
 
 	strcpy(iface, ifname);
-	ifaclen = strlen(iface);
 	if (iface[ifaclen - 1] == '+')
 		iface[ifaclen - 1] = '*';
 
-- 
2.24.0

[iptables PATCH 6/6] extensions: among: Check call to fstat()

[Phil Sutter]

If this fails, a bogus length value may be passed to mmap().

Fixes: 26753888720d8 ("nft: bridge: Rudimental among extension support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libebt_among.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/extensions/libebt_among.c b/extensions/libebt_among.c
index 2e87db3bc06fa..715d559f432c2 100644
--- a/extensions/libebt_among.c
+++ b/extensions/libebt_among.c
@@ -6,6 +6,7 @@
  * August, 2003
  */
 
+#include <errno.h>
 #include <ctype.h>
 #include <fcntl.h>
 #include <getopt.h>
@@ -137,7 +138,10 @@ static int bramong_parse(int c, char **argv, int invert,
 		if ((fd = open(optarg, O_RDONLY)) == -1)
 			xtables_error(PARAMETER_PROBLEM,
 				      "Couldn't open file '%s'", optarg);
-		fstat(fd, &stats);
+		if (fstat(fd, &stats) < 0)
+			xtables_error(PARAMETER_PROBLEM,
+				      "fstat(%s) failed: '%s'",
+				      optarg, strerror(errno));
 		flen = stats.st_size;
 		/* use mmap because the file will probably be big */
 		optarg = mmap(0, flen, PROT_READ | PROT_WRITE,
-- 
2.24.0

Re: [iptables PATCH 0/6] A series of covscan-indicated fixes

[Pablo Neira Ayuso]

On Fri, Dec 06, 2019 at 12:47:05PM +0100, Phil Sutter wrote:
> I had to dig through a huge coverity tool report, these are the things I
> found worth fixing.

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks Phil.