[PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

Currently, if the user add a stateful object with the name size exceed
NFT_OBJ_MAXNAMELEN - 1 (i.e. 31), we truncate it down to 31 silently.
This is not friendly, furthermore, this will cause duplicated stateful
objects when the first 31 characters of the name is same. So limit the
stateful object's name size to NFT_OBJ_MAXNAMELEN - 1.

After apply this patch, error message will be printed out like this:
  # name_32=$(printf "%0.sQ" {1..32})
  # nft add counter filter $name_32
  <cmdline>:1:1-52: Error: Could not process rule: Numerical result out
  of range
  add counter filter QQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQ
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Fixes: e50092404c1b ("netfilter: nf_tables: add stateful objects")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 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 091d2dc..9112ca9 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4033,7 +4033,8 @@ EXPORT_SYMBOL_GPL(nf_tables_obj_lookup);
 
 static const struct nla_policy nft_obj_policy[NFTA_OBJ_MAX + 1] = {
 	[NFTA_OBJ_TABLE]	= { .type = NLA_STRING },
-	[NFTA_OBJ_NAME]		= { .type = NLA_STRING },
+	[NFTA_OBJ_NAME]		= { .type = NLA_STRING,
+				    .len = NFT_OBJ_MAXNAMELEN - 1 },
 	[NFTA_OBJ_TYPE]		= { .type = NLA_U32 },
 	[NFTA_OBJ_DATA]		= { .type = NLA_NESTED },
 };
-- 
2.5.5

Re: [PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Pablo Neira Ayuso]

On Thu, Jan 19, 2017 at 10:00:20PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> Currently, if the user add a stateful object with the name size exceed
> NFT_OBJ_MAXNAMELEN - 1 (i.e. 31), we truncate it down to 31 silently.
> This is not friendly, furthermore, this will cause duplicated stateful
> objects when the first 31 characters of the name is same. So limit the
> stateful object's name size to NFT_OBJ_MAXNAMELEN - 1.
> 
> After apply this patch, error message will be printed out like this:
>   # name_32=$(printf "%0.sQ" {1..32})
>   # nft add counter filter $name_32
>   <cmdline>:1:1-52: Error: Could not process rule: Numerical result out
>   of range
>   add counter filter QQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQ
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Good catch.

At quick glance, I can see other spots lacking this validation:

static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] =
{
        [NFTA_CHAIN_TABLE]      = { .type = NLA_STRING },

Probably review and fix them in one go?

Re: [PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Liping Zhang]

2017-01-19 22:09 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Thu, Jan 19, 2017 at 10:00:20PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> Currently, if the user add a stateful object with the name size exceed
>> NFT_OBJ_MAXNAMELEN - 1 (i.e. 31), we truncate it down to 31 silently.
>> This is not friendly, furthermore, this will cause duplicated stateful
>> objects when the first 31 characters of the name is same. So limit the
>> stateful object's name size to NFT_OBJ_MAXNAMELEN - 1.
>>
>> After apply this patch, error message will be printed out like this:
>>   # name_32=$(printf "%0.sQ" {1..32})
>>   # nft add counter filter $name_32
>>   <cmdline>:1:1-52: Error: Could not process rule: Numerical result out
>>   of range
>>   add counter filter QQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQ
>>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Good catch.
>
> At quick glance, I can see other spots lacking this validation:
>
> static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] =
> {
>         [NFTA_CHAIN_TABLE]      = { .type = NLA_STRING },
>
> Probably review and fix them in one go?

The nft table name's size is limited at this place:
static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
        [NFTA_TABLE_NAME] = { .type = NLA_STRING,
                                                   .len =
NFT_TABLE_MAXNAMELEN - 1 },

If NFTA_CHAIN_TABLE's size exceeded 31, nf_tables_table_lookup will
fail eventually.

So I think adding the validation of NFTA_CHAIN_TABLE's size is not
important.

I had checked the table, chain, rule(no name), set, setelem(no name)
and object, I only found the validation of the object's name was missed.

Re: [PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Florian Westphal]

Liping Zhang <zlpnobody@gmail.com> wrote:
> > At quick glance, I can see other spots lacking this validation:
> >
> > static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] =
> > {
> >         [NFTA_CHAIN_TABLE]      = { .type = NLA_STRING },
> >
> > Probably review and fix them in one go?
> 
> The nft table name's size is limited at this place:
> static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
>         [NFTA_TABLE_NAME] = { .type = NLA_STRING,
>                                                    .len =
> NFT_TABLE_MAXNAMELEN - 1 },
> 
> If NFTA_CHAIN_TABLE's size exceeded 31, nf_tables_table_lookup will
> fail eventually.
> 
> So I think adding the validation of NFTA_CHAIN_TABLE's size is not
> important.

Perhaps but its better to have it anyway so that you don't need this
extra context to understand that its limited in practice.

Re: [PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Patrick PIGNOL]

Hi all,

I KNOW that I will say something stupid but I'm thinking 'Why don't set NFT_OBJ_MAXNAMELEN to 255(or maybe size_t|SIZE_MAX|) on x86 ?'.

The true question that I can't answer now is : 'Why it is so stupid ?'

Best regards,

Patrick


Le 19/01/2017 à 15:41, Liping Zhang a écrit :
> 2017-01-19 22:09 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
>> On Thu, Jan 19, 2017 at 10:00:20PM +0800, Liping Zhang wrote:
>>> From: Liping Zhang <zlpnobody@gmail.com>
>>>
>>> Currently, if the user add a stateful object with the name size exceed
>>> NFT_OBJ_MAXNAMELEN - 1 (i.e. 31), we truncate it down to 31 silently.
>>> This is not friendly, furthermore, this will cause duplicated stateful
>>> objects when the first 31 characters of the name is same. So limit the
>>> stateful object's name size to NFT_OBJ_MAXNAMELEN - 1.
>>>
>>> After apply this patch, error message will be printed out like this:
>>>    # name_32=$(printf "%0.sQ" {1..32})
>>>    # nft add counter filter $name_32
>>>    <cmdline>:1:1-52: Error: Could not process rule: Numerical result out
>>>    of range
>>>    add counter filter QQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQQ
>>>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Good catch.
>>
>> At quick glance, I can see other spots lacking this validation:
>>
>> static const struct nla_policy nft_chain_policy[NFTA_CHAIN_MAX + 1] =
>> {
>>          [NFTA_CHAIN_TABLE]      = { .type = NLA_STRING },
>>
>> Probably review and fix them in one go?
> The nft table name's size is limited at this place:
> static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
>          [NFTA_TABLE_NAME] = { .type = NLA_STRING,
>                                                     .len =
> NFT_TABLE_MAXNAMELEN - 1 },
>
> If NFTA_CHAIN_TABLE's size exceeded 31, nf_tables_table_lookup will
> fail eventually.
>
> So I think adding the validation of NFTA_CHAIN_TABLE's size is not
> important.
>
> I had checked the table, chain, rule(no name), set, setelem(no name)
> and object, I only found the validation of the object's name was missed.
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: nf_tables: report error if stateful obj's name is truncated

[Patrick PIGNOL]

Le 19/01/2017 à 15:41, Liping Zhang a écrit :
> The nft table name's size is limited at this place:
> static const struct nla_policy nft_table_policy[NFTA_TABLE_MAX + 1] = {
>          [NFTA_TABLE_NAME] = { .type = NLA_STRING,
>                                                     .len =
> NFT_TABLE_MAXNAMELEN - 1 },
>
> If NFTA_CHAIN_TABLE's size exceeded 31, nf_tables_table_lookup will
> fail eventually.
>
Why ? :


typedef unsigned short __u16; // minimum MaxValue = 65535

  /*
*  <------- NLA_HDRLEN ------> <-- NLA_ALIGN(payload)-->
* +---------------------+- - -+- - - - - - - - - -+- - -+
* |        Header       | Pad |     Payload     | Pad |
* |   (struct nlattr)   | ing |                        | ing |
* +---------------------+- - -+- - - - - - - - - -+- - -+
*  <-------------- nlattr->nla_len --------------> // Payload minimum 
MaxValue = 65535 - (at least)32 = 65503
*/

struct nlattr {
         __u16           nla_len;
         __u16           nla_type;
};

#define NFT_SET_MAXNAMELEN      32  // <- Why 32 ? Why not 65535 or 65503