* [iptables PATCH 0/6] A series of covscan-indicated fixes
@ 2019-12-06 11:47 Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data Phil Sutter
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 2/6] extensions: time: Avoid undefined shift Phil Sutter
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [iptables PATCH 2/6] extensions: time: Avoid undefined shift
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 3/6] extensions: cluster: " Phil Sutter
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [iptables PATCH 3/6] extensions: cluster: Avoid undefined shift
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 2/6] extensions: time: Avoid undefined shift Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 4/6] libxtables: Avoid buffer overrun in xtables_compatible_revision() Phil Sutter
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [iptables PATCH 4/6] libxtables: Avoid buffer overrun in xtables_compatible_revision()
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
` (2 preceding siblings ...)
2019-12-06 11:47 ` [iptables PATCH 3/6] extensions: cluster: " Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 5/6] xtables-translate: Guard strcpy() call in xlate_ifname() Phil Sutter
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [iptables PATCH 5/6] xtables-translate: Guard strcpy() call in xlate_ifname()
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
` (3 preceding siblings ...)
2019-12-06 11:47 ` [iptables PATCH 4/6] libxtables: Avoid buffer overrun in xtables_compatible_revision() Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 6/6] extensions: among: Check call to fstat() Phil Sutter
2019-12-07 18:25 ` [iptables PATCH 0/6] A series of covscan-indicated fixes Pablo Neira Ayuso
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [iptables PATCH 6/6] extensions: among: Check call to fstat()
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
` (4 preceding siblings ...)
2019-12-06 11:47 ` [iptables PATCH 5/6] xtables-translate: Guard strcpy() call in xlate_ifname() Phil Sutter
@ 2019-12-06 11:47 ` Phil Sutter
2019-12-07 18:25 ` [iptables PATCH 0/6] A series of covscan-indicated fixes Pablo Neira Ayuso
6 siblings, 0 replies; 8+ messages in thread
From: Phil Sutter @ 2019-12-06 11:47 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [iptables PATCH 0/6] A series of covscan-indicated fixes
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
` (5 preceding siblings ...)
2019-12-06 11:47 ` [iptables PATCH 6/6] extensions: among: Check call to fstat() Phil Sutter
@ 2019-12-07 18:25 ` Pablo Neira Ayuso
6 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2019-12-07 18:25 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-12-07 18:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 11:47 [iptables PATCH 0/6] A series of covscan-indicated fixes Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 1/6] xtables-restore: Avoid access of uninitialized data Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 2/6] extensions: time: Avoid undefined shift Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 3/6] extensions: cluster: " Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 4/6] libxtables: Avoid buffer overrun in xtables_compatible_revision() Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 5/6] xtables-translate: Guard strcpy() call in xlate_ifname() Phil Sutter
2019-12-06 11:47 ` [iptables PATCH 6/6] extensions: among: Check call to fstat() Phil Sutter
2019-12-07 18:25 ` [iptables PATCH 0/6] A series of covscan-indicated fixes Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).