[PATCH net IPv6]: Fix maximum IPv6 address limit violation

[PATCH net IPv6]: Fix maximum IPv6 address limit violation

[Balakumaran Kannan]

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(+)

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,
-- 
1.7.9.5

Re: [PATCH net IPv6]: Fix maximum IPv6 address limit violation

[Hannes Frederic Sowa]

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

Re: [PATCH net IPv6]: Fix maximum IPv6 address limit violation

[Balakumaran Kannan]

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

Re: [PATCH net IPv6]: Fix maximum IPv6 address limit violation

[David Miller]

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.