| Message ID | 533FEE9B.5090806@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi! On Sat, Apr 05, 2014 at 05:22:59PM +0530, Balakumaran Kannan wrote: > Kernel doesn't check with max IPv6 address limit before adding IPv6 temporary > address. > > Security fix CVE-2013-0343 removes max_addresses check from ipv6_create_tempaddr > function as this is handled before in addrconf_prefix_rcv function. But > addrconf_prefix_rcv does max_addresses check only before adding MAC based RA > address and if limit is already reached, it stops processing the prefix. > > When IPv6 privacy extension is enabled, two addresses will be created for a > new prefix received through RA. So if a machine has (max_addresses - 1) number > of IPv6 addresses, after receiving an RA with new prefix the machine will have > (max_addresses + 1) number of IPv6 addresses. > > So it is better to use a new prefix only if two IPv6 address slots available > in case IPv6 privacy extension is enabled. When I removed the check I did not think about any reason why the max_addresses must count exact. It is merely there to prevent DoS attacks. Is there a reason to revisit this? We don't care about locally added addresses and I decided to do so for temporary addresses, too, because only some small number will ever be generated for an autoconfigured one. Actually when rolling over privacy addresses we generate new ones before the old ones expire, so there could be more of those. This depends on the setting of the timers. IMHO we don't need this additional check and complexity or this there a reason I don't currently see? Thanks for the submission, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hannes, >> Kernel doesn't check with max IPv6 address limit before adding IPv6 temporary >> address. >> >> Security fix CVE-2013-0343 removes max_addresses check from ipv6_create_tempaddr >> function as this is handled before in addrconf_prefix_rcv function. But >> addrconf_prefix_rcv does max_addresses check only before adding MAC based RA >> address and if limit is already reached, it stops processing the prefix. >> >> When IPv6 privacy extension is enabled, two addresses will be created for a >> new prefix received through RA. So if a machine has (max_addresses - 1) number >> of IPv6 addresses, after receiving an RA with new prefix the machine will have >> (max_addresses + 1) number of IPv6 addresses. >> >> So it is better to use a new prefix only if two IPv6 address slots available >> in case IPv6 privacy extension is enabled. > > When I removed the check I did not think about any reason why the > max_addresses must count exact. It is merely there to prevent DoS attacks. > Is there a reason to revisit this? We don't care about locally added > addresses and I decided to do so for temporary addresses, too, because > only some small number will ever be generated for an autoconfigured one. > > Actually when rolling over privacy addresses we generate new ones before > the old ones expire, so there could be more of those. This depends on > the setting of the timers. > > IMHO we don't need this additional check and complexity or this there > a reason I don't currently see? > >Thanks for the submission, > > Hannes Thanks for the explanation. I agree with your argument that the reason behind max_addresses is to avoid DOS attack. But my humble opinion is when we impose some constrains, we must adhere to that. I don't think adding tolerance to an integer limit is a good policy. And before the security fix, there was an additional check while creating temporary address. I thought this additional check is a better way and try to make the code behave similar. But anyway the impact of this is very minimal or no impact at all. Thats why I kept the severity to less. I see this patch more policy oriented than technical. So either accepting or rejecting this is depending on 'how we code' than 'what we code'. Once again thanks for the detailed explanation. Regards, K.Balakumaran -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Balakumaran Kannan <kumaran.4353@gmail.com> Date: Sat, 05 Apr 2014 17:22:59 +0530 > +#ifdef CONFIG_IPV6_PRIVACY > + /* When IPv6 privacy extension is enabled, there must > + * be two IPv6 address slots available. > + * - One for MAC based address > + * - Another for temporary address > + */ > + if (max_addresses > 1) > + max_addresses--; > +#endif > if (!max_addresses || > ipv6_count_addresses(in6_dev) < max_addresses) > ifp = ipv6_add_addr(in6_dev, &addr, NULL, > -- > 1.7.9.5 > Your claim is that you're shooting for accurate policy enforcement, but even you patch doesn't do this. You are not taking into account whether temporary addresses will even be created. An accurate implementation would duplicate all of the special checks that manage_tempaddrs() does to determine whether a temporary address will be created or not. In particular, it needs to check if the device is even configured to use tempaddrs. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 6c7fa08..8a7e4ba 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -2278,6 +2278,15 @@ ok: /* Do not allow to create too much of autoconfigured * addresses; this would be too easy way to crash kernel. */ +#ifdef CONFIG_IPV6_PRIVACY + /* When IPv6 privacy extension is enabled, there must + * be two IPv6 address slots available. + * - One for MAC based address + * - Another for temporary address + */ + if (max_addresses > 1) + max_addresses--; +#endif if (!max_addresses || ipv6_count_addresses(in6_dev) < max_addresses) ifp = ipv6_add_addr(in6_dev, &addr, NULL,
Kernel doesn't check with max IPv6 address limit before adding IPv6 temporary address. Security fix CVE-2013-0343 removes max_addresses check from ipv6_create_tempaddr function as this is handled before in addrconf_prefix_rcv function. But addrconf_prefix_rcv does max_addresses check only before adding MAC based RA address and if limit is already reached, it stops processing the prefix. When IPv6 privacy extension is enabled, two addresses will be created for a new prefix received through RA. So if a machine has (max_addresses - 1) number of IPv6 addresses, after receiving an RA with new prefix the machine will have (max_addresses + 1) number of IPv6 addresses. So it is better to use a new prefix only if two IPv6 address slots available in case IPv6 privacy extension is enabled. Severity: Less Signed-off-by: Balakumaran Kannan <kumaran.4353@gmail.com> --- How to reproduce: 1. Enable IPv6 privacy extension by setting value of /proc/sys/net/ipv6/conf/<iface>/use_tempaddr to 2. 2. Flood RA with different prefixes. 3. Check total number of IPv6 address assigned for your iface. By default max_addresses will be 16. So after receiving 8 RAs, there will be 17 IPv6 addresses including link-local address. --- net/ipv6/addrconf.c | 9 +++++++++ 1 file changed, 9 insertions(+)