| Message ID | 20190405212414.24184-1-johannes@sipsolutions.net |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] netlink: limit recursion depth in policy validation | expand |
On Fri, Apr 05, 2019 at 11:24:14PM +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Now that we have nested policies, we can theoretically > recurse forever parsing attributes if a (sub-)policy > refers back to a higher level one. This is a situation > that has happened in nl80211, and we've avoided it there > by not linking it. > > Add some code to netlink parsing to limit recursion depth, > allowing us to safely change nl80211 to actually link the > nested policy, which in turn allows some code cleanups. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > lib/nlattr.c | 46 +++++++++++++++++++++++++++++++----------- > net/wireless/nl80211.c | 10 ++++----- > net/wireless/nl80211.h | 2 -- > net/wireless/pmsr.c | 3 +-- > 4 files changed, 39 insertions(+), 22 deletions(-) > > diff --git a/lib/nlattr.c b/lib/nlattr.c > index baf27844ecc8..bc41d3d96945 100644 > --- a/lib/nlattr.c > +++ b/lib/nlattr.c > @@ -44,6 +44,20 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = { > [NLA_S64] = sizeof(s64), > }; > > +/* > + * Nested policies might refer back to the original > + * policy in some cases, and userspace could try to > + * abuse that and recurse by nesting in the right > + * ways. Limit recursion to avoid this problem. > + */ > +#define MAX_POLICY_RECURSION_DEPTH 10 In your policy description approach, you iterate over the policy structures. How do you deal with this recursions from there?
On Fri, 2019-04-26 at 18:57 +0200, Pablo Neira Ayuso wrote: > > > +/* > > + * Nested policies might refer back to the original > > + * policy in some cases, and userspace could try to > > + * abuse that and recurse by nesting in the right > > + * ways. Limit recursion to avoid this problem. > > + */ > > +#define MAX_POLICY_RECURSION_DEPTH 10 > > In your policy description approach, you iterate over the policy > structures. How do you deal with this recursions from there? Well, check out the code :-) It doesn't actually recurse. What it does is build a list of policies that are reachable from the root policy and each policy in the list. So basically, there we do: list = [root policy] list_len = 1 i = 0 walk_policy(policy) { for_each_policy_entry(entry, policy) { nested = nested_policy_or_null(entry); if (nested) { list[i] = nested; list_len += 1 } } } while (i < list_len) { walk_policy(list[i]); i++; } Then, we walk the list again: for (i = 0; i < list_len; i++) { for_each_policy_entry(entry, list[i]) { send_entry_to_userspace(i, entry); // mark it as occurring in policy i } } This basically flattens the whole thing. Obviously, the walking may allocate some memory, and the last loop to send it out isn't actually a loop like that because it's a netlink dump with each entry being in a separate netlink message, but that's the gist of it. johannes
On Fri, Apr 26, 2019 at 07:03:10PM +0200, Johannes Berg wrote: > On Fri, 2019-04-26 at 18:57 +0200, Pablo Neira Ayuso wrote: > > > > > +/* > > > + * Nested policies might refer back to the original > > > + * policy in some cases, and userspace could try to > > > + * abuse that and recurse by nesting in the right > > > + * ways. Limit recursion to avoid this problem. > > > + */ > > > +#define MAX_POLICY_RECURSION_DEPTH 10 > > > > In your policy description approach, you iterate over the policy > > structures. How do you deal with this recursions from there? > > Well, check out the code :-) > > It doesn't actually recurse. What it does is build a list of policies > that are reachable from the root policy and each policy in the list. So > basically, there we do: > > list = [root policy] > list_len = 1 > i = 0 > > walk_policy(policy) > { > for_each_policy_entry(entry, policy) { > nested = nested_policy_or_null(entry); > if (nested) { > list[i] = nested; > list_len += 1 > } > } > } > > while (i < list_len) { > walk_policy(list[i]); > i++; > } > > Then, we walk the list again: > > for (i = 0; i < list_len; i++) { > for_each_policy_entry(entry, list[i]) { > send_entry_to_userspace(i, entry); // mark it as occurring in policy i > } > } > > > This basically flattens the whole thing. > > Obviously, the walking may allocate some memory, and the last loop to > send it out isn't actually a loop like that because it's a netlink dump > with each entry being in a separate netlink message, but that's the gist > of it. I see, following this approach, I can just remove the duplicated code in my netlink description stuff by using the list of policy structures.
On Fri, 2019-04-26 at 19:06 +0200, Pablo Neira Ayuso wrote: > > > This basically flattens the whole thing. > > > > Obviously, the walking may allocate some memory, and the last loop to > > send it out isn't actually a loop like that because it's a netlink dump > > with each entry being in a separate netlink message, but that's the gist > > of it. > > I see, following this approach, I can just remove the duplicated code > in my netlink description stuff by using the list of policy > structures. I wrote the code in a way that you can reuse it easily, check out the patch :-) Generic netlink is one user added by the patch, but the actual exposing code is with general attributes and general code that you can easily call. Meanwhile, I'm still writing a response to your other email, give me a few minutes. johannes
diff --git a/lib/nlattr.c b/lib/nlattr.c index baf27844ecc8..bc41d3d96945 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -44,6 +44,20 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = { [NLA_S64] = sizeof(s64), }; +/* + * Nested policies might refer back to the original + * policy in some cases, and userspace could try to + * abuse that and recurse by nesting in the right + * ways. Limit recursion to avoid this problem. + */ +#define MAX_POLICY_RECURSION_DEPTH 10 + +static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype, + const struct nla_policy *policy, + unsigned int validate, + struct netlink_ext_ack *extack, + struct nlattr **tb, unsigned int depth); + static int validate_nla_bitfield32(const struct nlattr *nla, const u32 valid_flags_mask) { @@ -70,7 +84,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla, static int nla_validate_array(const struct nlattr *head, int len, int maxtype, const struct nla_policy *policy, struct netlink_ext_ack *extack, - unsigned int validate) + unsigned int validate, unsigned int depth) { const struct nlattr *entry; int rem; @@ -87,8 +101,9 @@ static int nla_validate_array(const struct nlattr *head, int len, int maxtype, return -ERANGE; } - ret = __nla_validate(nla_data(entry), nla_len(entry), - maxtype, policy, validate, extack); + ret = __nla_validate_parse(nla_data(entry), nla_len(entry), + maxtype, policy, validate, extack, + NULL, depth + 1); if (ret < 0) return ret; } @@ -280,7 +295,7 @@ static int nla_validate_int_range(const struct nla_policy *pt, static int validate_nla(const struct nlattr *nla, int maxtype, const struct nla_policy *policy, unsigned int validate, - struct netlink_ext_ack *extack) + struct netlink_ext_ack *extack, unsigned int depth) { u16 strict_start_type = policy[0].strict_start_type; const struct nla_policy *pt; @@ -375,9 +390,10 @@ static int validate_nla(const struct nlattr *nla, int maxtype, if (attrlen < NLA_HDRLEN) goto out_err; if (pt->nested_policy) { - err = __nla_validate(nla_data(nla), nla_len(nla), pt->len, - pt->nested_policy, validate, - extack); + err = __nla_validate_parse(nla_data(nla), nla_len(nla), + pt->len, pt->nested_policy, + validate, extack, NULL, + depth + 1); if (err < 0) { /* * return directly to preserve the inner @@ -400,7 +416,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype, err = nla_validate_array(nla_data(nla), nla_len(nla), pt->len, pt->nested_policy, - extack, validate); + extack, validate, depth); if (err < 0) { /* * return directly to preserve the inner @@ -472,11 +488,17 @@ static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype, const struct nla_policy *policy, unsigned int validate, struct netlink_ext_ack *extack, - struct nlattr **tb) + struct nlattr **tb, unsigned int depth) { const struct nlattr *nla; int rem; + if (depth >= MAX_POLICY_RECURSION_DEPTH) { + NL_SET_ERR_MSG(extack, + "allowed policy recursion depth exceeded"); + return -EINVAL; + } + if (WARN_ON(validate & NL_VALIDATE_POLICY && !policy)) return -EINVAL; @@ -495,7 +517,7 @@ static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype, } if (policy) { int err = validate_nla(nla, maxtype, policy, - validate, extack); + validate, extack, depth); if (err < 0) return err; @@ -537,7 +559,7 @@ int __nla_validate(const struct nlattr *head, int len, int maxtype, struct netlink_ext_ack *extack) { return __nla_validate_parse(head, len, maxtype, policy, validate, - extack, NULL); + extack, NULL, 0); } EXPORT_SYMBOL(__nla_validate); @@ -592,7 +614,7 @@ int __nla_parse(struct nlattr **tb, int maxtype, struct netlink_ext_ack *extack) { return __nla_validate_parse(head, len, maxtype, policy, validate, - extack, tb); + extack, tb, 0); } EXPORT_SYMBOL(__nla_parse); diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index e5c0e693a1b2..6b241754b290 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -219,6 +219,8 @@ static int validate_ie_attr(const struct nlattr *attr, } /* policy for the attributes */ +static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR]; + static const struct nla_policy nl80211_ftm_responder_policy[NL80211_FTM_RESP_ATTR_MAX + 1] = { [0] = { .strict_start_type = NL80211_FTM_RESP_ATTR_CIVICLOC + 1 }, @@ -268,11 +270,7 @@ static const struct nla_policy nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = { [0] = { .strict_start_type = NL80211_PMSR_PEER_ATTR_RESP + 1 }, [NL80211_PMSR_PEER_ATTR_ADDR] = NLA_POLICY_ETH_ADDR, - /* - * we could specify this again to be the top-level policy, - * but that would open us up to recursion problems ... - */ - [NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_NESTED }, + [NL80211_PMSR_PEER_ATTR_CHAN] = NLA_POLICY_NESTED(nl80211_policy), [NL80211_PMSR_PEER_ATTR_REQ] = NLA_POLICY_NESTED(nl80211_pmsr_req_attr_policy), [NL80211_PMSR_PEER_ATTR_RESP] = { .type = NLA_REJECT }, @@ -289,7 +287,7 @@ nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = { NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy), }; -const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { +static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { [0] = { .strict_start_type = NL80211_ATTR_AIRTIME_WEIGHT + 1 }, [NL80211_ATTR_WIPHY] = { .type = NLA_U32 }, [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING, diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h index a41e94a49a89..d3e8e426c486 100644 --- a/net/wireless/nl80211.h +++ b/net/wireless/nl80211.h @@ -11,8 +11,6 @@ int nl80211_init(void); void nl80211_exit(void); -extern const struct nla_policy nl80211_policy[NUM_NL80211_ATTR]; - void *nl80211hdr_put(struct sk_buff *skb, u32 portid, u32 seq, int flags, u8 cmd); bool nl80211_put_sta_rate(struct sk_buff *msg, struct rate_info *info, diff --git a/net/wireless/pmsr.c b/net/wireless/pmsr.c index c2138fd97c42..6bed7b59ab15 100644 --- a/net/wireless/pmsr.c +++ b/net/wireless/pmsr.c @@ -155,10 +155,9 @@ static int pmsr_parse_peer(struct cfg80211_registered_device *rdev, /* reuse info->attrs */ memset(info->attrs, 0, sizeof(*info->attrs) * (NL80211_ATTR_MAX + 1)); - /* need to validate here, we don't want to have validation recursion */ err = nla_parse_nested_deprecated(info->attrs, NL80211_ATTR_MAX, tb[NL80211_PMSR_PEER_ATTR_CHAN], - nl80211_policy, info->extack); + NULL, info->extack); if (err) return err;