From: Juergen Perlinger Date: Mon, 29 Feb 2016 19:03:59 +0000 (+0100) Subject: [Bug 3022] authkeys.c should be refactored X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=06a4ac8aa79ee97a7dd10d073d88241fac8574b5;p=thirdparty%2Fntp.git [Bug 3022] authkeys.c should be refactored bk: 56d4961fuochj5Wnd_Oj59rOmxcfyA --- diff --git a/ChangeLog b/ChangeLog index c70fe8fc5..5592ce449 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,10 @@ * [Bug 2994] Systems with HAVE_SIGNALED_IO fail to compile. perlinger@ntp.org * [Bug 2995] Fixes to compile on Windows +* [Bug 3022] authkeys.c should be refactored. perlinger@ntp.org + - fixed memory leak in access list (auth[read]keys.c) + - refactored handling of key access lists (auth[read]keys.c) + - reduced number of error branches (authreadkeys.c) --- (4.2.8p6) 2016/01/20 Released by Harlan Stenn diff --git a/include/ntp_keyacc.h b/include/ntp_keyacc.h index 730c310ac..7e6650431 100644 --- a/include/ntp_keyacc.h +++ b/include/ntp_keyacc.h @@ -10,4 +10,10 @@ struct keyaccess { sockaddr_u addr; }; +extern KeyAccT* keyacc_new_push(KeyAccT *head, const sockaddr_u *addr); +extern KeyAccT* keyacc_pop_free(KeyAccT *head); +extern KeyAccT* keyacc_all_free(KeyAccT *head); +extern int keyacc_contains(const KeyAccT *head, const sockaddr_u *addr, + int res_on_empty_list); + #endif /* NTP_KEYACC_H */ diff --git a/libntp/authkeys.c b/libntp/authkeys.c index 93f1747d1..51337d5c4 100644 --- a/libntp/authkeys.c +++ b/libntp/authkeys.c @@ -54,7 +54,7 @@ static u_short auth_log2(size_t); static void auth_resize_hashtable(void); static void allocsymkey(keyid_t, u_short, u_short, u_long, size_t, u_char *, KeyAccT *); -static void freesymkey(symkey *, keyid_t); +static void freesymkey(symkey *); #ifdef DEBUG static void free_auth_mem(void); #endif @@ -75,10 +75,10 @@ symkey **key_hash; u_long authkeynotfound; /* keys not found */ u_long authkeylookups; /* calls to lookup keys */ -u_long authnumkeys; /* number of active keys */ +u_long authnumkeys; /* number of active keys */ u_long authkeyexpired; /* key lifetime expirations */ u_long authkeyuncached; /* cache misses */ -u_long authnokey; /* calls to encrypt with no key */ +u_long authnokey; /* calls to encrypt with no key */ u_long authencryptions; /* calls to encrypt */ u_long authdecryptions; /* calls to decrypt */ @@ -93,14 +93,87 @@ int authnumfreekeys; /* * The key cache. We cache the last key we looked at here. + * Note: this should hold the last *trusted* key. Also the + * cache is only loaded when the digest type / MAC algorithm + * is valid. */ keyid_t cache_keyid; /* key identifier */ u_char *cache_secret; /* secret */ size_t cache_secretsize; /* secret length */ -int cache_type; /* OpenSSL digest NID */ +int cache_type; /* OpenSSL digest NID */ u_short cache_flags; /* flags that wave */ KeyAccT *cache_keyacclist; /* key access list */ +/* -------------------------------------------------------------------- + * manage key access lists + * -------------------------------------------------------------------- + */ +/* allocate and populate new access node and pushes it on the list. + * Returns the new head. + */ +KeyAccT* +keyacc_new_push( + KeyAccT * head, + const sockaddr_u * addr + ) +{ + KeyAccT * node = emalloc(sizeof(KeyAccT)); + + memcpy(&node->addr, addr, sizeof(sockaddr_u)); + node->next = head; + return node; +} + +/* ----------------------------------------------------------------- */ +/* pop and deallocate the first node of a list of access nodes, if + * the list is not empty. Returns the tail of the list. + */ +KeyAccT* +keyacc_pop_free( + KeyAccT *head + ) +{ + KeyAccT * next = NULL; + if (head) { + next = head->next; + free(head); + } + return next; +} + +/* ----------------------------------------------------------------- */ +/* deallocate the list; returns an empty list. */ +KeyAccT* +keyacc_all_free( + KeyAccT * head + ) +{ + while (head) + head = keyacc_pop_free(head); + return head; +} + +/* ----------------------------------------------------------------- */ +/* scan a list to see if it contains a given address. Return the + * default result value in case of an empty list. + */ +int /*BOOL*/ +keyacc_contains( + const KeyAccT *head, + const sockaddr_u *addr, + int defv) +{ + if (head) { + do { + if (SOCK_EQ(&head->addr, addr)) + return TRUE; + } while (NULL != (head = head->next)); + return FALSE; + } else { + return !!defv; + } +} + /* * init_auth - initialize internal data @@ -139,7 +212,7 @@ free_auth_mem(void) symkey_alloc * next_alloc; while (NULL != (sk = HEAD_DLIST(key_listhead, llink))) { - freesymkey(sk, sk->keyid); + freesymkey(sk); } free(key_hash); key_hash = NULL; @@ -243,6 +316,21 @@ auth_log2(size_t x) return (u_short)r; } +static void +authcache_flush_id( + keyid_t id + ) +{ + if (cache_keyid == id) { + cache_keyid = 0; + cache_type = 0; + cache_flags = 0; + cache_secret = NULL; + cache_secretsize = 0; + cache_keyacclist = NULL; + } +} + /* * auth_resize_hashtable @@ -326,13 +414,19 @@ allocsymkey( */ static void freesymkey( - symkey * sk, - keyid_t id + symkey * sk ) { -symkey ** bucket = &key_hash[KEYHASH(id)]; -symkey * unlinked; + symkey ** bucket; + symkey * unlinked; + + if (NULL == sk) + return; + authcache_flush_id(sk->keyid); + keyacc_all_free(sk->keyacclist); + + bucket = &key_hash[KEYHASH(sk->keyid)]; if (sk->secret != NULL) { memset(sk->secret, '\0', sk->secretsize); free(sk->secret); @@ -358,35 +452,26 @@ auth_findkey( { symkey * sk; - for (sk = key_hash[KEYHASH(id)]; sk != NULL; sk = sk->hlink) { - if (id == sk->keyid) { + for (sk = key_hash[KEYHASH(id)]; sk != NULL; sk = sk->hlink) + if (id == sk->keyid) return sk; - } - } - return NULL; } /* - * auth_havekey - return TRUE if the key id is zero or known + * auth_havekey - return TRUE if the key id is zero or known. The + * key needs not to be trusted. */ int auth_havekey( keyid_t id ) { - symkey * sk; - - if (0 == id || cache_keyid == id) { - return TRUE; - } - - sk = auth_findkey(id); - if (sk != NULL && id == sk->keyid) { - return TRUE; - } - return FALSE; + return + (0 == id) || + (cache_keyid == id) || + (NULL != auth_findkey(id)); } @@ -402,33 +487,25 @@ authhavekey( symkey * sk; authkeylookups++; - if (0 == id || cache_keyid == id) { - return TRUE; - } + if (0 == id || cache_keyid == id) + return !!(KEY_TRUSTED & cache_flags); /* - * Seach the bin for the key. If found and the key type - * is zero, somebody marked it trusted without specifying - * a key or key type. In this case consider the key missing. + * Search the bin for the key. If not found, or found but the key + * type is zero, somebody marked it trusted without specifying a + * key or key type. In this case consider the key missing. */ authkeyuncached++; sk = auth_findkey(id); - if (sk != NULL && id == sk->keyid) { - if (sk->type == 0) { - authkeynotfound++; - return FALSE; - } + if ((sk == NULL) || (sk->type == 0)) { + authkeynotfound++; + return FALSE; } /* - * If the key is not found, or if it is found but not trusted, - * the key is not considered found. + * If the key is not trusted, the key is not considered found. */ - if (NULL == sk) { - authkeynotfound++; - return FALSE; - } - if (!(KEY_TRUSTED & sk->flags)) { + if ( ! (KEY_TRUSTED & sk->flags)) { authnokey++; return FALSE; } @@ -474,27 +551,22 @@ authtrust( * not to be trusted. */ if (sk != NULL) { - if (cache_keyid == id) { - cache_flags = 0; - cache_keyid = 0; - cache_keyacclist = NULL; - } - /* - * Key exists. If it is to be trusted, say so and - * update its lifetime. + * Key exists. If it is to be trusted, say so and update + * its lifetime. If no longer trusted, return it to the + * free list. Flush the cache first to be sure there are + * no discrepancies. */ + authcache_flush_id(id); if (trust > 0) { sk->flags |= KEY_TRUSTED; if (trust > 1) sk->lifetime = current_time + trust; else sk->lifetime = 0; - return; + } else { + freesymkey(sk); } - - /* No longer trusted, return it to the free list. */ - freesymkey(sk, id); return; } @@ -544,33 +616,21 @@ authistrusted( ) { symkey * sk; - KeyAccT * kal; - KeyAccT * k; + /* That specific key was already used to authenticate the + * packet. Therefore, the key *must* exist... There's a chance + * that is not trusted, though. + */ if (keyno == cache_keyid) { - kal = cache_keyacclist; + return (KEY_TRUSTED & cache_flags) && + keyacc_contains(cache_keyacclist, sau, TRUE); } else { authkeyuncached++; - sk = auth_findkey(keyno); - if (NULL == sk || !(KEY_TRUSTED & sk->flags)) { - INSIST(!"authistrustedip: keyid not found/trusted!"); - return FALSE; - } - kal = sk->keyacclist; + INSIST(NULL != sk); + return (KEY_TRUSTED & sk->flags) && + keyacc_contains(sk->keyacclist, sau, TRUE); } - - if (NULL == kal) { - return TRUE; - } - - for (k = kal; k; k = k->next) { - if (SOCK_EQ(&k->addr, sau)) { - return TRUE; - } - } - - return FALSE; } /* Note: There are two locations below where 'strncpy()' is used. While @@ -612,7 +672,11 @@ MD5auth_setkey( sk->secret = emalloc(secretsize + 1); sk->type = (u_short)keytype; sk->secretsize = secretsize; - sk->keyacclist = ka; + /* make sure access lists don't leak here! */ + if (ka != sk->keyacclist) { + keyacc_all_free(sk->keyacclist); + sk->keyacclist = ka; + } #ifndef DISABLE_BUG1243_FIX memcpy(sk->secret, key, secretsize); #else @@ -620,11 +684,7 @@ MD5auth_setkey( strncpy((char *)sk->secret, (const char *)key, secretsize); #endif - if (cache_keyid == keyno) { - cache_flags = 0; - cache_keyid = 0; - cache_keyacclist = NULL; - } + authcache_flush_id(keyno); return; } @@ -680,10 +740,11 @@ auth_delkeys(void) free(sk->secret); sk->secret = NULL; /* TALOS-CAN-0054 */ } + sk->keyacclist = keyacc_all_free(sk->keyacclist); sk->secretsize = 0; sk->lifetime = 0; } else { - freesymkey(sk, sk->keyid); + freesymkey(sk); } ITER_DLIST_END() } @@ -699,7 +760,7 @@ auth_agekeys(void) ITER_DLIST_BEGIN(key_listhead, sk, llink, symkey) if (sk->lifetime > 0 && current_time > sk->lifetime) { - freesymkey(sk, sk->keyid); + freesymkey(sk); authkeyexpired++; } ITER_DLIST_END() diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c index 1d4ee3059..8c0675093 100644 --- a/libntp/authreadkeys.c +++ b/libntp/authreadkeys.c @@ -101,13 +101,26 @@ 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 + ) +{ + if (node) { + keyacc_all_free(node->keyacclist); + /* 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 +169,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 +193,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 +210,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 */ /* @@ -269,26 +288,22 @@ authreadkeys( } token = nexttok(&line); -DPRINTF(0, ("authreadkeys: full access list <%s>\n", (token) ? token : "NULL")); + DPRINTF(0, ("authreadkeys: full access list <%s>\n", (token) ? token : "NULL")); if (token != NULL) { /* A comma-separated IP access list */ char *tp = token; while (tp) { char *i; - KeyAccT ka; + sockaddr_u addr; i = strchr(tp, (int)','); if (i) *i = '\0'; -DPRINTF(0, ("authreadkeys: access list: <%s>\n", tp)); - - if (is_ip_address(tp, AF_UNSPEC, &ka.addr)) { - KeyAccT *kap; + DPRINTF(0, ("authreadkeys: access list: <%s>\n", tp)); - kap = emalloc(sizeof(KeyAccT)); - memcpy(kap, &ka, sizeof ka); - kap->next = next->keyacclist; - next->keyacclist = kap; + if (is_ip_address(tp, AF_UNSPEC, &addr)) { + next->keyacclist = keyacc_new_push( + next->keyacclist, &addr); } else { log_maybe(&nerr, "authreadkeys: invalid IP address <%s> for key %d", @@ -303,21 +318,25 @@ 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; } fclose(fp); - if (nerr > nerr_maxlimit) { - msyslog(LOG_ERR, - "authreadkeys: rejecting file '%s' after %u errors (emergency break)", - file, nerr); - goto onerror; - } if (nerr > 0) { + const char * why = ""; + if (nerr > nerr_maxlimit) + why = " (emergency break)"; msyslog(LOG_ERR, - "authreadkeys: rejecting file '%s' after %u error(s)", - file, nerr); + "authreadkeys: rejecting file '%s' after %u error(s)%s", + file, nerr, why); goto onerror; } @@ -328,9 +347,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 +356,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); }