[PATCH] netlink: limit recursion depth in policy validation

[PATCH] netlink: limit recursion depth in policy validation

[Johannes Berg]

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 3db7a6984cb0..ef06645de56c 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 (tb)
 		memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
@@ -492,7 +514,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;
@@ -534,7 +556,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);
 
@@ -589,7 +611,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 4fc7c122e916..09a17b30ba73 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;
 
-- 
2.17.2

Re: [PATCH] netlink: limit recursion depth in policy validation

[Pablo Neira Ayuso]

On Fri, Apr 26, 2019 at 02:13:46PM +0200, Johannes Berg wrote:
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 4fc7c122e916..09a17b30ba73 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),

I guess you only allow one more nested instance of this attribute?

I mean, how many times is NL80211 allow to recurse on this?

Probably you can define a new nl80211_policy_recurse object and set a
flag somewhere to describe that no more recursion are permitted?

I would try to handle this from nl80211, instead of from the core by
limiting recursions to 10.

Once we expose the descriptions to userspace, I would expect we'll end
with tools to validate all kind of stuff like this, eg. fuzzy tested,
check for recursions like this (which IMO they should not be allowed).

Re: [PATCH] netlink: limit recursion depth in policy validation

[Johannes Berg]

Hi Pablo,


> > +	[NL80211_PMSR_PEER_ATTR_CHAN] = NLA_POLICY_NESTED(nl80211_policy),
> 
> I guess you only allow one more nested instance of this attribute?
> 
> I mean, how many times is NL80211 allow to recurse on this?

It doesn't actually recurse on this at all. We really should've
specified a channel originally as a nested attribute, and have had:

 * NL80211_ATTR_CHAN
   * NL80211_CHAN_ATTR_*

and then we could've reused NL80211_CHAN_ATTR_* here as
 * NL80211_ATTR_SOMETHING
   ...
     * NL80211_PMSR_PEER_ATTR_CHAN
       * NL80211_CHAN_ATTR_*

However, as we didn't, we have a few

 NL80211_ATTR_CHAN_*

attributes (at least conceptually) and allow these here inside this
deeply nested object so that we don't have to rewrite the channel
parsing and writing code in both kernel and userspace now.

> Probably you can define a new nl80211_policy_recurse object and set a
> flag somewhere to describe that no more recursion are permitted?

We really should even define which attributes are permitted in the
nesting to start with, and then we wouldn't even get into a recursion
situation, at least not in this particular case.

> I would try to handle this from nl80211, instead of from the core by
> limiting recursions to 10.

Nevertheless, I think (arbitrarily) limiting recursion is necessary,
because we don't want people to even accidentally build a policy that
links to sub a sub-policy and then somehow ends up linking back up to a
policy that's further up in the chain, and then this never really gets
thought about until somebody abuses it for a stack clash attack.

After all, I expect in most of these cases - if they happen - that it'd
be similar to the nl80211 example above, where semantically the
recursion makes no sense.

> Once we expose the descriptions to userspace, I would expect we'll end
> with tools to validate all kind of stuff like this, eg. fuzzy tested,
> check for recursions like this (which IMO they should not be allowed).

Yeah, but I'd rather have the fuzzers run into a reject than actually
have a stack clash bug here that we then find and fix, but leave as a
vulnerability until we do.

johannes

Re: [PATCH] netlink: limit recursion depth in policy validation

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 26 Apr 2019 14:13:46 +0200

> 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>

This doesn't apply cleanly to 'net', is there some dependency I am
unaware of or is this because of a recent mac80211 pull into my tree?

Thanks.

Re: [PATCH] netlink: limit recursion depth in policy validation

[Johannes Berg]

On Mon, 2019-04-29 at 23:08 -0400, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 26 Apr 2019 14:13:46 +0200
> 
> > 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>
> 
> This doesn't apply cleanly to 'net', is there some dependency I am
> unaware of or is this because of a recent mac80211 pull into my tree?

Sorry, I should've made it clear that this applies on top of the
patchset to expose netlink policies to userspace; due to all the
overlaping changes in lib/nlattr.c that seemed like the best solution to
me.

There's no real need to have this safeguard right now in net as nothing
there actually specifies a recursive policy (I knew about this issue and
explicitly made nl80211 *not* have a recursive policy as you can see in
this patch changing that), so I figured net-next was fine.

I'll rebase this on net-next along with the policy export (fixing the
full signed range thing) and resend as a combined set to clarify the
dependencies.

If you prefer to have the safeguard in net even if it shouldn't be
needed now, let me know and I'll make a version that applies there, but
note that will invariably cause conflicts with all the other changes in
lib/nlattr.c.

johannes

Re: [PATCH] netlink: limit recursion depth in policy validation

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 30 Apr 2019 08:58:10 +0200

> If you prefer to have the safeguard in net even if it shouldn't be
> needed now, let me know and I'll make a version that applies there, but
> note that will invariably cause conflicts with all the other changes in
> lib/nlattr.c.

No, that's not necessary.

Thanks for asking.