[ntp:security] [Bug 3114] Broadcast Mode Replay Prevention DoS

bugzilla-daemon at ntp.org bugzilla-daemon at ntp.org
Wed Sep 14 05:49:34 UTC 2016


--- Comment #10 from Juergen Perlinger <perlinger at ntp.org> 2016-09-14 05:49:34 UTC ---
(In reply to comment #5)
> (In reply to comment #3)
> > I'm working my way through the diff manually, since it is against the GIT repo
> > which is not quite the 4.2.8p8 release. AFAIK there has been trouble with the
> > GIT export, which makes things awkward to handle. Anyway, I'll cope somehow.
> I'm sorry about that.  I'm guessing someone edited the disclosure in a
> non-whitespace-preserving editor before it made it out the door. :-/  I can
> provide a patch against 4.2.8p8 if you like.

There have been some other changes, too -- it's not just whitespace. As I said,
just a nuisance, not a problem.

> > But there is at least one issue: The snippet in
> > 
> > @@ -1348,9 +1347,8 @@ receive(
> > 
> > damages the compare op. Since the NTP time scale is seconds since 1900 (mod
> > 2^32), a simple compare is not enough to handle the wrap-around / overflow. One
> > has indeed to take the difference (mod 2^32) and check if the result is
> > negative. So I won't apply that fragment, as it is definitely wrong.
> I'm pretty sure that the current code doesn't handle wrap-around / overflow
> gracefully either, so I think they're equivalent on that criteria.  I changed:
>     tdiff = p_xmt;
>     L_SUB(&tdiff, &peer->bxmt);
>     if (tdiff.l_i < 0) { ...
> to
>     if(!L_ISGEQU(&p_xmt, &peer->bxmt)){ ...
> because the first code has a subtle bug.  If the incoming timestamp is
> non-monotonic (p_xmt < peer->bxmt), then it works as expected (tdiff.l_i < 0 ==
> TRUE).  However, tdiff.l_i < 0 will also be true if p_xmt > peer->bxmt +
> 0x80000000.  So, if peer->bxmt currently contains a small value but a
> legitimate incoming p_xmt is greater than 2^31, the incoming packet will be
> rejected for having a non-monotonic timestamp.  This issue may have been
> obscured by the fact that peer->bxmt was being updated by packets that were
> failing the integrity check.  I changed it to an unsigned greater than or equal
> comparison to ensure that packets meeting p_xmt > peer->bxmt + 0x80000000 would
> not be rejected.

That's the problem when doing calculations in a number ring... Conceptually we
have to split the interval (2^32) into a range that is considered 'forward' and
one that is 'backward'. The sum of both ranges must not be bigger than the full
range. So I took the same split as two's complement does on signed numbers,
making 0x8000000 the most negative(!) value. As a result, x+0x8000000 becomes
conceptually smaller than x.

It might make sense the check for tdiff <= 0 instead of tdiff < 0 -- this would
weed out equal time stamps, too.

But believe it or not, the diff+comp code handles the era rollover ;) hint:

a < b          ===  a - b < 0            obvious for (unlimited) numbers
a < b  (mod N) !=! a - b (mod N) < 0     that's what we have here


Configure bugmail: https://bugs.ntp.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

More information about the security mailing list