Re: [PATCH] netfilter: fix buffer null termination

+ netdev

On Sat, Jun 11, 2016 at 10:18 AM, Kishan Sandeep
[off-list ref] wrote:

Cc'ing netfilter-devel.

On Tue, Jun 14, 2016 at 07:39:27PM +0530, Kishan Sandeep wrote:

+ netdev

On Sat, Jun 11, 2016 at 10:18 AM, Kishan Sandeep
[off-list ref] wrote:

quoted

strncpy generally perferable fo non-terminated
fixed-width strings. For NULL termination strlcpy
is preferrable.

Signed-off-by: Kishan Sandeep <redacted>
---
 net/netfilter/xt_repldata.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 8fd3241..a460211 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h

@@ -28,7 +28,7 @@
        if (tbl == NULL) \
                return NULL; \
        term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
-       strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
+       strlcpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \

I don't think this is actually fixing anything. Tables in x_tables
have a known and fixed name that is defined from the kernel side, that
is always smaller that the buffer we have there. So are you observing
any real problem from there?

Thanks.

Hi Pablo,

On Tue, Jun 14, 2016 at 8:38 PM, Pablo Neira Ayuso [off-list ref] wrote:

Cc'ing netfilter-devel.

On Tue, Jun 14, 2016 at 07:39:27PM +0530, Kishan Sandeep wrote:

quoted

+ netdev

On Sat, Jun 11, 2016 at 10:18 AM, Kishan Sandeep
[off-list ref] wrote:

quoted

strncpy generally perferable fo non-terminated
fixed-width strings. For NULL termination strlcpy
is preferrable.

Signed-off-by: Kishan Sandeep <redacted>
---
 net/netfilter/xt_repldata.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 8fd3241..a460211 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h

@@ -28,7 +28,7 @@
        if (tbl == NULL) \
                return NULL; \
        term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
-       strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
+       strlcpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \

I don't think this is actually fixing anything. Tables in x_tables
have a known and fixed name that is defined from the kernel side, that
is always smaller that the buffer we have there. So are you observing
any real problem from there?

Thanks.

Not observed any real problem. Here the string is not NULL terminated with
the use of strncpy - that is the problem. With the use of strlcpy we can make
the string to terminated properly.

Thanks!

On Tue, Jun 14, 2016 at 09:52:49PM +0530, Kishan Sandeep wrote:

Hi Pablo,

On Tue, Jun 14, 2016 at 8:38 PM, Pablo Neira Ayuso [off-list ref] wrote:

quoted

Cc'ing netfilter-devel.

On Tue, Jun 14, 2016 at 07:39:27PM +0530, Kishan Sandeep wrote:

quoted

+ netdev

On Sat, Jun 11, 2016 at 10:18 AM, Kishan Sandeep
[off-list ref] wrote:

quoted

strncpy generally perferable fo non-terminated
fixed-width strings. For NULL termination strlcpy
is preferrable.

Signed-off-by: Kishan Sandeep <redacted>
---
 net/netfilter/xt_repldata.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 8fd3241..a460211 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h

@@ -28,7 +28,7 @@
        if (tbl == NULL) \
                return NULL; \
        term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
-       strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
+       strlcpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \

I don't think this is actually fixing anything. Tables in x_tables
have a known and fixed name that is defined from the kernel side, that
is always smaller that the buffer we have there. So are you observing
any real problem from there?

Thanks.

Not observed any real problem. Here the string is not NULL terminated with
the use of strncpy - that is the problem. With the use of strlcpy we can make
the string to terminated properly.

1) Table names set from the kernel codebase, so they are always
   way smaller than that.

2) We're not expecting the addition of new tables in the future that
   we result in the hypothetical problem that you indicate.

Thanks.