nla_put_string() vs NLA_STRING

nla_put_string() vs NLA_STRING

[Kees Cook]

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

Re: nla_put_string() vs NLA_STRING

[David Miller]

From: Kees Cook <keescook@google.com>
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.

Re: nla_put_string() vs NLA_STRING

[Johannes Berg]

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