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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bab4016fe3138b2340efc5c0f2fbb5b78dd6414f.camel@sipsolutions.net>
Date:   Sun, 28 Apr 2019 21:38:00 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] netlink: limit recursion depth in policy validation

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