From: Juergen Perlinger <perlinger@ntp.org>
Date: Sun, 21 Feb 2016 08:39:25 +0000 (+0100)
Subject: Bug 3010] remote configuration trustedkey/requestkey values are not properly validated
X-Git-Tag: NTP_4_2_8P7~22^2^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc7d53eef18d9687e31bedce48aa6a59ee53dda7;p=thirdparty%2Fntp.git

Bug 3010] remote configuration trustedkey/requestkey values are not properly validated
 - sidekick: Ignore keys that have an unsupported MAC algorithm but are otherwise well-formed

bk: 56c977bdf6CLtHiqg1_rd2II7E0dqA
---

diff --git a/ChangeLog b/ChangeLog
index d5a2f9a23..79a953409 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,8 @@
 * [Bug 2995] Fixes to compile on Windows
 * [Bug 3010] remote configuration trustedkey/requestkey values
   are not properly validated. perlinger@ntp.org
+  - sidekick: Ignore keys that have an unsupported MAC algorithm
+    but are otherwise well-formed
 
 ---
 (4.2.8p6) 2016/01/20 Released by Harlan Stenn <stenn@ntp.org>
diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c
index 1d4ee3059..6dfca23d2 100644
--- a/libntp/authreadkeys.c
+++ b/libntp/authreadkeys.c
@@ -101,13 +101,33 @@ log_maybe(
 	...)
 {
 	va_list ap;
-	if (++(*pnerr) <= nerr_loglimit) {
+	if ((NULL == pnerr) || (++(*pnerr) <= nerr_loglimit)) {
 		va_start(ap, fmt);
 		mvsyslog(LOG_ERR, fmt, ap);
 		va_end(ap);
 	}
 }
 
+static void
+free_keydata(
+	KeyDataT *node
+	)
+{
+	KeyAccT *kap;
+	
+	if (node) {
+		while (node->keyacclist) {
+			kap = node->keyacclist;
+			node->keyacclist = kap->next;
+			free(kap);
+		}
+
+		/* purge secrets from memory before free()ing it */
+		memset(node, 0, sizeof(*node) + node->seclen);
+		free(node);
+	}
+}
+
 /*
  * authreadkeys - (re)read keys from a file.
  */
@@ -156,7 +176,7 @@ authreadkeys(
 		 * First is key number.  See if it is okay.
 		 */
 		keyno = atoi(token);
-		if (keyno == 0) {
+		if (keyno < 1) {
 			log_maybe(&nerr,
 				  "authreadkeys: cannot change key %s",
 				  token);
@@ -180,6 +200,14 @@ authreadkeys(
 				  keyno);
 			continue;
 		}
+
+		/* We want to silently ignore keys where we do not
+		 * support the requested digest type. OTOH, we want to
+		 * make sure the file is well-formed.  That means we
+		 * have to process the line completely and have to
+		 * finally throw away the result... This is a bit more
+		 * work, but it also results in better error detection.
+		 */ 
 #ifdef OPENSSL
 		/*
 		 * The key type is the NID used by the message digest 
@@ -189,30 +217,28 @@ authreadkeys(
 		 */
 		keytype = keytype_from_text(token, NULL);
 		if (keytype == 0) {
-			log_maybe(&nerr,
+			log_maybe(NULL,
 				  "authreadkeys: invalid type for key %d",
 				  keyno);
-			continue;
-		}
-		if (EVP_get_digestbynid(keytype) == NULL) {
-			log_maybe(&nerr,
+		} else if (EVP_get_digestbynid(keytype) == NULL) {
+			log_maybe(NULL,
 				  "authreadkeys: no algorithm for key %d",
 				  keyno);
-			continue;
+			keytype = 0;
 		}
 #else	/* !OPENSSL follows */
-
 		/*
 		 * The key type is unused, but is required to be 'M' or
 		 * 'm' for compatibility.
 		 */
 		if (!(*token == 'M' || *token == 'm')) {
-			log_maybe(&nerr,
+			log_maybe(NULL,
 				  "authreadkeys: invalid type for key %d",
 				  keyno);
-			continue;
+			keytype = 0;
+		} else {
+			keytype = KEY_TYPE_MD5;
 		}
-		keytype = KEY_TYPE_MD5;
 #endif	/* !OPENSSL */
 
 		/*
@@ -303,6 +329,13 @@ DPRINTF(0, ("authreadkeys: access list:  <%s>\n", tp));
 			}
 		}
 
+		/* check if this has to be weeded out... */
+		if (0 == keytype) {
+			free_keydata(next);
+			next = NULL;
+			continue;
+		}
+		
 		INSIST(NULL != next);
 		next->next = list;
 		list = next;
@@ -328,9 +361,8 @@ DPRINTF(0, ("authreadkeys: access list:  <%s>\n", tp));
 		list = next->next;
 		MD5auth_setkey(next->keyid, next->keytype,
 			       next->secbuf, next->seclen, next->keyacclist);
-		/* purge secrets from memory before free()ing it */
-		memset(next, 0, sizeof(*next) + next->seclen);
-		free(next);
+		next->keyacclist = NULL; /* consumed by MD5auth_setkey */
+		free_keydata(next);
 	}
 	return (1);
 
@@ -338,17 +370,7 @@ DPRINTF(0, ("authreadkeys: access list:  <%s>\n", tp));
 	/* Mop up temporary storage before bailing out. */
 	while (NULL != (next = list)) {
 		list = next->next;
-
-		while (next->keyacclist) {
-			KeyAccT *kap = next->keyacclist;
-
-			next->keyacclist = kap->next;
-			free(kap);
-		}
-
-		/* purge secrets from memory before free()ing it */
-		memset(next, 0, sizeof(*next) + next->seclen);
-		free(next);
+		free_keydata(next);
 	}
 	return (0);
 }