[Bug 2957] format specifies type 'unsigned int' but the argument has type 'size_t'

 - accept key file only if there are no parsing errors
 - fixed size_t/u_int format clash
 - restore pre-Bug1243 compatibility (revert 'strlcpy()' to 'strncpy()' as 'strlcpy()' is wrong here)

bk: 563c57c309lnETVruYn4ZaucSbQRiQ

diff --git a/ChangeLog b/ChangeLog
index b170d63fd58af327f9f686079588ea108eb235f9..0f9ac65a0e8a8c250b40eabb33ab0b23ce718cc2 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -5,6 +5,10 @@
 * [Bug 2954] Version 4.2.8p4 crashes on startup with sig fault
   - fixed data race conditions in threaded DNS worker. perlinger@ntp.org
   - limit threading warm-up to linux; FreeBSD bombs on it. perlinger@ntp.org
+* [Bug 2957] 'unsigned int' vs 'size_t' format clash. perlinger@ntp.org
+  - accept key file only if there are no parsing errors
+  - fixed size_t/u_int format clash
+  - fixed wrong use of 'strlcpy'
 ---
 (4.2.8p4) 2015/10/21 Released by Harlan Stenn <stenn@ntp.org>
 (4.2.8p4-RC1) 2015/10/06 Released by Harlan Stenn <stenn@ntp.org>

diff --git a/libntp/authkeys.c b/libntp/authkeys.c
index 667ca298b96fa38d9a7cc6ad43f267ebd151ad67..5bd696cc2758aca6d843d8568e5472a74a1f72d7 100644 (file)
--- a/libntp/authkeys.c
+++ b/libntp/authkeys.c
@@ -63,7 +63,7 @@ symkey        key_listhead;           /* list of all in-use keys */;
  * keyid. We make this fairly big for potentially busy servers.
  */
 #define        DEF_AUTHHASHSIZE        64
-//#define      HASHMASK        ((HASHSIZE)-1)
+/*#define      HASHMASK        ((HASHSIZE)-1)*/
 #define        KEYHASH(keyid)  ((keyid) & authhashmask)
 
 int    authhashdisabled;
@@ -511,7 +511,17 @@ authistrusted(
        return TRUE;
 }
 
