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
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(a)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; -- 2.41.0
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(a)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 */ -- 2.41.0
On Wed, 28 Jun 2023 15:11:13 +1000 David Gibson <david(a)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