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 d5a2f9a2323a80c6a4f27f914b4347706f72a4ab..79a953409f6877a5982f70e007e158ce6ec3fe78 100644 (file)
--- 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 1d4ee3059345f429decaa010532a71e008c77554..6dfca23d2630b316f35676a41b378f2dd7d60b27 100644 (file)
--- 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);
 }