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

Powered by blists - more mailing lists