[conntrack-tools PATCH 0/4] Fix some minor bugs

[conntrack-tools PATCH 0/4] Fix some minor bugs

[Phil Sutter]

All these were identified by Covscan.

Phil Sutter (4):
  conntrack: Fix potential array out of bounds access
  conntrack: Fix for unused assignment in do_command_ct()
  conntrack: Fix for unused assignment in ct_save_snprintf()
  conntrack: Sanitize free_tmpl_objects()

 src/conntrack.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.38.0

[conntrack-tools PATCH 1/4] conntrack: Fix potential array out of bounds access

[Phil Sutter]

If the link target length exceeds 'sizeof(tmp)' bytes, readlink() will
return 'sizeof(tmp)'. Using this value as index is illegal.

Fixes: b031cd2102d9b ("conntrack: pretty-print the portid")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/conntrack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index 859a4835580b0..aa6323dfbd1b1 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -1769,7 +1769,7 @@ static char *portid2name(pid_t pid, uint32_t portid, unsigned long inode)
 			continue;
 
 		rl = readlink(procname, tmp, sizeof(tmp));
-		if (rl <= 0 || rl > (ssize_t)sizeof(tmp))
+		if (rl <= 0 || rl >= (ssize_t)sizeof(tmp))
 			continue;
 
 		tmp[rl] = 0;
-- 
2.38.0

[conntrack-tools PATCH 2/4] conntrack: Fix for unused assignment in do_command_ct()

[Phil Sutter]

The variable is overwritten immediately in the next iteration and the
loop can't exit before doing that.

Instead of dropping the assignment, one could add a return code check -
but since event_cb() never fails, that check is pointless as well.

Fixes: e0dac21ed02e3 ("conntrack: use libmnl for conntrack events")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/conntrack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index aa6323dfbd1b1..07fae3dc2ff07 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -3479,7 +3479,7 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd,
 					   strerror(errno));
 				break;
 			}
-			res = mnl_cb_run(buf, res, 0, 0, event_cb, cmd);
+			mnl_cb_run(buf, res, 0, 0, event_cb, cmd);
 		}
 		mnl_socket_close(event_sock->mnl);
 		break;
-- 
2.38.0

[conntrack-tools PATCH 3/4] conntrack: Fix for unused assignment in ct_save_snprintf()

[Phil Sutter]

Setting 'ret' without calling BUFFER_SIZE() is pointless.

Fixes: 1c596b9ec8f26 ("conntrack: implement save output format")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/conntrack.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index 07fae3dc2ff07..2bd71e17e6be6 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -761,7 +761,6 @@ static int ct_save_snprintf(char *buf, size_t len,
 		BUFFER_SIZE(ret, size, len, offset);
 		break;
 	default:
-		ret = 0;
 		break;
 	}
 
-- 
2.38.0

[conntrack-tools PATCH 4/4] conntrack: Sanitize free_tmpl_objects()

[Phil Sutter]

The function unconditionally dereferenced its parameter, yet it is
possible for the passed 'cur_tmpl' pointer when called from exit_error()
to be still NULL: It is assigned to by alloc_tmpl_objects() at start of
do_parse(), though callers of that function might call exit_error() in
beforehand.

Fixes: 258b4540f4512 ("conntrack: add struct ct_tmpl")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/conntrack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/conntrack.c b/src/conntrack.c
index 2bd71e17e6be6..23eaf274a78a1 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -139,6 +139,8 @@ static int alloc_tmpl_objects(struct ct_tmpl *tmpl)
 
 static void free_tmpl_objects(struct ct_tmpl *tmpl)
 {
+	if (!tmpl)
+		return;
 	if (tmpl->ct)
 		nfct_destroy(tmpl->ct);
 	if (tmpl->exptuple)
-- 
2.38.0

Re: [conntrack-tools PATCH 0/4] Fix some minor bugs

[Pablo Neira Ayuso]

On Tue, Dec 20, 2022 at 04:38:43PM +0100, Phil Sutter wrote:
> All these were identified by Covscan.

Series LGTM.

Re: [conntrack-tools PATCH 0/4] Fix some minor bugs

[Phil Sutter]

On Wed, Dec 21, 2022 at 05:48:01PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 20, 2022 at 04:38:43PM +0100, Phil Sutter wrote:
> > All these were identified by Covscan.
> 
> Series LGTM.

Series applied, thanks for the review!