Re: nla_put_string() vs NLA_STRING
On Tue, 2018-02-20 at 22:00 -0800, Kees Cook wrote: > It seems that in at least one case[1], nla_put_string() is being used > on an NLA_STRING, which lacks a NULL terminator, which leads to > silliness when nla_put_string() uses strlen() to figure out the size: Fun! I'm not a big fan of the whole NLA_STRING thing with or without NUL terminator anyway, it's a bit confusing at times :-) > This is a problem at least here: > > struct regulatory_request { > ... > char alpha2[2]; > ... > > static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { > ... > [NL80211_ATTR_REG_ALPHA2] = { .type = NLA_STRING, .len = 2 }, > ... Yeah, this is clearly stupid. We already fixed one of these, see commit a5fe8e7695dc ("regulatory: add NUL to alpha2"). I'll fix up the second one too. > So, this specific problem needs fixing (in at least two places calling > nla_put_string(msg, NL80211_ATTR_REG_ALPHA2, ...)). While I suspect > it's only ever written an extra byte from the following variable in > the structure which is an enum nl80211_dfs_regions, Only one, since the other has alpha2[3] already :-) And in that case, yes, on little endian and only if the dfs region is non-zero, though the dfs region was added later so dunno what else there was - but certainly this struct would have always contained some enum value that had zero-bytes. > I worry there > might be a lot more of these (though I'd hope unterminated strings are > uncommon for internal representation). Generally they are, I'd argue. > And more generally, it seems > like only the NLA _input_ functions actually check nla_policy details. > It seems that the output functions should do the same too, yes? It doesn't really work that way - there's no real guarantee that the policy is symmetric on input/output. johannes
Re: nla_put_string() vs NLA_STRING
From: Kees Cook Date: Tue, 20 Feb 2018 22:00:26 -0800 > So, this specific problem needs fixing (in at least two places calling > nla_put_string(msg, NL80211_ATTR_REG_ALPHA2, ...)). While I suspect > it's only ever written an extra byte from the following variable in > the structure which is an enum nl80211_dfs_regions, I worry there > might be a lot more of these (though I'd hope unterminated strings are > uncommon for internal representation). And more generally, it seems > like only the NLA _input_ functions actually check nla_policy details. > It seems that the output functions should do the same too, yes? Generally speaking, the policy is for making sure the user doesn't give us garbage. When building netlink attributes itself, the kernel is supposed to know what it is doing.
nla_put_string() vs NLA_STRING
Hi, It seems that in at least one case[1], nla_put_string() is being used on an NLA_STRING, which lacks a NULL terminator, which leads to silliness when nla_put_string() uses strlen() to figure out the size: /** * nla_put_string - Add a string netlink attribute to a socket buffer * @skb: socket buffer to add attribute to * @attrtype: attribute type * @str: NUL terminated string */ static inline int nla_put_string(struct sk_buff *skb, int attrtype, const char *str) { return nla_put(skb, attrtype, strlen(str) + 1, str); } This is a problem at least here: struct regulatory_request { ... char alpha2[2]; ... static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { ... [NL80211_ATTR_REG_ALPHA2] = { .type = NLA_STRING, .len = 2 }, ... AIUI, working with NLA_STRING needs nla_strlcpy() to "extract" them, and that takes the nla_policy size normally to bounds-check the copy. So, this specific problem needs fixing (in at least two places calling nla_put_string(msg, NL80211_ATTR_REG_ALPHA2, ...)). While I suspect it's only ever written an extra byte from the following variable in the structure which is an enum nl80211_dfs_regions, I worry there might be a lot more of these (though I'd hope unterminated strings are uncommon for internal representation). And more generally, it seems like only the NLA _input_ functions actually check nla_policy details. It seems that the output functions should do the same too, yes? -Kees [1] https://github.com/copperhead/linux-hardened/issues/72 -- Kees Cook Pixel Security