[ntp:security] NOEPEER patch

Harlan Stenn stenn at nwtime.org
Fri Aug 3 08:52:03 UTC 2018


Martin,

On 8/3/2018 12:51 AM, Martin Burnicki wrote:
> Harlan Stenn wrote:
>> OK, I had a talk with Dave today.
>>
>> This gets more fascinating to me, and more intricate.
>>
>> ...
>>
>> If a valid crypto-NAK is received, it really doesn't matter:
>>
>> - what mode it is (although it should be appropriate to the request)
>> - what ANY of the content values are (LI bits, timestamps, etc,
>>   other than the stamps to show it is a valid response packet)
>>
>> because EVERY client that receives a valid crypto-NAK should squawk and
>> then drop the packet.  There is NO CASE where the crypto-NAK packet
>> should be expected to contain valid response data.
> 
> OK, that's basically what happens.
> 
>> So we're back to:
>>
>> - why did a crypto-NAK get sent?
> 
> Because the request was sent with an unknown/invalid **symmetric** key
> in this case.
> 
>> - why did the receiving system think the crypto-NAK was invalid?
> 
> I've had a look at the valid_NAK() function in ntp_proto.c.
> 
> There's a code section that originally reads:
> 
>   /*
>    * Only valid if peer uses a key
>    */
>   if (!peer || !peer->keyid || !(peer->flags & FLAG_SKEY)) {
>     return INVALIDNAK;
>   }
> 
> However, according to the comment near the the definition of FLAG_SKEY
> in ntp.h
> 
> #define	FLAG_SKEY	0x0800  /* autokey authentication */
> 
> FLAG_SKEY is only set in case of autokey, and it's actually not set in
> this case where a packet has been received with an unknown/invalid
> **symmetric** key.
> 
> After I've removed the FLAG_SKEY check in the code above, these
> misleading error messages disappear, but in the output of e.g. "ntpq -c
> as" the peer association is still listed with "auth" set to "bad", as
> expected.
> 
> bk annotate says the FLAG_SKEY check has been introduced by Pearly,
> probably with autokey in mind. Pearly, do you think it's OK to remove
> this check, like I did?

I don't think that's what we want.

Before Pearly's change, the code was:

  if (   peer
      && (peer->keyid > 0 || peer->flags & FLAG_SKEY))
	return VALIDNAK;

and now it is:

  if (!peer || !peer->keyid || !(peer->flags & FLAG_SKEY)
	return INVALIDNAK;

and I think we want:

  if (!peer || (!peer->keyid && !(peer->flags & FLAG_SKEY))
	return INVALIDNAK;

or:

  if (!peer || !(peer->keyid || (peer->flags & FLAG_SKEY))
	return INVALIDNAK;

In grad school, I didn't hate De Morgan's laws.  Having said that,
Karnaugh Maps are not my friends.

Having said *that*, it's nearly 0200 and I'm not as awake as I'd like to be.
-- 
Harlan Stenn, Network Time Foundation
http://nwtime.org - be a Member!


More information about the security mailing list