[PATCH 0/7] netfilter: refactor deprecated strncpy

[PATCH 0/7] netfilter: refactor deprecated strncpy

[Justin Stitt]

`strncpy` is deprecated for use on NUL-terminated destination strings [1].

A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on its destination buffer argument which is
_not_ the case for `strncpy`!

This series of patches aims to swap out `strncpy` for more robust and
less ambiguous interfaces like `strscpy` and `strtomem`. This patch
series, if applied in its entirety, removes most if not all instances of
`strncpy` in the `net/netfilter` directory.

[1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[2]: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html

Link: https://github.com/KSPP/linux/issues/90
---
Justin Stitt (7):
      netfilter: ipset: refactor deprecated strncpy
      netfilter: nf_tables: refactor deprecated strncpy
      netfilter: nf_tables: refactor deprecated strncpy
      netfilter: nft_meta: refactor deprecated strncpy
      netfilter: nft_osf: refactor deprecated strncpy to strscpy
      netfilter: x_tables: refactor deprecated strncpy
      netfilter: xtables: refactor deprecated strncpy

 net/netfilter/ipset/ip_set_core.c | 10 +++++-----
 net/netfilter/nft_ct.c            |  2 +-
 net/netfilter/nft_fib.c           |  2 +-
 net/netfilter/nft_meta.c          |  6 +++---
 net/netfilter/nft_osf.c           |  6 +++---
 net/netfilter/x_tables.c          |  5 ++---
 net/netfilter/xt_repldata.h       |  2 +-
 7 files changed, 16 insertions(+), 17 deletions(-)
---
base-commit: 14f9643dc90adea074a0ffb7a17d337eafc6a5cc
change-id: 20230807-net-netfilter-4027219bb6e7

Best regards,
--
Justin Stitt <justinstitt@google.com>

[PATCH 1/7] netfilter: ipset: refactor deprecated strncpy

[Justin Stitt]

Fixes several buffer overread bugs present in `ip_set_core.c` by using
`strscpy` over `strncpy`.

Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>

---
There exists several potential buffer overread bugs here. These bugs
exist due to the fact that the destination and source strings may have
the same length which is equal to the max length `IPSET_MAXNAMELEN`.

Here's an example:
|  #define MAXLEN 5
|  char dest[MAXLEN];
|  const char *src = "hello";
|  strncpy(dest, src, MAXLEN); // -> should use strscpy()
|  // dest is now not NUL-terminated

Note:
This patch means that truncation now happens silently (which is better
than a silent bug) but perhaps we should have some assertions that
fail when a truncation is imminent. Thoughts?
---
 net/netfilter/ipset/ip_set_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 0b68e2e2824e..fc77080d41a2 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -872,7 +872,7 @@ ip_set_name_byindex(struct net *net, ip_set_id_t index, char *name)
 	BUG_ON(!set);
 
 	read_lock_bh(&ip_set_ref_lock);
-	strncpy(name, set->name, IPSET_MAXNAMELEN);
+	strscpy(name, set->name, IPSET_MAXNAMELEN);
 	read_unlock_bh(&ip_set_ref_lock);
 }
 EXPORT_SYMBOL_GPL(ip_set_name_byindex);
@@ -1326,7 +1326,7 @@ static int ip_set_rename(struct sk_buff *skb, const struct nfnl_info *info,
 			goto out;
 		}
 	}
-	strncpy(set->name, name2, IPSET_MAXNAMELEN);
+	strscpy(set->name, name2, IPSET_MAXNAMELEN);
 
 out:
 	write_unlock_bh(&ip_set_ref_lock);
@@ -1380,9 +1380,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
 		return -EBUSY;
 	}
 
-	strncpy(from_name, from->name, IPSET_MAXNAMELEN);
-	strncpy(from->name, to->name, IPSET_MAXNAMELEN);
-	strncpy(to->name, from_name, IPSET_MAXNAMELEN);
+	strscpy(from_name, from->name, IPSET_MAXNAMELEN);
+	strscpy(from->name, to->name, IPSET_MAXNAMELEN);
+	strscpy(to->name, from_name, IPSET_MAXNAMELEN);
 
 	swap(from->ref, to->ref);
 	ip_set(inst, from_id) = to;

