4:14 p.m.
Some l_rtnl functions that return a stringified address could
potentially overrun a user supplied buffer of insufficient length.
Add a buffer length argument to prevent ELL from making incorrect
assumptions. Impacted functions are l_rtnl_address_get_address(),
l_rtnl_route_get_gateway(), and l_rtnl_route_get_prefsrc().
Ossama Othman (3):
rtnl: Fix address conversion buffer overflow.
unit: Add l_rtnl_address_get_address() test case.
Makefile: Bump library 'current' version.
Makefile.am | 4 ++--
ell/rtnl.c | 28 +++++++++++++++++-----------
ell/rtnl.h | 8 +++++---
unit/test-rtnl.c | 26 ++++++++++++++++++++++++++
4 files changed, 50 insertions(+), 16 deletions(-)
--
2.32.0
4:14 p.m.
New subject: [PATCH 1/3] rtnl: Fix address conversion buffer overflow.
Some l_rtnl functions that return a stringified address could
potentially overrun a user supplied buffer of insufficient length.
Add a buffer length argument to prevent ELL from making incorrect
assumptions. Impacted functions are l_rtnl_address_get_address(),
l_rtnl_route_get_gateway(), and l_rtnl_route_get_prefsrc().
---
ell/rtnl.c | 28 +++++++++++++++++-----------
ell/rtnl.h | 8 +++++---
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/ell/rtnl.c b/ell/rtnl.c
index 4a1a248..bfda632 100644
--- a/ell/rtnl.c
+++ b/ell/rtnl.c
@@ -58,21 +58,24 @@ struct l_rtnl_address {
static inline int address_to_string(int family, const struct in_addr *v4,
const struct in6_addr *v6,
- char *out_address)
+ char *out_buf, size_t buflen)
{
+ const void *src = NULL;
+
switch (family) {
case AF_INET:
- if (!inet_ntop(family, v4, out_address, INET_ADDRSTRLEN))
- return -errno;
+ src = v4;
break;
case AF_INET6:
- if (!inet_ntop(family, v6, out_address, INET6_ADDRSTRLEN))
- return -errno;
+ src = v6;
break;
default:
return false;
}
+ if (!inet_ntop(family, src, out_buf, buflen))
+ return -errno;
+
return 0;
}
@@ -162,14 +165,15 @@ LIB_EXPORT void l_rtnl_address_free(struct l_rtnl_address *addr)
}
LIB_EXPORT bool l_rtnl_address_get_address(const struct l_rtnl_address *addr,
- char *out_buf)
+ char *out_buf,
+ size_t buflen)
{
if (unlikely(!addr))
return false;
return !address_to_string(addr->family, &addr->in_addr,
&addr->in6_addr,
- out_buf);
+ out_buf, buflen);
}
LIB_EXPORT uint8_t l_rtnl_address_get_family(const struct l_rtnl_address *addr)
@@ -394,13 +398,14 @@ LIB_EXPORT uint8_t l_rtnl_route_get_family(const struct l_rtnl_route
*rt)
}
LIB_EXPORT bool l_rtnl_route_get_gateway(const struct l_rtnl_route *rt,
- char *out_buf)
+ char *out_buf,
+ size_t buflen)
{
if (unlikely(!rt))
return false;
return !address_to_string(rt->family, &rt->gw.in_addr,
&rt->gw.in6_addr,
- out_buf);
+ out_buf, buflen);
}
LIB_EXPORT uint32_t l_rtnl_route_get_lifetime(const struct l_rtnl_route *rt)
@@ -461,7 +466,8 @@ LIB_EXPORT bool l_rtnl_route_set_preference(struct l_rtnl_route *rt,
}
LIB_EXPORT bool l_rtnl_route_get_prefsrc(const struct l_rtnl_route *rt,
- char *out_address)
+ char *out_buf,
+ size_t buflen)
{
if (unlikely(!rt))
return false;
@@ -473,7 +479,7 @@ LIB_EXPORT bool l_rtnl_route_get_prefsrc(const struct l_rtnl_route
*rt,
return !address_to_string(rt->family, &rt->prefsrc.in_addr,
&rt->prefsrc.in6_addr,
- out_address);
+ out_buf, buflen);
}
LIB_EXPORT bool l_rtnl_route_set_prefsrc(struct l_rtnl_route *rt,
diff --git a/ell/rtnl.h b/ell/rtnl.h
index 274816c..802692b 100644
--- a/ell/rtnl.h
+++ b/ell/rtnl.h
@@ -43,7 +43,7 @@ struct l_rtnl_address *l_rtnl_address_clone(const struct l_rtnl_address
*orig);
void l_rtnl_address_free(struct l_rtnl_address *addr);
DEFINE_CLEANUP_FUNC(l_rtnl_address_free);
bool l_rtnl_address_get_address(const struct l_rtnl_address *addr,
- char *out_buf);
+ char *out_buf, size_t buflen);
uint8_t l_rtnl_address_get_family(const struct l_rtnl_address *addr);
uint8_t l_rtnl_address_get_prefix_length(const struct l_rtnl_address *addr);
bool l_rtnl_address_get_broadcast(const struct l_rtnl_address *addr,
@@ -68,14 +68,16 @@ struct l_rtnl_route *l_rtnl_route_new_prefix(const char *ip,
void l_rtnl_route_free(struct l_rtnl_route *rt);
DEFINE_CLEANUP_FUNC(l_rtnl_route_free);
uint8_t l_rtnl_route_get_family(const struct l_rtnl_route *rt);
-bool l_rtnl_route_get_gateway(const struct l_rtnl_route *rt, char *out_buf);
+bool l_rtnl_route_get_gateway(const struct l_rtnl_route *rt, char *out_buf,
+ size_t buflen);
uint32_t l_rtnl_route_get_lifetime(const struct l_rtnl_route *rt);
bool l_rtnl_route_set_lifetime(struct l_rtnl_route *rt, uint32_t lt);
uint32_t l_rtnl_route_get_mtu(const struct l_rtnl_route *rt);
bool l_rtnl_route_set_mtu(struct l_rtnl_route *rt, uint32_t mtu);
uint8_t l_rtnl_route_get_preference(const struct l_rtnl_route *rt);
bool l_rtnl_route_set_preference(struct l_rtnl_route *rt, uint8_t preference);
-bool l_rtnl_route_get_prefsrc(const struct l_rtnl_route *rt, char *out_address);
+bool l_rtnl_route_get_prefsrc(const struct l_rtnl_route *rt, char *out_buf,
+ size_t buflen);
bool l_rtnl_route_set_prefsrc(struct l_rtnl_route *rt, const char *address);
uint32_t l_rtnl_route_get_priority(const struct l_rtnl_route *rt);
bool l_rtnl_route_set_priority(struct l_rtnl_route *rt, uint32_t priority);
--
2.32.0
7:55 a.m.
New subject: [PATCH 1/3] rtnl: Fix address conversion buffer overflow.
Hi Ossama,
On 11/9/21 6:14 PM, Ossama Othman wrote:
Some l_rtnl functions that return a stringified address could
potentially overrun a user supplied buffer of insufficient length.
Add a buffer length argument to prevent ELL from making incorrect
assumptions. Impacted functions are l_rtnl_address_get_address(),
l_rtnl_route_get_gateway(), and l_rtnl_route_get_prefsrc().
---
ell/rtnl.c | 28 +++++++++++++++++-----------
ell/rtnl.h | 8 +++++---
2 files changed, 22 insertions(+), 14 deletions(-)
<snip>
bool l_rtnl_address_get_address(const struct l_rtnl_address *addr,
- char *out_buf);
+ char *out_buf, size_t buflen);
So I'm a little torn about this. On the one hand, specifying the buffer len is
indeed safer. However, simply passing a buffer of size INET6_ADDRSTRLEN is also
guaranteed to work for any address that we support and saves the need for the
extra argument. Alternatively, if you know the family or can look it up via
l_rtnl_*_get_family(), you can use INET_ADDRSTRLEN for ipv4 getters. Maybe this
is more of a documentation issue?
If you look at how iwd uses these functions, the extra argument is unnecessary.
Can you share more info about what made you propose this change? Are you using
these functions in ways where the buffer size argument would be advantageous?
Regards,
-Denis
11:10 a.m.
New subject: [PATCH 1/3] rtnl: Fix address conversion buffer overflow.
Re-sending to the mailing list. My original response was inadvertently sent to
Denis alone. Apologies.
Hi Denis,
On Wed, Nov 10, 2021 at 8:13 AM Denis Kenzior <denkenz(a)gmail.com> wrote:
So I'm a little torn about this. On the one hand, specifying the
buffer len is
indeed safer. However, simply passing a buffer of size INET6_ADDRSTRLEN is also
guaranteed to work for any address that we support and saves the need for the
extra argument. Alternatively, if you know the family or can look it up via
l_rtnl_*_get_family(), you can use INET_ADDRSTRLEN for ipv4 getters. Maybe this
is more of a documentation issue?
If you look at how iwd uses these functions, the extra argument is unnecessary.
Can you share more info about what made you propose this change? Are you using
these functions in ways where the buffer size argument would be advantageous?
My own usage for the ELL l_rtnl API is still experimental since I was
looking to replace direct rtnetlink calls in mptcpd's network
monitoring code with l_rtnl based ones. In this particular case, I
was using l_rtnl_address_get_address() solely for logging purposes.
I agree that passing a buffer of length INET6_ADDRSTRLEN is sufficient
for both IPv4 and IPv6 cases. Mptcpd does this, too. Regardless, I
felt that a public library API shouldn't be making assumptions about
user supplied buffer lengths. Other than making the user responsible
for potential buffer overrun issues, there is no other advantage I can
think of.
-Ossama
On Wed, Nov 10, 2021 at 8:13 AM Denis Kenzior <denkenz(a)gmail.com> wrote:
>
> Hi Ossama,
>
> On 11/9/21 6:14 PM, Ossama Othman wrote:
> > Some l_rtnl functions that return a stringified address could
> > potentially overrun a user supplied buffer of insufficient length.
> > Add a buffer length argument to prevent ELL from making incorrect
> > assumptions. Impacted functions are l_rtnl_address_get_address(),
> > l_rtnl_route_get_gateway(), and l_rtnl_route_get_prefsrc().
> > ---
> > ell/rtnl.c | 28 +++++++++++++++++-----------
> > ell/rtnl.h | 8 +++++---
> > 2 files changed, 22 insertions(+), 14 deletions(-)
> >
>
> <snip>
>
> > bool l_rtnl_address_get_address(const struct l_rtnl_address *addr,
> > - char *out_buf);
> > + char *out_buf, size_t buflen);
>
So I'm a little torn about this. On the one hand, specifying the
buffer len is
indeed safer. However, simply passing a buffer of size INET6_ADDRSTRLEN is also
guaranteed to work for any address that we support and saves the need for the
extra argument. Alternatively, if you know the family or can look it up via
l_rtnl_*_get_family(), you can use INET_ADDRSTRLEN for ipv4 getters. Maybe this
is more of a documentation issue?
If you look at how iwd uses these functions, the extra argument is unnecessary.
Can you share more info about what made you propose this change? Are you using
these functions in ways where the buffer size argument would be advantageous?
>
> Regards,
> -Denis
4:14 p.m.
New subject: [PATCH 2/3] unit: Add l_rtnl_address_get_address() test case.
---
unit/test-rtnl.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/unit/test-rtnl.c b/unit/test-rtnl.c
index 107f69a..9826272 100644
--- a/unit/test-rtnl.c
+++ b/unit/test-rtnl.c
@@ -31,6 +31,7 @@
#include <stdlib.h>
#include <signal.h>
#include <sys/wait.h>
+#include <netinet/in.h>
#include <ell/ell.h>
#include "ell/dbus-private.h"
@@ -232,6 +233,30 @@ static void test_ifaddr6_dump(struct l_netlink *rntl, void
*user_data)
NULL, ifaddr6_dump_destroy_cb));
}
+static void test_get_address(struct l_netlink *rntl, void *user_data)
+{
+ static const char ip[] = "192.0.2.1";
+ static const uint8_t prefix_len = 32;
+ struct l_rtnl_address *addr = NULL;
+ size_t buflen, badlen;
+
+ char buf[INET_ADDRSTRLEN];
+ buflen = L_ARRAY_SIZE(buf);
+ badlen = 4;
+
+ addr = l_rtnl_address_new(ip, prefix_len);
+ test_assert(addr);
+
+ test_assert(l_rtnl_address_get_address(addr, buf, buflen));
+ test_assert(!l_rtnl_address_get_address(addr, buf, badlen));
+
+ test_assert(strcmp(ip, buf) == 0);
+
+ l_rtnl_address_free(addr);
+
+ test_next();
+}
+
static void test_run(void)
{
success = false;
@@ -249,6 +274,7 @@ int main(int argc, char *argv[])
test_add("Dump IPv6 routing table", test_route6_dump, NULL);
test_add("Dump IPv4 addresses", test_ifaddr4_dump, NULL);
test_add("Dump IPv6 addresses", test_ifaddr6_dump, NULL);
+ test_add("Get address", test_get_address, NULL);
l_log_set_stderr();
--
2.32.0
4:14 p.m.
New subject: [PATCH 3/3] Makefile: Bump library 'current' version.
The l_rtnl interface changed. Bump the "current" ELL library version
number accordingly.
---
Makefile.am | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 4e650c9..73f5298 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -8,8 +8,8 @@ ACLOCAL_AMFLAGS = -I build-aux
# Interfaces added: CURRENT++ REVISION=0 AGE++
# Interfaces removed: CURRENT++ REVISION=0 AGE=0
-ELL_CURRENT = 0
-ELL_REVISION = 2
+ELL_CURRENT = 1
+ELL_REVISION = 0
ELL_AGE = 0
linux_headers = linux/gpio.h
--
2.32.0
9:58 p.m.
New subject: [PATCH 3/3] Makefile: Bump library 'current' version.
Hi Ossama,
The l_rtnl interface changed. Bump the "current" ELL
library version
number accordingly.
---
Makefile.am | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 4e650c9..73f5298 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -8,8 +8,8 @@ ACLOCAL_AMFLAGS = -I build-aux
# Interfaces added: CURRENT++ REVISION=0 AGE++
# Interfaces removed: CURRENT++ REVISION=0 AGE=0
-ELL_CURRENT = 0
-ELL_REVISION = 2
+ELL_CURRENT = 1
+ELL_REVISION = 0
ELL_AGE = 0
actually we have bothered changing this as long as we are still 0.x and not stable. So I
rather skip this for now.
Regards
Marcel
9:56 a.m.
New subject: [PATCH 3/3] Makefile: Bump library 'current' version.
Hi Marcel,
On Tue, Nov 9, 2021 at 9:58 PM Marcel Holtmann <marcel(a)holtmann.org> wrote:
> -ELL_CURRENT = 0
> -ELL_REVISION = 2
> +ELL_CURRENT = 1
> +ELL_REVISION = 0
> ELL_AGE = 0
actually we have bothered changing this as long as we are still 0.x and not stable. So I
rather skip this for now.
Okay, sounds good.
-Ossama
269
days inactive
274
days old
7 comments
3 participants
participants (3)
-
Denis Kenzior -
Marcel Holtmann -
Ossama Othman