netdev - Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

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]

Date:   Wed, 19 Sep 2018 09:44:37 -0700
From:   David Ahern <dsahern@...il.com>
To:     Johannes Berg <johannes@...solutions.net>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 5/7] netlink: prepare validate extack setting for
 recursion

On 9/19/18 9:36 AM, Johannes Berg wrote:
> On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
>> On 9/19/18 5:08 AM, Johannes Berg wrote:
>>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>>> index 966cd3dcf31b..2b015e43b725 100644
>>> --- a/lib/nlattr.c
>>> +++ b/lib/nlattr.c
>>> @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
>>>  
>>>  static int validate_nla(const struct nlattr *nla, int maxtype,
>>>  			const struct nla_policy *policy,
>>> -			const char **error_msg)
>>> +			struct netlink_ext_ack *extack, bool *extack_set)
>>
>> extack_set arg is not needed if you handle the "Attribute failed policy
>> validation" message and NL_SET_BAD_ATTR here as well.
> 
> I'm not sure that's true, but perhaps you have a better idea than me?
> 
> My thought would be to introduce an "error" label in validate_nla(),
> that sets up the extack data.
> 
> Then we could skip over that if we have a separate message to report,
> making the NLA_REJECT case easier.
> 
> However, if we do nested validation, I'm not sure it really is that much
> easier? We still need to figure out if the nested validation was setting
> the message (and bad attr), rather than it having been set before we
> even get into this function.
> 
> So let's say we have
> 
>         case NLA_NESTED:
>                 /* a nested attributes is allowed to be empty; if its not,
>                  * it must have a size of at least NLA_HDRLEN.
>                  */
>                 if (attrlen == 0)
>                         break;
>                 if (attrlen < NLA_HDRLEN)
>                         return -ERANGE;
>                 if (pt->validation_data) {
>                         int err;
> 
>                         err = nla_validate_parse(nla_data(nla), nla_len(nla),
>                                                  pt->len, pt->validation_data,
>                                                  extack, extack_set, NULL);
>                         if (err < 0)
>                                 return err;
>                 }
>                 break;
> 
> right now after all the patches.
> 
> The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
> I'm not really sure we can cleanly handle the other case?
> 
> Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
> return points that can fail outside of validate_nla(), or we set up the
> extack data there as well, so that once we have a nested
> nla_validate_parse() we know that it's been set.
> 
> Actually, we need to do that anyway so that we can move the setting into
> validate_nla(), and then it should work.
> 
> Mechanics aside - I'll take a look later tonight or tomorrow - do you
> think the goal/external interface of this makes sense?

If it fails and returns (nested and all) on the first failure it should
be fine. I was thinking something like this (whitespace damaged on paste):

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e335bcafa9e4..f18f0ed3f1cd 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -73,6 +73,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
 {
        const struct nla_policy *pt;
        int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+       int err = -ERANGE;

        if (type <= 0 || type > maxtype)
                return 0;
@@ -89,7 +90,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
        switch (pt->type) {
        case NLA_FLAG:
                if (attrlen > 0)
-                       return -ERANGE;
+                       goto out_err;
                break;

        case NLA_BITFIELD32:

...
(similar for other error places. the one EINVAL needs to set err first)
...

@@ -156,6 +157,10 @@ static int validate_nla(const struct nlattr *nla,
int maxtype,
        }

        return 0;
+out_err:
+       NL_SET_ERR_MSG_ATTR(extack, nla,
+                           "Attribute failed policy validation");
+       return err;
 }

 /**