-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH 1/7] netfilter: ipset: refactor deprecated strncpy

[Florian Westphal]

Justin Stitt <justinstitt@google.com> wrote:
> Fixes several buffer overread bugs present in `ip_set_core.c` by using
> `strscpy` over `strncpy`.
> 
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> ---
> There exists several potential buffer overread bugs here. These bugs
> exist due to the fact that the destination and source strings may have
> the same length which is equal to the max length `IPSET_MAXNAMELEN`.

There is no truncation.  Inputs are checked via nla_policy:

[IPSET_ATTR_SETNAME2]   = { .type = NLA_NUL_STRING, .len = IPSET_MAXNAMELEN - 1 },

Re: [PATCH 1/7] netfilter: ipset: refactor deprecated strncpy

[Kees Cook]

On Wed, Aug 09, 2023 at 01:38:55AM +0200, Florian Westphal wrote:
> Justin Stitt <justinstitt@google.com> wrote:
> > Fixes several buffer overread bugs present in `ip_set_core.c` by using
> > `strscpy` over `strncpy`.
> > 
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > 
> > ---
> > There exists several potential buffer overread bugs here. These bugs
> > exist due to the fact that the destination and source strings may have
> > the same length which is equal to the max length `IPSET_MAXNAMELEN`.
> 
> There is no truncation.  Inputs are checked via nla_policy:
> 
> [IPSET_ATTR_SETNAME2]   = { .type = NLA_NUL_STRING, .len = IPSET_MAXNAMELEN - 1 },

Ah, perfect. Yeah, so if it needs to zero-padding, but it is always
NUL-terminated, strscpy_pad() is the right replacement. Thanks!

-- 
Kees Cook

[PATCH 2/7] netfilter: nf_tables: refactor deprecated strncpy

[Justin Stitt]

Prefer `strscpy` over `strncpy`.

Signed-off-by: Justin Stitt <justinstitt@google.com>

---
Note:
It is hard to tell if there was a bug here in the first place but it's better
to use a more robust and less ambiguous interface anyways.

