From fdfcabd79bd3a6a8377e7d5f608d79d6a1ba4313 Mon Sep 17 00:00:00 2001 From: Miroslav Lichvar Date: Thu, 10 Sep 2020 10:22:27 +0200 Subject: [PATCH] ntp: drop support for long NTPv4 MACs Don't accept NTPv4 packets which have a MAC longer than 24 octets to strictly follow RFC 7822, which specifies the maximum length of a MAC and the minimum length of the last extension field to avoid an ambiguity in parsing of the packet. This removes an ugly hack that was needed to accept packets that contained one or more extension fields without a MAC, before RFC 7822 was written and NTP implementations started using truncated MACs. The long MACs were used by chrony in versions 2.x when configured to authenticate a server or peer with a key using a 256-bit or longer hash (e.g. SHA256). For compatibility with chrony >= 4.0, these clients/peers will need to have "version 3" added to the server/peer line in chrony.conf. --- ntp_auth.c | 17 +---------------- ntp_core.c | 12 +++--------- test/unit/ntp_core.c | 21 ++++++++++++++++----- 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/ntp_auth.c b/ntp_auth.c index 50b78a8..58c8988 100644 --- a/ntp_auth.c +++ b/ntp_auth.c @@ -300,21 +300,9 @@ NAU_ParsePacket(NTP_Packet *packet, NTP_PacketInfo *info) if (remainder >= NTP_MIN_MAC_LENGTH && remainder <= NTP_MAX_V4_MAC_LENGTH) break; - /* The NTPv4-specific limit for MAC length enables deterministic parsing of - packets with extension fields (RFC 7822), but we support longer MACs in - packets with no extension fields for compatibility with older chrony - clients. Check if the longer MAC would authenticate the packet before - trying to parse the data as an extension field. */ - if (parsed == NTP_HEADER_LENGTH && - remainder > NTP_MAX_V4_MAC_LENGTH && remainder <= NTP_MAX_MAC_LENGTH && - KEY_CheckAuth(ntohl(*(uint32_t *)(data + parsed)), data, parsed, - data + parsed + 4, remainder - 4, NTP_MAX_MAC_LENGTH - 4)) - break; - /* Check if this is a valid NTPv4 extension field and skip it */ if (!NEF_ParseField(packet, info->length, parsed, &ef_length, &ef_type, NULL, NULL)) { - /* Invalid MAC or format error */ - DEBUG_LOG("Invalid format or MAC"); + DEBUG_LOG("Invalid format"); return 0; } @@ -340,9 +328,6 @@ NAU_ParsePacket(NTP_Packet *packet, NTP_PacketInfo *info) /* No MAC */ return 1; } else if (remainder >= NTP_MIN_MAC_LENGTH) { - /* This is not 100% reliable as a MAC could fail to authenticate and could - pass as an extension field, leaving reminder smaller than the minimum MAC - length */ info->auth.mode = NTP_AUTH_SYMMETRIC; info->auth.mac.start = parsed; info->auth.mac.length = remainder; diff --git a/ntp_core.c b/ntp_core.c index c306b03..60c0303 100644 --- a/ntp_core.c +++ b/ntp_core.c @@ -2095,15 +2095,6 @@ NCR_ProcessRxUnknown(NTP_Remote_Address *remote_addr, NTP_Local_Address *local_a CLG_LogAuthNtpRequest(); } - /* If it is an NTPv4 packet with a long MAC and no extension fields, - respond with an NTPv3 packet to avoid breaking RFC 7822 and keep - the length symmetric. Otherwise, respond with the same version. */ - if (info.version == 4 && info.ext_fields == 0 && info.auth.mode == NTP_AUTH_SYMMETRIC && - info.auth.mac.length > NTP_MAX_V4_MAC_LENGTH) - version = 3; - else - version = info.version; - local_ntp_rx = local_ntp_tx = NULL; tx_ts = NULL; interleaved = 0; @@ -2133,6 +2124,9 @@ NCR_ProcessRxUnknown(NTP_Remote_Address *remote_addr, NTP_Local_Address *local_a poll = CLG_GetNtpMinPoll(); poll = MAX(poll, message->poll); + /* Respond with the same version */ + version = info.version; + /* Send a reply */ transmit_packet(my_mode, interleaved, poll, version, kod, NULL, &message->receive_ts, &message->transmit_ts, diff --git a/test/unit/ntp_core.c b/test/unit/ntp_core.c index 63b2d55..a7f89ee 100644 --- a/test/unit/ntp_core.c +++ b/test/unit/ntp_core.c @@ -190,7 +190,7 @@ send_response(int interleaved, int authenticated, int allow_update, int valid_ts key_id = get_random_key_id(); auth_len = KEY_GetAuthLength(key_id); assert(auth_len); - if (NTP_LVM_TO_VERSION(res->lvm) == 4 && random() % 2) + if (NTP_LVM_TO_VERSION(res->lvm) == 4) auth_len = MIN(auth_len, NTP_MAX_V4_MAC_LENGTH - 4); if (KEY_GenerateAuth(key_id, (unsigned char *)res, NTP_HEADER_LENGTH, @@ -204,7 +204,7 @@ send_response(int interleaved, int authenticated, int allow_update, int valid_ts if (!valid_auth && authenticated) { assert(auth_len); - switch (random() % 4) { + switch (random() % 5) { case 0: key_id++; break; @@ -219,9 +219,20 @@ send_response(int interleaved, int authenticated, int allow_update, int valid_ts break; case 3: res_length = NTP_HEADER_LENGTH + 4 * (random() % ((4 + auth_len) / 4)); - if (NTP_LVM_TO_VERSION(res->lvm) == 4 && - res_length == NTP_HEADER_LENGTH + NTP_MAX_V4_MAC_LENGTH) - res_length -= 4; + break; + case 4: + if (NTP_LVM_TO_VERSION(res->lvm) == 4 && random() % 2 && + KEY_GetAuthLength(key_id) > NTP_MAX_V4_MAC_LENGTH - 4) { + auth_len += 4 + 4 * (random() % + ((KEY_GetAuthLength(key_id) - NTP_MAX_V4_MAC_LENGTH - 4) / 4)); + if (KEY_GenerateAuth(key_id, (unsigned char *)res, NTP_HEADER_LENGTH, + res->extensions + 4, auth_len) != auth_len) + assert(0); + res_length = NTP_HEADER_LENGTH + 4 + auth_len; + } else { + memset((unsigned char *)res + res_length, 0, 4); + res_length += 4; + } break; default: assert(0);