[PATCH 1/3] netfilter: nf_tables: set names cannot be larger than 15 bytes

[PATCH 1/3] netfilter: nf_tables: set names cannot be larger than 15 bytes

[Pablo Neira Ayuso]

Currently, nf_tables trims off the set name if it exceeeds 15
bytes, so explicitly reject set names that are too large.

Reported-by: Giuseppe Longo <giuseppelng@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 33045a5..43ae487 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
 
 static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
 	[NFTA_SET_TABLE]		= { .type = NLA_STRING },
-	[NFTA_SET_NAME]			= { .type = NLA_STRING },
+	[NFTA_SET_NAME]			= { .type = NLA_STRING,
+					    .len = IFNAMSIZ - 1 },
 	[NFTA_SET_FLAGS]		= { .type = NLA_U32 },
 	[NFTA_SET_KEY_TYPE]		= { .type = NLA_U32 },
 	[NFTA_SET_KEY_LEN]		= { .type = NLA_U32 },
-- 
1.7.10.4

[PATCH 2/3] netfilter: nf_tables: fix wrong format in request_module()

[Pablo Neira Ayuso]

The intended format in request_module is .*%s instead of *.%s.

Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 43ae487..3fd159d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -152,8 +152,8 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
 #ifdef CONFIG_MODULES
 	if (autoload) {
 		nfnl_unlock(NFNL_SUBSYS_NFTABLES);
-		request_module("nft-chain-%u-%*.s", afi->family,
-			       nla_len(nla)-1, (const char *)nla_data(nla));
+		request_module("nft-chain-%u-%.*s", afi->family,
+			       nla_len(nla), (const char *)nla_data(nla));
 		nfnl_lock(NFNL_SUBSYS_NFTABLES);
 		type = __nf_tables_chain_type_lookup(afi->family, nla);
 		if (type != NULL)
-- 
1.7.10.4

[PATCH 3/3] netfilter: nf_tables: enforce NLA_NUL_STRING in strings

[Pablo Neira Ayuso]

nla_strcmp compares the string length plus one, so it's implicitly
including the nul-termination in the comparison.

 int nla_strcmp(const struct nlattr *nla, const char *str)
 {
        int len = strlen(str) + 1;
        ...
		d = memcmp(nla_data(nla), str, len);

However, if NLA_STRING is used, userspace can send us a string without
the null-termination. This is a problem since the nf_tables lookup
functions won't find any matching as the last byte may mismatch.
So we have to enforce that strings are nul-termination to avoid
mismatches.

The userspace library libnftnl always appends the nul-termination
to all strings that are sent to the kernel, that's why this bug didn't
manifest in userspace.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/uapi/linux/netfilter/nf_tables.h |   26 +++++++++++++-------------
 net/netfilter/nf_tables_api.c            |   22 +++++++++++-----------
 net/netfilter/nft_log.c                  |    2 +-
 net/netfilter/nft_lookup.c               |    2 +-
 4 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index c88ccbf..564a5ac 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -109,7 +109,7 @@ enum nft_table_flags {
 /**
  * enum nft_table_attributes - nf_tables table netlink attributes
  *
- * @NFTA_TABLE_NAME: name of the table (NLA_STRING)
+ * @NFTA_TABLE_NAME: name of the table (NLA_NUL_STRING)
  * @NFTA_TABLE_FLAGS: bitmask of enum nft_table_flags (NLA_U32)
  * @NFTA_TABLE_USE: number of chains in this table (NLA_U32)
  */
@@ -125,9 +125,9 @@ enum nft_table_attributes {
 /**
  * enum nft_chain_attributes - nf_tables chain netlink attributes
  *
- * @NFTA_CHAIN_TABLE: name of the table containing the chain (NLA_STRING)
+ * @NFTA_CHAIN_TABLE: name of the table containing the chain (NLA_NUL_STRING)
  * @NFTA_CHAIN_HANDLE: numeric handle of the chain (NLA_U64)
- * @NFTA_CHAIN_NAME: name of the chain (NLA_STRING)
+ * @NFTA_CHAIN_NAME: name of the chain (NLA_NUL_STRING)
  * @NFTA_CHAIN_HOOK: hook specification for basechains (NLA_NESTED: nft_hook_attributes)
  * @NFTA_CHAIN_POLICY: numeric policy of the chain (NLA_U32)
  * @NFTA_CHAIN_USE: number of references to this chain (NLA_U32)
@@ -151,8 +151,8 @@ enum nft_chain_attributes {
 /**
  * enum nft_rule_attributes - nf_tables rule netlink attributes
  *
- * @NFTA_RULE_TABLE: name of the table containing the rule (NLA_STRING)
- * @NFTA_RULE_CHAIN: name of the chain containing the rule (NLA_STRING)
+ * @NFTA_RULE_TABLE: name of the table containing the rule (NLA_NUL_STRING)
+ * @NFTA_RULE_CHAIN: name of the chain containing the rule (NLA_NUL_STRING)
  * @NFTA_RULE_HANDLE: numeric handle of the rule (NLA_U64)
  * @NFTA_RULE_EXPRESSIONS: list of expressions (NLA_NESTED: nft_expr_attributes)
  * @NFTA_RULE_COMPAT: compatibility specifications of the rule (NLA_NESTED: nft_rule_compat_attributes)
@@ -214,8 +214,8 @@ enum nft_set_flags {
 /**
  * enum nft_set_attributes - nf_tables set netlink attributes
  *
- * @NFTA_SET_TABLE: table name (NLA_STRING)
- * @NFTA_SET_NAME: set name (NLA_STRING)
+ * @NFTA_SET_TABLE: table name (NLA_NUL_STRING)
+ * @NFTA_SET_NAME: set name (NLA_NUL_STRING)
  * @NFTA_SET_FLAGS: bitmask of enum nft_set_flags (NLA_U32)
  * @NFTA_SET_KEY_TYPE: key data type, informational purpose only (NLA_U32)
  * @NFTA_SET_KEY_LEN: key data length (NLA_U32)
@@ -263,8 +263,8 @@ enum nft_set_elem_attributes {
 /**
  * enum nft_set_elem_list_attributes - nf_tables set element list netlink attributes
  *
- * @NFTA_SET_ELEM_LIST_TABLE: table of the set to be changed (NLA_STRING)
- * @NFTA_SET_ELEM_LIST_SET: name of the set to be changed (NLA_STRING)
+ * @NFTA_SET_ELEM_LIST_TABLE: table of the set to be changed (NLA_NUL_STRING)
+ * @NFTA_SET_ELEM_LIST_SET: name of the set to be changed (NLA_NUL_STRING)
  * @NFTA_SET_ELEM_LIST_ELEMENTS: list of set elements (NLA_NESTED: nft_set_elem_attributes)
  */
 enum nft_set_elem_list_attributes {
@@ -315,7 +315,7 @@ enum nft_data_attributes {
  * enum nft_verdict_attributes - nf_tables verdict netlink attributes
  *
  * @NFTA_VERDICT_CODE: nf_tables verdict (NLA_U32: enum nft_verdicts)
- * @NFTA_VERDICT_CHAIN: jump target chain name (NLA_STRING)
+ * @NFTA_VERDICT_CHAIN: jump target chain name (NLA_NUL_STRING)
  */
 enum nft_verdict_attributes {
 	NFTA_VERDICT_UNSPEC,
@@ -328,7 +328,7 @@ enum nft_verdict_attributes {
 /**
  * enum nft_expr_attributes - nf_tables expression netlink attributes
  *
- * @NFTA_EXPR_NAME: name of the expression type (NLA_STRING)
+ * @NFTA_EXPR_NAME: name of the expression type (NLA_NUL_STRING)
  * @NFTA_EXPR_DATA: type specific data (NLA_NESTED)
  */
 enum nft_expr_attributes {
@@ -454,7 +454,7 @@ enum nft_cmp_attributes {
 /**
  * enum nft_lookup_attributes - nf_tables set lookup expression netlink attributes
  *
- * @NFTA_LOOKUP_SET: name of the set where to look for (NLA_STRING)
+ * @NFTA_LOOKUP_SET: name of the set where to look for (NLA_NUL_STRING)
  * @NFTA_LOOKUP_SREG: source register of the data to look for (NLA_U32: nft_registers)
  * @NFTA_LOOKUP_DREG: destination register (NLA_U32: nft_registers)
  */
@@ -657,7 +657,7 @@ enum nft_counter_attributes {
  * enum nft_log_attributes - nf_tables log expression netlink attributes
  *
  * @NFTA_LOG_GROUP: netlink group to send messages to (NLA_U32)
- * @NFTA_LOG_PREFIX: prefix to prepend to log messages (NLA_STRING)
+ * @NFTA_LOG_PREFIX: prefix to prepend to log messages (NLA_NUL_STRING)
  * @NFTA_LOG_SNAPLEN: length of payload to include in netlink message (NLA_U32)
  * @NFTA_LOG_QTHRESHOLD: queue threshold (NLA_U32)
  */
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3fd159d..8228622 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -164,7 +164,7 @@ nf_tables_chain_type_lookup(const struct nft_af_info *afi,
 }
 
 static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
-	[NFTA_TABLE_NAME]	= { .type = NLA_STRING },
+	[NFTA_TABLE_NAME]	= { .type = NLA_NUL_STRING },
 	[NFTA_TABLE_FLAGS]	= { .type = NLA_U32 },
 };
 
@@ -535,9 +535,9 @@ static struct nft_chain *nf_tables_chain_lookup(const struct nft_table *table,
 }
 
 static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] = {
-	[NFTA_CHAIN_TABLE]	= { .type = NLA_STRING },
+	[NFTA_CHAIN_TABLE]	= { .type = NLA_NUL_STRING },
 	[NFTA_CHAIN_HANDLE]	= { .type = NLA_U64 },
-	[NFTA_CHAIN_NAME]	= { .type = NLA_STRING,
+	[NFTA_CHAIN_NAME]	= { .type = NLA_NUL_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
 	[NFTA_CHAIN_HOOK]	= { .type = NLA_NESTED },
 	[NFTA_CHAIN_POLICY]	= { .type = NLA_U32 },
@@ -1159,7 +1159,7 @@ static const struct nft_expr_type *nft_expr_type_get(u8 family,
 }
 
 static const struct nla_policy nft_expr_policy[NFTA_EXPR_MAX + 1] = {
-	[NFTA_EXPR_NAME]	= { .type = NLA_STRING },
+	[NFTA_EXPR_NAME]	= { .type = NLA_NUL_STRING },
 	[NFTA_EXPR_DATA]	= { .type = NLA_NESTED },
 };
 
@@ -1289,8 +1289,8 @@ static struct nft_rule *nf_tables_rule_lookup(const struct nft_chain *chain,
 }
 
 static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
-	[NFTA_RULE_TABLE]	= { .type = NLA_STRING },
-	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
+	[NFTA_RULE_TABLE]	= { .type = NLA_NUL_STRING },
+	[NFTA_RULE_CHAIN]	= { .type = NLA_NUL_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
 	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
@@ -1945,8 +1945,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
 }
 
 static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
-	[NFTA_SET_TABLE]		= { .type = NLA_STRING },
-	[NFTA_SET_NAME]			= { .type = NLA_STRING,
+	[NFTA_SET_TABLE]		= { .type = NLA_NUL_STRING },
+	[NFTA_SET_NAME]			= { .type = NLA_NUL_STRING,
 					    .len = IFNAMSIZ - 1 },
 	[NFTA_SET_FLAGS]		= { .type = NLA_U32 },
 	[NFTA_SET_KEY_TYPE]		= { .type = NLA_U32 },
@@ -2549,8 +2549,8 @@ static const struct nla_policy nft_set_elem_policy[NFTA_SET_ELEM_MAX + 1] = {
 };
 
 static const struct nla_policy nft_set_elem_list_policy[NFTA_SET_ELEM_LIST_MAX + 1] = {
-	[NFTA_SET_ELEM_LIST_TABLE]	= { .type = NLA_STRING },
-	[NFTA_SET_ELEM_LIST_SET]	= { .type = NLA_STRING },
+	[NFTA_SET_ELEM_LIST_TABLE]	= { .type = NLA_NUL_STRING },
+	[NFTA_SET_ELEM_LIST_SET]	= { .type = NLA_NUL_STRING },
 	[NFTA_SET_ELEM_LIST_ELEMENTS]	= { .type = NLA_NESTED },
 };
 
@@ -3167,7 +3167,7 @@ EXPORT_SYMBOL_GPL(nft_validate_data_load);
 
 static const struct nla_policy nft_verdict_policy[NFTA_VERDICT_MAX + 1] = {
 	[NFTA_VERDICT_CODE]	= { .type = NLA_U32 },
-	[NFTA_VERDICT_CHAIN]	= { .type = NLA_STRING,
+	[NFTA_VERDICT_CHAIN]	= { .type = NLA_NUL_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
 };
 
diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 10cfb15..047a1b0 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -38,7 +38,7 @@ static void nft_log_eval(const struct nft_expr *expr,
 
 static const struct nla_policy nft_log_policy[NFTA_LOG_MAX + 1] = {
 	[NFTA_LOG_GROUP]	= { .type = NLA_U16 },
-	[NFTA_LOG_PREFIX]	= { .type = NLA_STRING },
+	[NFTA_LOG_PREFIX]	= { .type = NLA_NUL_STRING },
 	[NFTA_LOG_SNAPLEN]	= { .type = NLA_U32 },
 	[NFTA_LOG_QTHRESHOLD]	= { .type = NLA_U16 },
 };
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 7fd2bea..37f09cc 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -38,7 +38,7 @@ static void nft_lookup_eval(const struct nft_expr *expr,
 }
 
 static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {
-	[NFTA_LOOKUP_SET]	= { .type = NLA_STRING },
+	[NFTA_LOOKUP_SET]	= { .type = NLA_NUL_STRING },
 	[NFTA_LOOKUP_SREG]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_DREG]	= { .type = NLA_U32 },
 };
-- 
1.7.10.4

Re: [PATCH 3/3] netfilter: nf_tables: enforce NLA_NUL_STRING in strings

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:

[cc'd Thomas ]

> nla_strcmp compares the string length plus one, so it's implicitly
> including the nul-termination in the comparison.
> 
>  int nla_strcmp(const struct nlattr *nla, const char *str)
>  {
>         int len = strlen(str) + 1;
>         ...
> 		d = memcmp(nla_data(nla), str, len);

> nla_strcmp compares the string length plus one, so it's implicitly
> including the nul-termination in the comparison.
>  int nla_strcmp(const struct nlattr *nla, const char *str)
>  {
>         int len = strlen(str) + 1;
>         ...
> 		d = memcmp(nla_data(nla), str, len);

[..]

> However, if NLA_STRING is used, userspace can send us a string without
> the null-termination. This is a problem since the nf_tables lookup
> functions won't find any matching as the last byte may mismatch.
> So we have to enforce that strings are nul-termination to avoid
> mismatches.

Looks to me as if the real fix is:

int nla_strcmp(const struct nlattr *nla, const char *str)
{
	return nla_memcmp(nla, str, strlen(str));
}

[ better yet, add static inline wrapper for it ].

Re: [PATCH 3/3] netfilter: nf_tables: enforce NLA_NUL_STRING in strings

[Pablo Neira Ayuso]

On Mon, Mar 31, 2014 at 02:15:51PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> [cc'd Thomas ]
> 
> > nla_strcmp compares the string length plus one, so it's implicitly
> > including the nul-termination in the comparison.
> >  int nla_strcmp(const struct nlattr *nla, const char *str)
> >  {
> >         int len = strlen(str) + 1;
> >         ...
> > 		d = memcmp(nla_data(nla), str, len);
> 
> [..]
> 
> > However, if NLA_STRING is used, userspace can send us a string without
> > the null-termination. This is a problem since the nf_tables lookup
> > functions won't find any matching as the last byte may mismatch.
> > So we have to enforce that strings are nul-termination to avoid
> > mismatches.
> 
> Looks to me as if the real fix is:
> 
> int nla_strcmp(const struct nlattr *nla, const char *str)
> {
> 	return nla_memcmp(nla, str, strlen(str));
> }
> 
> [ better yet, add static inline wrapper for it ].

I think you're right, a quick look at the users of this:

net/core/fib_rules.c:               nla_strcmp(tb[FRA_IIFNAME], rule->iifname))
net/core/fib_rules.c:               nla_strcmp(tb[FRA_OIFNAME], rule->oifname))
net/core/neighbour.c:           if (nla_strcmp(tb[NDTA_NAME], tbl->id) == 0)
net/decnet/dn_dev.c:            if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
net/ipv4/devinet.c:             if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, table->name))
net/netfilter/nf_tables_api.c:              !nla_strcmp(nla, chain_type[family][i]->name))
net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, chain->name))
net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, type->name) &&
net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, set->name))
net/sched/act_api.c:                    if (nla_strcmp(kind, a->kind) == 0) {
net/sched/cls_api.c:                    if (nla_strcmp(kind, t->kind) == 0) {
net/sched/cls_api.c:    } else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
net/sched/sch_api.c:                    if (nla_strcmp(kind, q->id) == 0) {
net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
net/sched/sch_api.c:                            if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
net/sched/sch_api.c: nla_strcmp(tca[TCA_KIND], q->ops->id))))
net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id

In the current iproute2 tree: /ip/ipntable.c:

        len = strlen(namep) + 1;
        addattr_l(&req.n, sizeof(req), NDTA_NAME, namep, len)

from ip/ipaddress.c:

addattr_l(&req.n, sizeof(req), IFA_LABEL, l, strlen(l)+1)

They are indeed including the nul-termination, that's why the
comparison is working.

I don't find any validation for TCA_KIND though, but that nla_strcmp
is implicitly enforcing the nul-termination, otherwise will return a
mismatch.

Re: [PATCH 3/3] netfilter: nf_tables: enforce NLA_NUL_STRING in strings

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I think you're right, a quick look at the users of this:
> 
> net/core/fib_rules.c:               nla_strcmp(tb[FRA_IIFNAME], rule->iifname))
> net/core/fib_rules.c:               nla_strcmp(tb[FRA_OIFNAME], rule->oifname))
> net/core/neighbour.c:           if (nla_strcmp(tb[NDTA_NAME], tbl->id) == 0)
> net/decnet/dn_dev.c:            if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
> net/ipv4/devinet.c:             if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
> net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, table->name))
> net/netfilter/nf_tables_api.c:              !nla_strcmp(nla, chain_type[family][i]->name))
> net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, chain->name))
> net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, type->name) &&
> net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, set->name))
> net/sched/act_api.c:                    if (nla_strcmp(kind, a->kind) == 0) {
> net/sched/cls_api.c:                    if (nla_strcmp(kind, t->kind) == 0) {
> net/sched/cls_api.c:    } else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
> net/sched/sch_api.c:                    if (nla_strcmp(kind, q->id) == 0) {
> net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
> net/sched/sch_api.c:                            if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
> net/sched/sch_api.c: nla_strcmp(tca[TCA_KIND], q->ops->id))))
> net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id
> 
> In the current iproute2 tree: /ip/ipntable.c:
> 
>         len = strlen(namep) + 1;
>         addattr_l(&req.n, sizeof(req), NDTA_NAME, namep, len)
> 
> from ip/ipaddress.c:
> 
> addattr_l(&req.n, sizeof(req), IFA_LABEL, l, strlen(l)+1)
> 
> They are indeed including the nul-termination, that's why the
> comparison is working.
> I don't find any validation for TCA_KIND though, but that nla_strcmp
> is implicitly enforcing the nul-termination, otherwise will return a
> mismatch.

