[PATCH 0/2] Fix bugs in validation of interface names

[PATCH 0/2] Fix bugs in validation of interface names

[David Gibson]

We have a number of off by one bugs when checking the lengths of
networking interface names.

David Gibson (2):
  conf: Fix size checking of -I interface name
  conf: Correct length checking of interface names in conf_ports()

 conf.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH 1/2] conf: Fix size checking of -I interface name

[David Gibson]

Network interface names must fit in a buffer of IFNAMSIZ bytes, including
the terminating \0.  IFNAMSIZ is 16 on Linux, so interface names can be
up to (and including) 15 characters long.

We validate this for the -I option, but we have an off by one error.  We
pass (IFNAMSIZ - 1) as the buffer size to snprintf(), but that buffer size
already includes the terminating \0, so this actually truncates the value
to 14 characters.  The return value returned from snprintf() however, is
the number of characters that would have been printed *excluding* the
terminating \0, so by comparing it >= IFNAMSIZ - 1 we are giving an error
on names >= 15 characters rather than strictly > 15 characters.

Bugzila: https://bugs.passt.top/show_bug.cgi?id=61

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/conf.c b/conf.c
index 68487a41..19064368 100644
--- a/conf.c
+++ b/conf.c
@@ -1439,9 +1439,9 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (*c->pasta_ifn)
 				die("Multiple --ns-ifname options given");
 
-			ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
+			ret = snprintf(c->pasta_ifn, IFNAMSIZ, "%s",
 				       optarg);
-			if (ret <= 0 || ret >= IFNAMSIZ - 1)
+			if (ret <= 0 || ret >= IFNAMSIZ)
 				die("Invalid interface name: %s", optarg);
 
 			break;
-- 
@@ -1439,9 +1439,9 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (*c->pasta_ifn)
 				die("Multiple --ns-ifname options given");
 
-			ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
+			ret = snprintf(c->pasta_ifn, IFNAMSIZ, "%s",
 				       optarg);
-			if (ret <= 0 || ret >= IFNAMSIZ - 1)
+			if (ret <= 0 || ret >= IFNAMSIZ)
 				die("Invalid interface name: %s", optarg);
 
 			break;
-- 
2.41.0

[PATCH 2/2] conf: Correct length checking of interface names in conf_ports()

[David Gibson]

When interface names are specified in forwarding specs, we need to check
the length of the given interface name against the limit of IFNAMSIZ - 1
(15) characters.  However, we managed to have 3 separate off-by-one errors
here meaning we only accepted interface names up to 12 characters.

1. At the point of the check 'ifname' was still on the '%' character, not
   the first character of the name, meaning we overestimated the length by
   one
2. At the point of the check 'spec' had been advanced one character past
   the '/' which terminates the interface name, meaning we overestimated
   the length by another one
3. We checked if the (miscalculated) length was >= IFNAMSIZ - 1, that is
   >= 15, whereas lengths equal to 15 should be accepted.

Correct all 3 errors.

Bugzilla: https://bugs.passt.top/show_bug.cgi?id=61

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c
index 19064368..78eaf2d1 100644
--- a/conf.c
+++ b/conf.c
@@ -256,11 +256,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			goto bad;
 
 		if ((ifname = strchr(buf, '%'))) {
-			if (spec - ifname >= IFNAMSIZ - 1)
-				goto bad;
-
 			*ifname = 0;
 			ifname++;
+
+			/* spec is already advanced one past the '/',
+			 * so the length of the given ifname is:
+			 * (spec - ifname - 1)
+			 */
+			if (spec - ifname - 1 >= IFNAMSIZ)
+				goto bad;
+
 		}
 
 		if (ifname == buf + 1)		/* Interface without address */
-- 
@@ -256,11 +256,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			goto bad;
 
 		if ((ifname = strchr(buf, '%'))) {
-			if (spec - ifname >= IFNAMSIZ - 1)
-				goto bad;
-
 			*ifname = 0;
 			ifname++;
+
+			/* spec is already advanced one past the '/',
+			 * so the length of the given ifname is:
+			 * (spec - ifname - 1)
+			 */
+			if (spec - ifname - 1 >= IFNAMSIZ)
+				goto bad;
+
 		}
 
 		if (ifname == buf + 1)		/* Interface without address */
-- 
2.41.0

Re: [PATCH 0/2] Fix bugs in validation of interface names

[Stefano Brivio]

On Wed, 28 Jun 2023 15:11:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We have a number of off by one bugs when checking the lengths of
> networking interface names.
> 
> David Gibson (2):
>   conf: Fix size checking of -I interface name
>   conf: Correct length checking of interface names in conf_ports()
> 
>  conf.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Both applied, thanks.

-- 
Stefano