From: Juergen Perlinger Date: Fri, 6 Nov 2015 07:33:23 +0000 (+0100) Subject: [Bug 2957] format specifies type 'unsigned int' but the argument has type 'size_t' X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9260289f68ad8edcf9eb3b1247376b28cbe46220;p=thirdparty%2Fntp.git [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 b170d63fd..0f9ac65a0 100644 --- 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 (4.2.8p4-RC1) 2015/10/06 Released by Harlan Stenn diff --git a/libntp/authkeys.c b/libntp/authkeys.c index 667ca298b..5bd696cc2 100644 --- 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 1c4c07ca5..5107dd096 100644 --- 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); }