You're right, aliasing it to nla_memcmp would break iproute2.

So looks like we'd have to add backwards compat to nla_strcmp and check if the last
byte of nla data is a zero byte to catch this.

Lets see if Thomas has a better idea.

Re: [PATCH 3/3] netfilter: nf_tables: enforce NLA_NUL_STRING in strings

[Thomas Graf]

On 03/31/14 at 03:08pm, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I think you're right, a quick look at the users of this:
> > 
> > net/core/fib_rules.c:               nla_strcmp(tb[FRA_IIFNAME], rule->iifname))
> > net/core/fib_rules.c:               nla_strcmp(tb[FRA_OIFNAME], rule->oifname))
> > net/core/neighbour.c:           if (nla_strcmp(tb[NDTA_NAME], tbl->id) == 0)
> > net/decnet/dn_dev.c:            if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
> > net/ipv4/devinet.c:             if (tb[IFA_LABEL] && nla_strcmp(tb[IFA_LABEL], ifa->ifa_label))
> > net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, table->name))
> > net/netfilter/nf_tables_api.c:              !nla_strcmp(nla, chain_type[family][i]->name))
> > net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, chain->name))
> > net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, type->name) &&
> > net/netfilter/nf_tables_api.c:          if (!nla_strcmp(nla, set->name))
> > net/sched/act_api.c:                    if (nla_strcmp(kind, a->kind) == 0) {
> > net/sched/cls_api.c:                    if (nla_strcmp(kind, t->kind) == 0) {
> > net/sched/cls_api.c:    } else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
> > net/sched/sch_api.c:                    if (nla_strcmp(kind, q->id) == 0) {
> > net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
> > net/sched/sch_api.c:                            if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
> > net/sched/sch_api.c: nla_strcmp(tca[TCA_KIND], q->ops->id))))
> > net/sched/sch_api.c:    if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id
> > 
> > In the current iproute2 tree: /ip/ipntable.c:
> > 
> >         len = strlen(namep) + 1;
> >         addattr_l(&req.n, sizeof(req), NDTA_NAME, namep, len)
> > 
> > from ip/ipaddress.c:
> > 
> > addattr_l(&req.n, sizeof(req), IFA_LABEL, l, strlen(l)+1)
> > 
> > They are indeed including the nul-termination, that's why the
> > comparison is working.
> > I don't find any validation for TCA_KIND though, but that nla_strcmp
> > is implicitly enforcing the nul-termination, otherwise will return a
> > mismatch.
> 
> You're right, aliasing it to nla_memcmp would break iproute2.
> 
> So looks like we'd have to add backwards compat to nla_strcmp and check if the last
> byte of nla data is a zero byte to catch this.
> 
> Lets see if Thomas has a better idea.

Seems safe to just fix nla_strcmp() to disregard the terminating
NUL in the attribute data if it is present, just like validate_nla()
does for NLA_STRING.