netdev - Re: [PATCH] ipv6/addrconf: fix timing bug in tempaddr regen

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] [day] [month] [year] [list]

Message-ID: <CAH5Ym4gXVvESnE9ZbJL+QbucrOB=M4LXEZw+XvSA04xxFVfy6g@mail.gmail.com>
Date:   Thu, 26 May 2022 13:11:57 -0600
From:   Sam Edwards <cfsworks@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>
Subject: Re: [PATCH] ipv6/addrconf: fix timing bug in tempaddr regen

On Thu, May 26, 2022 at 1:40 AM Paolo Abeni <pabeni@...hat.com> wrote:
> 'age', 'ifp->prefered_lft' and 'regen_advance' are unsigned, and it
> looks like 'regen_advance' is not constrained to be less then 'ifp-
> >prefered_lft'. If that happens the condition will (allways) evaluate
> to false, there will be no temporary address regenaration,
> 'regen_count' will be untouched and the temporary address will never
> expire...

Hmm... I think the answer to whether `regen_advance` is constrained is
"yes but actually no."

ipv6_create_tempaddr() does have a check that will return early
without even creating an address if the calculated preferred lifetime
isn't greater than regen_advance. Now, if regen_advance were a
constant, I'd say that's the end of it, but it's actually computed
from some configuration variables which the system administrator might
change (increase) between then and here. So what you said could end up
happening, albeit in rare circumstances.

> Otherwise I think we need to explicitly handle the 'regen_advance >
> ifp->prefered_lft' condition possibly with something alike:
>
>         unsigned long regen_advance = ifp->idev->cnf.regen_max_retry * //...
>
>         regen_adavance = min(regen_advance, ifp->prefered_lft);
>         if (age >= ifp->prefered_lft - regen_advance) { //...

For readability's sake, it might be better to mirror what
ipv6_create_tempaddr does:
if (age + regen_advance >= ifp->prefered_lft)

Does that seem good? I doubt we'll have overflows as they would
require unreasonably large regen_advance values.

===

Now, in thinking about this some more, I'm starting to believe that
this if/elseif/elseif/... block is not the appropriate place for
tempaddr regeneration at all. The branches of this block implement the
*destructive* parts of an address's lifecycle. Most importantly, the
branches can be in latest-to-earliest order because the effects are
*cumulative*: if an address has a valid_lft only a second longer than
the prefered_lft, and addrconf_verify_rtnl runs a second late, then
the DEPRECATED branch will be skipped, but that's okay because the
address actually gets deleted instead. Regenerating a temporary
address is *not* a destructive stage in the address's lifecycle -- the
effects are not cumulative -- so if that stage is skipped,
regeneration never happens. My patch is trying to fix that by
essentially *holding up the lifecycle* until regeneration happens. But
that's leading to the more general concern I am seeing from you: a
logic error in regeneration (even one that has been there all along,
such as the potential underflow you just pointed out) could then
affect the whole deprecation flow, not just regeneration.

So, I think my v2 patch should probably put the tempaddr case right:

age = (now - ifp->tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
// here
if (ifp->valid_lft != INFINITY_LIFE_TIME &&
    age >= ifp->valid_lft) {

Thoughts? :)

Thank you for your input,
Sam