From d48f01280977582f4aaadc5dee4cae2e64dc7ab2 Mon Sep 17 00:00:00 2001 From: Miroslav Lichvar Date: Mon, 27 Jul 2020 15:38:46 +0200 Subject: [PATCH] nts: improve NTS-NTP server/client code Add more comments, assertions, debug messages, and other minor changes to make the code more robust. --- nts_ntp_auth.c | 9 +++++++-- nts_ntp_client.c | 44 +++++++++++++++++++++++++++++--------------- nts_ntp_server.c | 26 +++++++++++++++++++------- 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/nts_ntp_auth.c b/nts_ntp_auth.c index 86cd7cd..7580377 100644 --- a/nts_ntp_auth.c +++ b/nts_ntp_auth.c @@ -102,6 +102,10 @@ NNA_GenerateAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, body = (unsigned char *)(header + 1); ciphertext = body + nonce_length + nonce_padding; + if ((unsigned char *)header + auth_length != + ciphertext + ciphertext_length + ciphertext_padding + additional_padding) + assert(0); + memcpy(body, nonce, nonce_length); memset(body + nonce_length, 0, nonce_padding); @@ -135,7 +139,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in NULL, &ef_type, &ef_body, &ef_body_length)) return 0; - if (ef_type != NTP_EF_NTS_AUTH_AND_EEF) + if (ef_type != NTP_EF_NTS_AUTH_AND_EEF || ef_body_length < sizeof (*header)) return 0; header = ef_body; @@ -148,7 +152,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in return 0; nonce = (unsigned char *)(header + 1); - ciphertext = (unsigned char *)(header + 1) + get_padded_length(nonce_length); + ciphertext = nonce + get_padded_length(nonce_length); siv_tag_length = SIV_GetTagLength(siv); @@ -166,6 +170,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in } *plaintext_length = ciphertext_length - siv_tag_length; + assert(*plaintext_length >= 0); if (!SIV_Decrypt(siv, nonce, nonce_length, packet, ef_start, ciphertext, ciphertext_length, plaintext, *plaintext_length)) { diff --git a/nts_ntp_client.c b/nts_ntp_client.c index effe060..79136f0 100644 --- a/nts_ntp_client.c +++ b/nts_ntp_client.c @@ -43,8 +43,10 @@ #include "siv.h" #include "util.h" +/* Maximum length of all cookies to avoid IP fragmentation */ #define MAX_TOTAL_COOKIE_LENGTH (8 * 108) +/* Magic string of files containing keys and cookies */ #define DUMP_IDENTIFIER "NNC0\n" struct NNC_Instance_Record { @@ -87,6 +89,7 @@ reset_instance(NNC_Instance inst) memset(&inst->context, 0, sizeof (inst->context)); inst->context_id = 0; + memset(inst->cookies, 0, sizeof (inst->cookies)); inst->num_cookies = 0; inst->cookie_index = 0; inst->nak_response = 0; @@ -136,17 +139,15 @@ NNC_DestroyInstance(NNC_Instance inst) static int check_cookies(NNC_Instance inst) { - /* Force NKE if a NAK was received since last valid auth */ - if (inst->nak_response && !inst->ok_response && inst->num_cookies > 0) { + /* Force a new NTS-KE session if a NAK was received without a valid response, + or the keys encrypting the cookies need to be refreshed */ + if (inst->num_cookies > 0 && + ((inst->nak_response && !inst->ok_response) || + SCH_GetLastEventMonoTime() - inst->last_nke_success > CNF_GetNtsRefresh())) { inst->num_cookies = 0; DEBUG_LOG("Dropped cookies"); } - /* Force NKE if the keys encrypting the cookies are too old */ - if (inst->num_cookies > 0 && - SCH_GetLastEventMonoTime() - inst->last_nke_success > CNF_GetNtsRefresh()) - inst->num_cookies = 0; - return inst->num_cookies > 0; } @@ -203,10 +204,11 @@ get_cookies(NNC_Instance inst) double now; int got_data; - assert(!check_cookies(inst)); + assert(inst->num_cookies == 0); now = SCH_GetLastEventMonoTime(); + /* Create and start a new NTS-KE session if not already present */ if (!inst->nke) { if (now < inst->next_nke_attempt) { DEBUG_LOG("Limiting NTS-KE request rate (%f seconds)", @@ -231,9 +233,13 @@ get_cookies(NNC_Instance inst) update_next_nke_attempt(inst, now); + /* Wait until the session stops */ if (NKC_IsActive(inst->nke)) return 0; + assert(sizeof (inst->cookies) / sizeof (inst->cookies[0]) == NTS_MAX_COOKIES); + + /* Get the new keys, cookies and NTP address if the session was successful */ got_data = NKC_GetNtsData(inst->nke, &inst->context, inst->cookies, &inst->num_cookies, NTS_MAX_COOKIES, &ntp_address); @@ -250,17 +256,18 @@ get_cookies(NNC_Instance inst) inst->context_id++; + /* Force a new session if the NTP address is used by another source, with + an expectation that it will eventually get a non-conflicting address */ if (!set_ntp_address(inst, &ntp_address)) { inst->num_cookies = 0; return 0; } + inst->last_nke_success = now; inst->cookie_index = 0; inst->nak_response = 0; - inst->last_nke_success = now; - return 1; } @@ -269,11 +276,13 @@ get_cookies(NNC_Instance inst) int NNC_PrepareForAuth(NNC_Instance inst) { + /* Try to reload saved keys and cookies (once for the NTS-KE address) */ if (!inst->load_attempt) { load_cookies(inst); inst->load_attempt = 1; } + /* Get new cookies if there are not any, or they are no longer usable */ if (!check_cookies(inst)) { if (!get_cookies(inst)) return 0; @@ -288,8 +297,9 @@ NNC_PrepareForAuth(NNC_Instance inst) return 0; } - UTI_GetRandomBytes(&inst->uniq_id, sizeof (inst->uniq_id)); - UTI_GetRandomBytes(&inst->nonce, sizeof (inst->nonce)); + /* Prepare data for NNC_GenerateRequestAuth() */ + UTI_GetRandomBytes(inst->uniq_id, sizeof (inst->uniq_id)); + UTI_GetRandomBytes(inst->nonce, sizeof (inst->nonce)); return 1; } @@ -315,7 +325,7 @@ NNC_GenerateRequestAuth(NNC_Instance inst, NTP_Packet *packet, MAX_TOTAL_COOKIE_LENGTH / (cookie->length + 4)); if (!NEF_AddField(packet, info, NTP_EF_NTS_UNIQUE_IDENTIFIER, - &inst->uniq_id, sizeof (inst->uniq_id))) + inst->uniq_id, sizeof (inst->uniq_id))) return 0; if (!NEF_AddField(packet, info, NTP_EF_NTS_COOKIE, @@ -372,6 +382,9 @@ extract_cookies(NNC_Instance inst, unsigned char *plaintext, int length) continue; index = (inst->cookie_index + inst->num_cookies) % NTS_MAX_COOKIES; + assert(index >= 0 && index < NTS_MAX_COOKIES); + assert(sizeof (inst->cookies) / sizeof (inst->cookies[0]) == NTS_MAX_COOKIES); + memcpy(inst->cookies[index].cookie, ef_body, ef_body_length); inst->cookies[index].length = ef_body_length; inst->num_cookies++; @@ -398,7 +411,7 @@ NNC_CheckResponseAuth(NNC_Instance inst, NTP_Packet *packet, if (info->ext_fields == 0 || info->mode != MODE_SERVER) return 0; - /* Accept only one response per request */ + /* Accept at most one response per request */ if (inst->ok_response) return 0; @@ -411,7 +424,8 @@ NNC_CheckResponseAuth(NNC_Instance inst, NTP_Packet *packet, for (parsed = NTP_HEADER_LENGTH; parsed < info->length; parsed += ef_length) { if (!NEF_ParseField(packet, info->length, parsed, &ef_length, &ef_type, &ef_body, &ef_body_length)) - break; + /* This is not expected as the packet already passed NAU_ParsePacket() */ + return 0; switch (ef_type) { case NTP_EF_NTS_UNIQUE_IDENTIFIER: diff --git a/nts_ntp_server.c b/nts_ntp_server.c index 6ab8fb9..399f7fa 100644 --- a/nts_ntp_server.c +++ b/nts_ntp_server.c @@ -112,15 +112,18 @@ NNS_CheckRequestAuth(NTP_Packet *packet, NTP_PacketInfo *info, uint32_t *kod) for (parsed = NTP_HEADER_LENGTH; parsed < info->length; parsed += ef_length) { if (!NEF_ParseField(packet, info->length, parsed, &ef_length, &ef_type, &ef_body, &ef_body_length)) - break; + /* This is not expected as the packet already passed NAU_ParsePacket() */ + return 0; switch (ef_type) { case NTP_EF_NTS_UNIQUE_IDENTIFIER: has_uniq_id = 1; break; case NTP_EF_NTS_COOKIE: - if (has_cookie || ef_body_length > sizeof (cookie.cookie)) + if (has_cookie || ef_body_length > sizeof (cookie.cookie)) { + DEBUG_LOG("Unexpected cookie/length"); return 0; + } cookie.length = ef_body_length; memcpy(cookie.cookie, ef_body, ef_body_length); has_cookie = 1; @@ -199,13 +202,18 @@ NNS_CheckRequestAuth(NTP_Packet *packet, NTP_PacketInfo *info, uint32_t *kod) return 0; } + /* Prepare data for NNS_GenerateResponseAuth() to minimise the time spent + there (when the TX timestamp is already set) */ + UTI_GetRandomBytes(server->nonce, sizeof (server->nonce)); - server->num_cookies = MIN(NTS_MAX_COOKIES, requested_cookies); - for (i = 0; i < server->num_cookies; i++) + assert(sizeof (server->cookies) / sizeof (server->cookies[0]) == NTS_MAX_COOKIES); + for (i = 0; i < NTS_MAX_COOKIES && i < requested_cookies; i++) if (!NKS_GenerateCookie(&context, &server->cookies[i])) return 0; + server->num_cookies = i; + return 1; } @@ -224,14 +232,16 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info, if (!server || req_info->mode != MODE_CLIENT || res_info->mode != MODE_SERVER) return 0; - /* Make sure this is a response to the expected request */ + /* Make sure this is a response to the request from the last call + of NNS_CheckRequestAuth() */ if (UTI_CompareNtp64(&server->req_tx, &request->transmit_ts) != 0) assert(0); for (parsed = NTP_HEADER_LENGTH; parsed < req_info->length; parsed += ef_length) { if (!NEF_ParseField(request, req_info->length, parsed, &ef_length, &ef_type, &ef_body, &ef_body_length)) - break; + /* This is not expected as the packet already passed NAU_ParsePacket() */ + return 0; switch (ef_type) { case NTP_EF_NTS_UNIQUE_IDENTIFIER: @@ -249,7 +259,7 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info, for (i = 0, plaintext_length = 0; i < server->num_cookies; i++) { if (!NEF_SetField(plaintext, sizeof (plaintext), plaintext_length, - NTP_EF_NTS_COOKIE, &server->cookies[i].cookie, + NTP_EF_NTS_COOKIE, server->cookies[i].cookie, server->cookies[i].length, &ef_length)) return 0; @@ -259,6 +269,8 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info, server->num_cookies = 0; + /* Generate an authenticator field which will make the length + of the response equal to the length of the request */ if (!NNA_GenerateAuthEF(response, res_info, server->siv, server->nonce, sizeof (server->nonce), plaintext, plaintext_length,