* [libnftnl PATCH 2/2] Avoid out of bounds read from data
@ 2021-02-17 20:46 Maya Rashish
2021-02-17 23:01 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: Maya Rashish @ 2021-02-17 20:46 UTC (permalink / raw)
To: netfilter-devel
This might introduce some issues since we're now not
filling the rest of the memory, but filling out with
uninitialized garbage is probably as bad as leaving it
as garbage.
Signed-off-by: Maya Rashish <mrashish@redhat.com>
---
include/utils.h | 2 ++
src/expr/counter.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/utils.h b/include/utils.h
index 8af5a8e..7413534 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -67,6 +67,8 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
#define array_size(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+#define MIN(a,b) (a>b ? (b) : (a))
+
const char *nftnl_family2str(uint32_t family);
int nftnl_str2family(const char *family);
diff --git a/src/expr/counter.c b/src/expr/counter.c
index 89a602e..fb036dd 100644
--- a/src/expr/counter.c
+++ b/src/expr/counter.c
@@ -35,10 +35,10 @@ nftnl_expr_counter_set(struct nftnl_expr *e, uint16_t type,
switch(type) {
case NFTNL_EXPR_CTR_BYTES:
- memcpy(&ctr->bytes, data, sizeof(ctr->bytes));
+ memcpy(&ctr->bytes, data, MIN(data_len, sizeof(ctr->bytes)));
break;
case NFTNL_EXPR_CTR_PACKETS:
- memcpy(&ctr->pkts, data, sizeof(ctr->pkts));
+ memcpy(&ctr->pkts, data, MIN(data_len, sizeof(ctr->pkts)));
break;
default:
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [libnftnl PATCH 2/2] Avoid out of bounds read from data
2021-02-17 20:46 [libnftnl PATCH 2/2] Avoid out of bounds read from data Maya Rashish
@ 2021-02-17 23:01 ` Pablo Neira Ayuso
2021-02-18 11:06 ` [libnftnl PATCH 2/2 v2] " Maya Rashish
0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2021-02-17 23:01 UTC (permalink / raw)
To: Maya Rashish; +Cc: netfilter-devel
On Wed, Feb 17, 2021 at 10:46:50PM +0200, Maya Rashish wrote:
> This might introduce some issues since we're now not
> filling the rest of the memory, but filling out with
> uninitialized garbage is probably as bad as leaving it
> as garbage.
What are you trying to fix?
Caller sends pass data as parameter that is smaller than
sizeof(ctr->byte)?
Thanks.
> Signed-off-by: Maya Rashish <mrashish@redhat.com>
> ---
> include/utils.h | 2 ++
> src/expr/counter.c | 4 ++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/utils.h b/include/utils.h
> index 8af5a8e..7413534 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -67,6 +67,8 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
>
> #define array_size(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>
> +#define MIN(a,b) (a>b ? (b) : (a))
> +
> const char *nftnl_family2str(uint32_t family);
> int nftnl_str2family(const char *family);
>
> diff --git a/src/expr/counter.c b/src/expr/counter.c
> index 89a602e..fb036dd 100644
> --- a/src/expr/counter.c
> +++ b/src/expr/counter.c
> @@ -35,10 +35,10 @@ nftnl_expr_counter_set(struct nftnl_expr *e, uint16_t type,
>
> switch(type) {
> case NFTNL_EXPR_CTR_BYTES:
> - memcpy(&ctr->bytes, data, sizeof(ctr->bytes));
> + memcpy(&ctr->bytes, data, MIN(data_len, sizeof(ctr->bytes)));
> break;
> case NFTNL_EXPR_CTR_PACKETS:
> - memcpy(&ctr->pkts, data, sizeof(ctr->pkts));
> + memcpy(&ctr->pkts, data, MIN(data_len, sizeof(ctr->pkts)));
> break;
> default:
> return -1;
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [libnftnl PATCH 2/2 v2] Avoid out of bounds read from data
2021-02-17 23:01 ` Pablo Neira Ayuso
@ 2021-02-18 11:06 ` Maya Rashish
2021-02-19 0:18 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: Maya Rashish @ 2021-02-18 11:06 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
When data is smaller than the destination, &ctr->pkts.
This might introduce some issues since we're now not
filling the rest of the memory, but filling out with
uninitialized garbage is probably as bad as leaving it
as garbage.
Signed-off-by: Maya Rashish <mrashish@redhat.com>
---
include/utils.h | 2 ++
src/expr/counter.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/utils.h b/include/utils.h
index 8af5a8e..6b22e46 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -67,6 +67,8 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
#define array_size(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+#define MIN(a,b) ((a) > (b) ? (b) : (a))
+
const char *nftnl_family2str(uint32_t family);
int nftnl_str2family(const char *family);
diff --git a/src/expr/counter.c b/src/expr/counter.c
index 89a602e..fb036dd 100644
--- a/src/expr/counter.c
+++ b/src/expr/counter.c
@@ -35,10 +35,10 @@ nftnl_expr_counter_set(struct nftnl_expr *e, uint16_t type,
switch(type) {
case NFTNL_EXPR_CTR_BYTES:
- memcpy(&ctr->bytes, data, sizeof(ctr->bytes));
+ memcpy(&ctr->bytes, data, MIN(data_len, sizeof(ctr->bytes)));
break;
case NFTNL_EXPR_CTR_PACKETS:
- memcpy(&ctr->pkts, data, sizeof(ctr->pkts));
+ memcpy(&ctr->pkts, data, MIN(data_len, sizeof(ctr->pkts)));
break;
default:
return -1;
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [libnftnl PATCH 2/2 v2] Avoid out of bounds read from data
2021-02-18 11:06 ` [libnftnl PATCH 2/2 v2] " Maya Rashish
@ 2021-02-19 0:18 ` Pablo Neira Ayuso
0 siblings, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2021-02-19 0:18 UTC (permalink / raw)
To: Maya Rashish; +Cc: netfilter-devel
Hi Maya,
On Thu, Feb 18, 2021 at 01:06:38PM +0200, Maya Rashish wrote:
> When data is smaller than the destination, &ctr->pkts.
>
> This might introduce some issues since we're now not
> filling the rest of the memory, but filling out with
> uninitialized garbage is probably as bad as leaving it
> as garbage.
Probably you could update src/expr/ to use nftnl_assert_validate() to
sanity check the input data length?
Please, have a look at nftnl_assert_attr_exists() and
nftnl_assert_validate().
> Signed-off-by: Maya Rashish <mrashish@redhat.com>
> ---
> include/utils.h | 2 ++
> src/expr/counter.c | 4 ++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/utils.h b/include/utils.h
> index 8af5a8e..6b22e46 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -67,6 +67,8 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
>
> #define array_size(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>
> +#define MIN(a,b) ((a) > (b) ? (b) : (a))
> +
> const char *nftnl_family2str(uint32_t family);
> int nftnl_str2family(const char *family);
>
> diff --git a/src/expr/counter.c b/src/expr/counter.c
> index 89a602e..fb036dd 100644
> --- a/src/expr/counter.c
> +++ b/src/expr/counter.c
> @@ -35,10 +35,10 @@ nftnl_expr_counter_set(struct nftnl_expr *e, uint16_t type,
>
> switch(type) {
> case NFTNL_EXPR_CTR_BYTES:
> - memcpy(&ctr->bytes, data, sizeof(ctr->bytes));
> + memcpy(&ctr->bytes, data, MIN(data_len, sizeof(ctr->bytes)));
> break;
> case NFTNL_EXPR_CTR_PACKETS:
> - memcpy(&ctr->pkts, data, sizeof(ctr->pkts));
> + memcpy(&ctr->pkts, data, MIN(data_len, sizeof(ctr->pkts)));
> break;
> default:
> return -1;
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-19 0:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-17 20:46 [libnftnl PATCH 2/2] Avoid out of bounds read from data Maya Rashish
2021-02-17 23:01 ` Pablo Neira Ayuso
2021-02-18 11:06 ` [libnftnl PATCH 2/2 v2] " Maya Rashish
2021-02-19 0:18 ` 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).