-
+/* Note: There are two locations below where 'strncpy()' is used. While
+ * this function is a hazard by itself, it's essential that it is used
+ * here. Bug 1243 involved that the secret was filled with NUL bytes
+ * after the first NUL encountered, and 'strlcpy()' simply does NOT have
+ * this behaviour. So disabling the fix and reverting to the buggy
+ * behaviour due to compatibility issues MUST also fill with NUL and
+ * this needs 'strncpy'. Also, the secret is managed as a byte blob of a
+ * given size, and eventually truncating it and replacing the last byte
+ * with a NUL would be a bug.
+ * perlinger@ntp.org 2015-10-10
+ */
 void
 MD5auth_setkey(
        keyid_t keyno,
@@ -546,7 +556,8 @@ MD5auth_setkey(
 #ifndef DISABLE_BUG1243_FIX
                        memcpy(sk->secret, key, secretsize);
 #else
-                       strlcpy((char *)sk->secret, (const char *)key,
+                       /* >MUST< use 'strncpy()' here! See above! */
+                       strncpy((char *)sk->secret, (const char *)key,
                                secretsize);
 #endif
                        if (cache_keyid == keyno) {
@@ -565,7 +576,8 @@ MD5auth_setkey(
 #ifndef DISABLE_BUG1243_FIX
        memcpy(secret, key, secretsize);
 #else
-       strlcpy((char *)secret, (const char *)key, secretsize);
+       /* >MUST< use 'strncpy()' here! See above! */
+       strncpy((char *)secret, (const char *)key, secretsize);
 #endif
        allocsymkey(bucket, keyno, 0, (u_short)keytype, 0,
                    (u_short)secretsize, secret);
@@ -647,7 +659,7 @@ authencrypt(
        u_int32 *       pkt,
        int             length
        )
-{\
+{
        /*
         * A zero key identifier means the sender has not verified
         * the last message was correctly authenticated. The MAC

diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c
index 1c4c07ca5926784b708ba91161504be795772e67..5107dd0962f2a82a43c399ff3bf71e585b8cb76b 100644 (file)
--- a/libntp/authreadkeys.c
+++ b/libntp/authreadkeys.c
@@ -77,14 +77,23 @@ nexttok(
  * data on global/static level.
  */
 
-static const size_t nerr_loglimit = 5u;
-static const size_t nerr_maxlimit = 15;
+static const u_int nerr_loglimit = 5u;
+static const u_int nerr_maxlimit = 15;
 
-static void log_maybe(size_t*, const char*, ...) NTP_PRINTF(2, 3);
+static void log_maybe(u_int*, const char*, ...) NTP_PRINTF(2, 3);
+
+typedef struct keydata KeyDataT;
+struct keydata {
+       KeyDataT *next;         /* queue/stack link             */
+       keyid_t   keyid;        /* stored key ID                */
+       u_short   keytype;      /* stored key type              */
+       u_short   seclen;       /* length of secret             */
+       u_char    secbuf[1];    /* begin of secret (formal only)*/
+};
 
 static void
 log_maybe(
-       size_t     *pnerr,
+       u_int      *pnerr,
        const char *fmt  ,
        ...)
 {
@@ -113,25 +122,24 @@ authreadkeys(
        u_char  keystr[32];             /* Bug 2537 */
        size_t  len;
        size_t  j;
-       size_t  nerr;
+       u_int   nerr;
+       KeyDataT *list = NULL;
+       KeyDataT *next = NULL;
        /*
         * Open file.  Complain and return if it can't be opened.
         */
        fp = fopen(file, "r");
        if (fp == NULL) {
-               msyslog(LOG_ERR, "authreadkeys: file %s: %m",
+               msyslog(LOG_ERR, "authreadkeys: file '%s': %m",
                    file);
-               return (0);
+               goto onerror;
        }
        INIT_SSL();
 
        /*
-        * Remove all existing keys
-        */
-       auth_delkeys();
-
-       /*
-        * Now read lines from the file, looking for key entries
+        * Now read lines from the file, looking for key entries. Put
+        * the data into temporary store for later propagation to avoid
+        * two-pass processing.
         */
        nerr = 0;
        while ((line = fgets(buf, sizeof buf, fp)) != NULL) {
@@ -216,11 +224,16 @@ authreadkeys(
                                  "authreadkeys: no key for key %d", keyno);
                        continue;
                }
+               next = NULL;
                len = strlen(token);
                if (len <= 20) {        /* Bug 2537 */
-                       MD5auth_setkey(keyno, keytype, (u_char *)token, len);
+                       next = emalloc(sizeof(KeyDataT) + len);
+                       next->keyid   = keyno;
+                       next->keytype = keytype;
+                       next->seclen  = len;
+                       memcpy(next->secbuf, token, len);
                } else {
-                       char    hex[] = "0123456789abcdef";
+                       static const char hex[] = "0123456789abcdef";
                        u_char  temp;
                        char    *ptr;
                        size_t  jlim;
@@ -242,19 +255,52 @@ authreadkeys(
                                          keyno);
                                continue;
                        }
-                       MD5auth_setkey(keyno, keytype, keystr, jlim / 2);
+                       len = jlim/2; /* hmmmm.... what about odd length?!? */
+                       next = emalloc(sizeof(KeyDataT) + len);
+                       next->keyid   = keyno;
+                       next->keytype = keytype;
+                       next->seclen  = len;
+                       memcpy(next->secbuf, keystr, len);
+               }
+               if (next) {
+                       next->next = list;
+                       list = next;
                }
        }
        fclose(fp);
        if (nerr > nerr_maxlimit) {
                msyslog(LOG_ERR,
-                       "authreadkeys: emergency break after %u errors",
-                       nerr);
-               return (0);
-       } else if (nerr > nerr_loglimit) {
+                       "authreadkeys: rejecting file '%s' after %u errors (emergency break)",
+                       file, nerr);
+               goto onerror;
+       }
+       if (nerr > 0) {
                msyslog(LOG_ERR,
-                       "authreadkeys: found %u more error(s)",
-                       nerr - nerr_loglimit);
+                       "authreadkeys: rejecting file '%s' after %u error(s)",
+                       file, nerr);
+               goto onerror;
+       }
+
+       /* first remove old file-based keys */
+       auth_delkeys();
+       /* insert the new key material */
+       while (NULL != (next = list)) {
+               list = next->next;
+               MD5auth_setkey(next->keyid, next->keytype,
+                              next->secbuf, next->seclen);
+               /* purge secrets from memory before free()ing it */
+               memset(next, 0, sizeof(*next) + next->seclen);
+               free(next);
        }
        return (1);
+
+  onerror:
+       /* Mop up temporary storage before bailing out. */
+       while (NULL != (next = list)) {
+               list = next->next;
+               /* purge secrets from memory before free()ing it */
+               memset(next, 0, sizeof(*next) + next->seclen);
+               free(next);
+       }
+       return (0);
 }