netdev - Re: [PATCH] rtnetlink: Mask the rta

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:	Thu, 14 Mar 2013 20:30:46 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Thomas Graf <tgraf@...g.ch>
CC:	Stephen Hemminger <stephen@...workplumber.org>,
	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] rtnetlink: Mask the rta_type when range checking

On 03/14/2013 05:28 PM, Thomas Graf wrote:
> On 03/14/13 at 01:40pm, Vlad Yasevich wrote:
>> So let me rebuff this a bit more intelligently.
>>
>> 1) NLA_F_NESTED is used by netfilter in a lot of places.  It seems that
>> only rtnetlink interface doesn't account for it.
>
> Stephen is not wrong, strictly speaking this is an ABI breaker.
>
> However, we have been applying NLA_TYPE_MASK in nla_type() and
> thus in nla_parse(), nlmsg_find_attr(), etc. since the
> introduction of the current kernel netlink API.
>
> static inline int nla_type(const struct nlattr *nla)
> {
>          return nla->nla_type & NLA_TYPE_MASK;
> }
>
> I have been attempting to get rid of that ugly attribute parsing
> code in rtnetlink_rcv_msg() but it's hard to get rid of. I shall
> give this another try.
>
>> 2) The following commit:
>>
>> commit 25c71c75ac87508528db053b818944f3650dd7a6
>> Author: stephen hemminger <shemminger@...tta.com>
>> Date:   Tue Nov 13 07:53:05 2012 +0000
>>
>>      bridge: bridge port parameters over netlink
>>
>> introduced NLA_F_NESTED usage in rtnetlink for both setlink and
>> getlink, so one could argue that was an ABI change.
>
> You are right, user space applications that do not apply
> NLA_TYPE_MASK will no longer see the IFLA_PROTINFO attribute
> with the above commit.
>
>> prove that without the change I am introducing, one can not use the
>> above mentioned bridge API.  Feel free to try it with iproute2
>> patches
>> I sent earlier ([PATCH iproute2 0/2] Add support for bridge port
>> link information).
>
> I am not against your patch but I would love to see the affected
> code paths to no longer rely on the attribute array provided by
> the rtnetlink doit function but instead call nla_parse() themselves
> for the sake of proper validation.
>

Doing a quick check on all the callers for rtnl_register and their 
handlers the following do not use nla_parse:
  1) dn_fib_rtm_newroute/delroute  - Don't seem to care about attribute
                                     types.
  2) dn_cache_getroute() - suspect use.  relies on rta_buf populated by
                           rtnetlink_rcv_msg

  3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop
                                   with nla_type(), so safe.

That's all that a quick look finds.  Out of all of them, looks like on 
dn_cache_getroute() would be broken.

-vlad



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html