| Message ID | 1396266691-3538-3-git-send-email-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Headers | show |
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 ]. -- 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
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. -- 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
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. -- 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
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. -- 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
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 }, };
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(-)