netfilter: nf_tables: set names cannot be larger than 15 bytes

PATCH In review Pablo Neira Ayuso 2 participants 6 messages Mar 24, 2014 – Mar 28, 2014

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2014-03-24 14:10 UTC #0 lore raw

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

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 <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_tables_api.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/netfilter/nf_tables_api.c +2 -1

index 33045a5..43ae487 100644
@@ -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 },

Signature · 1 line

1.7.10.4

Florian Westphal

maintainer

2014-03-24 22:41 UTC #1 lore raw

net/netfilter/nf_tables_api.c static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const · line 1950

Quoted 11 lines · Pablo Neira Ayuso

net/netfilter/nf_tables_api.c +2 -1

index 33045a5..43ae487 100644
@@ -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 },

Not related to your patch, but it looks to me as if all of these should be NLA_NUL_STRING, no?

Pablo Neira Ayuso

author maintainer

2014-03-25 20:45 UTC #2 lore raw

Quoted 7 lines · Florian Westphal

Quoted 6 lines · Pablo Neira Ayuso
net/netfilter/nf_tables_api.c +0 -0
index 33045a5..43ae487 100644
@@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
 

Quoted 8 lines

net/netfilter/nf_tables_api.c static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const · line 1950
Quoted 5 lines
net/netfilter/nf_tables_api.c +2 -1
 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 },
Not related to your patch, but it looks to me as if all of these should be NLA_NUL_STRING, no?

This is what I originally thought. But all of the nla_* functions use the nla_len to know length of the string coming from userspace, so they don't rely on the trailing nul-termination.

Anyway, I'd appreciate if you can do a second look at this to make sure I'm not overlooking anything. Thanks.

Florian Westphal

maintainer

2014-03-26 00:32 UTC #3 lore raw

Quoted 17 lines · Pablo Neira Ayuso

Quoted 7 lines · Florian Westphal
Quoted 6 lines · Pablo Neira Ayuso
net/netfilter/nf_tables_api.c +0 -0
index 33045a5..43ae487 100644
@@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
 
Quoted 8 lines
net/netfilter/nf_tables_api.c static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const · line 1950
Quoted 5 lines
net/netfilter/nf_tables_api.c +2 -1
 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 },
Not related to your patch, but it looks to me as if all of these should be NLA_NUL_STRING, no?

Quoted 3 lines

This is what I originally thought. But all of the nla_* functions use the nla_len to know length of the string coming from userspace, so they don't rely on the trailing nul-termination.

Indeed. I reviewed this again but only 'bug' I spotted:

nf_tables_chain_type_lookup() does:
request_module("nft-chain-%u-%*.s", ....

('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.

Pablo Neira Ayuso

author maintainer

2014-03-28 11:21 UTC #4 lore raw

Quoted 22 lines · Florian Westphal

Quoted 17 lines · Pablo Neira Ayuso
Quoted 7 lines · Florian Westphal
Quoted 6 lines · Pablo Neira Ayuso
net/netfilter/nf_tables_api.c +0 -0
index 33045a5..43ae487 100644
@@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
 
Quoted 8 lines
net/netfilter/nf_tables_api.c static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const · line 1950
Quoted 5 lines
net/netfilter/nf_tables_api.c +2 -1
 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 },
Not related to your patch, but it looks to me as if all of these should be NLA_NUL_STRING, no?
Quoted 3 lines
This is what I originally thought. But all of the nla_* functions use the nla_len to know length of the string coming from userspace, so they don't rely on the trailing nul-termination.

Quoted 7 lines

Indeed. I reviewed this again but only 'bug' I spotted:
nf_tables_chain_type_lookup() does:
request_module("nft-chain-%u-%*.s", ....
('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.

I think we can change that to NLA_STRING to make it consistent with other attributes. libnftnl is anyway sending the nul-termination all the time, so nothing should break IMO.

Florian Westphal

maintainer

2014-03-28 13:18 UTC #5 lore raw

Quoted 9 lines · Pablo Neira Ayuso

Quoted 9 lines
Quoted 3 lines
This is what I originally thought. But all of the nla_* functions use the nla_len to know length of the string coming from userspace, so they don't rely on the trailing nul-termination.
Indeed. I reviewed this again but only 'bug' I spotted:
nf_tables_chain_type_lookup() does:
request_module("nft-chain-%u-%*.s", ....

Quoted 6 lines

Quoted 2 lines
('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.
I think we can change that to NLA_STRING to make it consistent with other attributes. libnftnl is anyway sending the nul-termination all the time, so nothing should break IMO.

Right, but the kernel shouldn't rely on that :)

I see no problem converting it to NLA_STRING (if request_module args are fixed).