Prefer `strscpy` to `strncpy` for use on NUL-terminated destination
buffers.
This fixes a potential bug due to the fact that both `t->u.user.name`
and `name` share the same size.
Signed-off-by: Justin Stitt <[email protected]>
---
Here's an example of what happens when dest and src share same size:
| #define MAXLEN 5
| char dest[MAXLEN];
| const char *src = "hello";
| strncpy(dest, src, MAXLEN); // -> should use strscpy()
| // dest is now not NUL-terminated
---
net/netfilter/x_tables.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 470282cf3fae..714a38ec9055 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -768,7 +768,7 @@ void xt_compat_match_from_user(struct xt_entry_match *m, void **dstptr,
m->u.user.match_size = msize;
strscpy(name, match->name, sizeof(name));
module_put(match->me);
- strncpy(m->u.user.name, name, sizeof(m->u.user.name));
+ strscpy(m->u.user.name, name, sizeof(m->u.user.name));
*size += off;
*dstptr += msize;
@@ -1148,7 +1148,7 @@ void xt_compat_target_from_user(struct xt_entry_target *t, void **dstptr,
t->u.user.target_size = tsize;
strscpy(name, target->name, sizeof(name));
module_put(target->me);
- strncpy(t->u.user.name, name, sizeof(t->u.user.name));
+ strscpy(t->u.user.name, name, sizeof(t->u.user.name));
*size += off;
*dstptr += tsize;
@@ -2014,4 +2014,3 @@ static void __exit xt_fini(void)
module_init(xt_init);
module_exit(xt_fini);
-
--
2.41.0.640.ga95def55d0-goog
Justin Stitt <[email protected]> wrote:
> Prefer `strscpy` to `strncpy` for use on NUL-terminated destination
> buffers.
>
> This fixes a potential bug due to the fact that both `t->u.user.name`
> and `name` share the same size.
This replacement seems fine.
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Here's an example of what happens when dest and src share same size:
> | #define MAXLEN 5
> | char dest[MAXLEN];
> | const char *src = "hello";
> | strncpy(dest, src, MAXLEN); // -> should use strscpy()
> | // dest is now not NUL-terminated
This can't happen here, the source string is coming from the kernel
(xt target and matchinfo struct).
But, even if it would it should be fine, this function prepares
the translated 64bit blob which gets passed to translate_table(),
and that function has to check for '\0' presence.
Normally it handles the native (non-compat) data originating from
userspace, so m-->user.name can not be assumed to contain a \0.
On Tue, Aug 08, 2023 at 10:48:11PM +0000, Justin Stitt wrote:
> Prefer `strscpy` to `strncpy` for use on NUL-terminated destination
> buffers.
>
> This fixes a potential bug due to the fact that both `t->u.user.name`
> and `name` share the same size.
>
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Here's an example of what happens when dest and src share same size:
> | #define MAXLEN 5
> | char dest[MAXLEN];
> | const char *src = "hello";
> | strncpy(dest, src, MAXLEN); // -> should use strscpy()
> | // dest is now not NUL-terminated
> ---
> net/netfilter/x_tables.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 470282cf3fae..714a38ec9055 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -768,7 +768,7 @@ void xt_compat_match_from_user(struct xt_entry_match *m, void **dstptr,
> m->u.user.match_size = msize;
> strscpy(name, match->name, sizeof(name));
> module_put(match->me);
> - strncpy(m->u.user.name, name, sizeof(m->u.user.name));
> + strscpy(m->u.user.name, name, sizeof(m->u.user.name));
Two hints here are that this is dealing with user-space memory copies, so
NUL-padding is needed (i.e. don't leak memory contents), and immediately
above is a strscpy() already, which means there is likely some reason
strncpy() is needed (i.e. padding).
--
Kees Cook