`helper->name` has a size of 16 and the 3rd argument to `strncpy`
(NF_CT_HELPER_LEN) is also 16. This means that depending on where
`dest`'s offset is relative to `regs->data` which has a length of 20,
there may be a chance the dest buffer ends up non NUL-terminated. This
is probably fine though as the destination buffer in this case may be
fine being non NUL-terminated. If this is the case, we should probably
opt for `strtomem` instead of `strscpy`.
---
 net/netfilter/nft_ct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 38958e067aa8..10126559038b 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -108,7 +108,7 @@ static void nft_ct_get_eval(const struct nft_expr *expr,
 		helper = rcu_dereference(help->helper);
 		if (helper == NULL)
 			goto err;
-		strncpy((char *)dest, helper->name, NF_CT_HELPER_NAME_LEN);
+		strscpy((char *)dest, helper->name, NF_CT_HELPER_NAME_LEN);
 		return;
 #ifdef CONFIG_NF_CONNTRACK_LABELS
 	case NFT_CT_LABELS: {

-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH 2/7] netfilter: nf_tables: refactor deprecated strncpy

[Florian Westphal]

Justin Stitt <justinstitt@google.com> wrote:
> Prefer `strscpy` over `strncpy`.

Just like all other nft_*.c changes, this relies on zeroing
the remaining buffer, so please use strscpy_pad if this is
really needed for some reason.

Re: [PATCH 2/7] netfilter: nf_tables: refactor deprecated strncpy

[Justin Stitt]

On Tue, Aug 8, 2023 at 4:40 PM Florian Westphal <fw@strlen.de> wrote:
>
> Justin Stitt <justinstitt@google.com> wrote:
> > Prefer `strscpy` over `strncpy`.
>
> Just like all other nft_*.c changes, this relies on zeroing
> the remaining buffer, so please use strscpy_pad if this is
> really needed for some reason.
I'm soon sending a v2 series that prefers `strscpy_pad` to `strscpy`
as per Florian and Kees' feedback.

It should be noted that there was a similar refactor that took place
in this tree as well [1]. Wolfram Sang opted for `strscpy` as a
replacement to `strlcpy`. I assume the zero-padding is not needed in
such instances, right?

[1]: https://lore.kernel.org/all/20220818210224.8563-1-wsa+renesas@sang-engineering.com/

Thanks.

[PATCH 3/7] netfilter: nf_tables: refactor deprecated strncpy

[Justin Stitt]

Prefer `strscpy` over `strncpy`.

Signed-off-by: Justin Stitt <justinstitt@google.com>

---
Note:
`strscpy` is generally preferred to `strncpy` for use on NUL-terminated
destination strings. In this case, however, it is hard for me to tell if
the dest buffer wants to be NUL-terminated or not. If NUL-termination is
not needed behavior here, let's use `strtomem`.
---
 net/netfilter/nft_fib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nft_fib.c b/net/netfilter/nft_fib.c
index 6e049fd48760..f1a3692f2dbd 100644
--- a/net/netfilter/nft_fib.c
+++ b/net/netfilter/nft_fib.c
@@ -150,7 +150,7 @@ void nft_fib_store_result(void *reg, const struct nft_fib *priv,
 		if (priv->flags & NFTA_FIB_F_PRESENT)
 			*dreg = !!dev;
 		else
-			strncpy(reg, dev ? dev->name : "", IFNAMSIZ);
+			strscpy(reg, dev ? dev->name : "", IFNAMSIZ);
 		break;
 	default:
 		WARN_ON_ONCE(1);

-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH 3/7] netfilter: nf_tables: refactor deprecated strncpy

[Florian Westphal]

Justin Stitt <justinstitt@google.com> wrote:
> Prefer `strscpy` over `strncpy`.

No, this relies on zeroing out the entire register.

If you absolutely have to do this, use _pad version.

[PATCH 4/7] netfilter: nft_meta: refactor deprecated strncpy

[Justin Stitt]

Prefer `strscpy` to `strncpy` since it's more robust and less ambiguous.

Signed-off-by: Justin Stitt <justinstitt@google.com>

---
Note:
I wasn't able to tell what the expected size of
`out->rtnl_link_ops->kind` is. If it is less than or equal to `IFNAMSIZ`
then there was no bug present and a bug present otherwise. Nonetheless,
let's swap over to strscpy.
---
 net/netfilter/nft_meta.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 8fdc7318c03c..de8ced05a273 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -185,12 +185,12 @@ static noinline bool nft_meta_get_eval_kind(enum nft_meta_keys key,
 	case NFT_META_IIFKIND:
 		if (!in || !in->rtnl_link_ops)
 			return false;
-		strncpy((char *)dest, in->rtnl_link_ops->kind, IFNAMSIZ);
+		strscpy((char *)dest, in->rtnl_link_ops->kind, IFNAMSIZ);
 		break;
 	case NFT_META_OIFKIND:
 		if (!out || !out->rtnl_link_ops)
 			return false;
-		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
+		strscpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
 		break;
 	default:
 		return false;
@@ -206,7 +206,7 @@ static void nft_meta_store_ifindex(u32 *dest, const struct net_device *dev)
 
 static void nft_meta_store_ifname(u32 *dest, const struct net_device *dev)
 {
-	strncpy((char *)dest, dev ? dev->name : "", IFNAMSIZ);
+	strscpy((char *)dest, dev ? dev->name : "", IFNAMSIZ);
 }
 
 static bool nft_meta_store_iftype(u32 *dest, const struct net_device *dev)

-- 
2.41.0.640.ga95def55d0-goog

[PATCH 5/7] netfilter: nft_osf: refactor deprecated strncpy to strscpy

[Justin Stitt]

Use `strscpy` over `strncpy` for NUL-terminated strings.

We can also drop the + 1 from `NFT_OSF_MAXGENRELEN + 1` since `strscpy`
will guarantee NUL-termination.

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 net/netfilter/nft_osf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_osf.c b/net/netfilter/nft_osf.c
index 70820c66b591..4844e0109a58 100644
--- a/net/netfilter/nft_osf.c
+++ b/net/netfilter/nft_osf.c
@@ -23,7 +23,7 @@ static void nft_osf_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	struct nft_osf *priv = nft_expr_priv(expr);
 	u32 *dest = &regs->data[priv->dreg];
 	struct sk_buff *skb = pkt->skb;
-	char os_match[NFT_OSF_MAXGENRELEN + 1];
+	char os_match[NFT_OSF_MAXGENRELEN];
 	const struct tcphdr *tcp;
 	struct nf_osf_data data;
 	struct tcphdr _tcph;
@@ -45,7 +45,7 @@ static void nft_osf_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	}
 
 	if (!nf_osf_find(skb, nf_osf_fingers, priv->ttl, &data)) {
-		strncpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN);
+		strscpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN);
 	} else {
 		if (priv->flags & NFT_OSF_F_VERSION)
 			snprintf(os_match, NFT_OSF_MAXGENRELEN, "%s:%s",
@@ -53,7 +53,7 @@ static void nft_osf_eval(const struct nft_expr *expr, struct nft_regs *regs,
 		else
 			strscpy(os_match, data.genre, NFT_OSF_MAXGENRELEN);
 
-		strncpy((char *)dest, os_match, NFT_OSF_MAXGENRELEN);
+		strscpy((char *)dest, os_match, NFT_OSF_MAXGENRELEN);
 	}
 }
 

-- 
2.41.0.640.ga95def55d0-goog

[PATCH 6/7] netfilter: x_tables: refactor deprecated strncpy

[Justin Stitt]

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 <justinstitt@google.com>

---
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

Re: [PATCH 6/7] netfilter: x_tables: refactor deprecated strncpy

[Florian Westphal]

Justin Stitt <justinstitt@google.com> 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 <justinstitt@google.com>
> 
> ---
> 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.

Re: [PATCH 6/7] netfilter: x_tables: refactor deprecated strncpy

[Kees Cook]

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 <justinstitt@google.com>
> 
> ---
> 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

[PATCH 7/7] netfilter: xtables: refactor deprecated strncpy

[Justin Stitt]

Prefer `strscpy` as it's a more robust interface.

There may have existed a bug here due to both `tbl->repl.name` and
`info->name` having a size of 32 as defined below:
|  #define XT_TABLE_MAXNAMELEN 32

This may lead to buffer overreads in some situations -- `strscpy` solves
this by guaranteeing NUL-termination of the dest buffer.

Signed-off-by: Justin Stitt <justinstitt@google.com>

---
Note: build tested only
---
 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 68ccbe50bb1e..63869fd0ec57 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -29,7 +29,7 @@
 	if (tbl == NULL) \
 		return NULL; \
 	term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
-	strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
+	strscpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
 	*term = (struct type##_error)typ2##_ERROR_INIT;  \
 	tbl->repl.valid_hooks = hook_mask; \
 	tbl->repl.num_entries = nhooks + 1; \

-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH 7/7] netfilter: xtables: refactor deprecated strncpy

[Jan Engelhardt]

On Wednesday 2023-08-09 00:48, Justin Stitt wrote:

>Prefer `strscpy` as it's a more robust interface.
>
>There may have existed a bug here due to both `tbl->repl.name` and
>`info->name` having a size of 32 as defined below:
>|  #define XT_TABLE_MAXNAMELEN 32
>
>This may lead to buffer overreads in some situations -- `strscpy` solves
>this by guaranteeing NUL-termination of the dest buffer.

It generally will not lead to overreads.
xt not only deals with strings on its own turf, it even takes
them from userspace-provided buffers, which means extra scrutiny is
absolutely required. Done in places like

x_tables.c:     if (strnlen(name, XT_EXTENSION_MAXNAMELEN) == XT_EXTENSION_MAXNAMELEN)


(Which is not to say the strncpy->strscpy mop-up is bad.)