[ntp:hackers] [Bug 1378] Unnecessary resetting of peers during interface update

Frank Kardel kardel at ntp.org
Sat Nov 14 20:59:35 UTC 2009


Danny !

I believe you have seen this in the wild, But up to now I have yet to 
see logs that document such an incident.

As for resetting the peer - there was once a version that just cleared 
the crypto information. Dave suggested
on 2007-06-21 a conservative approach for the matter:

Dave Mills wrote 2007-06-21:
To be brutally security-correct, once you change cryptographic 
credentials, the association must be completely reset and all old 
information purged. That's what the peer_clear() routine is for. So, 
just change the address, call peer_clear() and let nature take its course.

Thats how it was implemented then. We could avoid resetting a peer if 
not crypto is going on - but you mentioned crypto associationg being 
affected by this - so it wouldn't help there.

I am not saying that a routing change is more important than keeping the 
association. It is that if an association would e configured at 
interface scan time and we detect that a different local address would 
be configured we just track this change of the local address (currently 
with resetting the peer which could be refined somewhat - but see Dave's 
comment above). No attempt is made to examine if the previous 
configuration is still operable - that would cause many decisions like 
'when is it inoperable?'.

You mention broadcast as being problematic here. This is where I do not 
get you. The code in ntp_peer.c:peer_refresh_interface() reads:
   ...
    set_peerdstadr(peer, niface);
    if (peer->dstadr) {
        /*
         * clear crypto if we change the local address
         */
        if (peer->dstadr != piface && !(peer->cast_flags &
            (MDF_ACAST | MDF_BCLNT)))
            peer_clear(peer, "XFAC");
   ...

broadcast clients are mobilized with the MDF_BCLNT flag - why, then are 
you seeing peer-resets when MDF_BCLNT is set? Ohh, wait -  I just found 
in ntp_proto.c a section that clears MDF_BCLNT when peer->pmode == 
MODE_BROADCAST - so the assumption this being a stable config flag is 
false - it may have been true in another time - nowadays it seems to be 
a calibration flag.

So would it be safer to use peer->pmode == MODE_BROADCAST as safeguard 
against peer_clears() ?

The clearing of MDF_BCLNT now explains why broadcast associations get 
cleared.

Still open is the question why a change in the local address binding is 
detected - again a log would help.
My suspicion with broadcast client is that this may be an effect of 
findpeer() setting the peer local address to the interface * passed to 
it. If that does not match the determined local address (reason to be 
determined) and MDF_BCLNT being cleared in ntp_proto.c this could be the 
culprit.

So instead of modifying the interface scan code I'd recommend to replace 
the MDF_BCLNT bit check by a check for peer->pmode == MODE_BROADCAST in 
peer_refresh_interface().  In set_peerdstadr() we should probably check 
for the presense of (peer->hmode ==  MODE_BCLIENT. or MDF_BCLNT bit) to 
avoid
overwritig the local address.

To sum up: it seems that MDF_BCLNT is not the flag to check to avoid 
peer_clear()ing broadcast associations. The above modification should be 
tested whether they solve the problem.

Frank

Danny Mayer wrote:
> I am redirecting the discussion to hackers and copying Dave on this.
> While you claim that nothing happens if nothing has changed, I beg to
> differ especially as I see this in the wild. I fail to see why it is
> essential to reset peers just because routing has changed as long as the
> client is still receiving the packets. The underlying code takes care of
> dealing with the the delays and jitter caused by the existing routing,
> but yanking the association causes a lot of problems especially when
> dealing with authenticating peers.
>
> What you appear to be saying here is that the routing change is more
> important than keeping the association which I think is fundamentally
> wrong. If you had lost the connection that would be something different
> but that has not been the case here in the two situations that I have
> looked at. The routing changes that you write about seem to be only
> about sending packets and not about receiving packets. Since broadcast
> is only about receiving packets (outside of the autokey dance) this
> should not normally apply to broadcast associations.
>
> Note that the changes I have made cause the refresh to happen if a local
> address has been added or removed but that will not happen on statically
> allocated addresses.
>
> Dave, can you weigh in here on your opinion on this?
>
> Danny
>
>   



More information about the hackers mailing list