From: Juergen Perlinger Date: Wed, 6 Sep 2017 08:16:25 +0000 (+0200) Subject: [Bug 3430] ntpq dumps core (SIGSEGV) for "keytype md2" X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=26a90fed8f0df7952ed2aff74fdd4ea8eecc1452;p=thirdparty%2Fntp.git [Bug 3430] ntpq dumps core (SIGSEGV) for "keytype md2" bk: 59afaed9xYmMX8dCb0S0K2pzrGkkCw --- diff --git a/ChangeLog b/ChangeLog index a1a1cfae4..d76fe2e84 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +--- +* [Bug 3430] ntpq dumps core (SIGSEGV) for "keytype md2" + - fixed several issues with hash algos in ntpd, sntp, ntpq, + ntpdc and the test suites + --- (4.2.8p10-win-beta1) 2017/03/21 Released by Harlan Stenn (4.2.8p10) diff --git a/include/ntp.h b/include/ntp.h index 323135da4..ca1131133 100644 --- a/include/ntp.h +++ b/include/ntp.h @@ -553,11 +553,13 @@ struct pkt { l_fp rec; /* receive time stamp */ l_fp xmt; /* transmit time stamp */ -#define MIN_V4_PKT_LEN (12 * sizeof(u_int32)) /* min header length */ -#define LEN_PKT_NOMAC (12 * sizeof(u_int32)) /* min header length */ -#define MIN_MAC_LEN (1 * sizeof(u_int32)) /* crypto_NAK */ -#define MAX_MD5_LEN (5 * sizeof(u_int32)) /* MD5 */ +#define MIN_V4_PKT_LEN (12 * sizeof(u_int32)) /* min header length */ +#define LEN_PKT_NOMAC (12 * sizeof(u_int32)) /* min header length */ +#define MIN_MAC_LEN (1 * sizeof(u_int32)) /* crypto_NAK */ +#define MAX_MD5_LEN (5 * sizeof(u_int32)) /* MD5 */ #define MAX_MAC_LEN (6 * sizeof(u_int32)) /* SHA */ +#define KEY_MAC_LEN sizeof(u_int32) /* key ID in MAC */ +#define MAX_MDG_LEN (MAX_MAC_LEN-KEY_MAC_LEN) /* max. digest len */ /* * The length of the packet less MAC must be a multiple of 64 diff --git a/include/ntp_request.h b/include/ntp_request.h index c750b7750..93a27e2ad 100644 --- a/include/ntp_request.h +++ b/include/ntp_request.h @@ -141,7 +141,7 @@ struct req_pkt { req_data_u u; /* data area */ l_fp tstamp; /* time stamp, for authentication */ keyid_t keyid; /* (optional) encryption key */ - char mac[MAX_MAC_LEN-sizeof(keyid_t)]; /* (optional) auth code */ + char mac[MAX_MDG_LEN]; /* (optional) auth code */ }; /* @@ -151,7 +151,7 @@ struct req_pkt { struct req_pkt_tail { l_fp tstamp; /* time stamp, for authentication */ keyid_t keyid; /* (optional) encryption key */ - char mac[MAX_MAC_LEN-sizeof(keyid_t)]; /* (optional) auth code */ + char mac[MAX_MDG_LEN]; /* (optional) auth code */ }; /* MODE_PRIVATE request packet header length before optional items. */ diff --git a/libntp/a_md5encrypt.c b/libntp/a_md5encrypt.c index 7394d0d27..eef9515f9 100644 --- a/libntp/a_md5encrypt.c +++ b/libntp/a_md5encrypt.c @@ -46,10 +46,10 @@ MD5authencrypt( EVP_DigestFinal(ctx, digest, &len); EVP_MD_CTX_free(ctx); /* If the MAC is longer than the MAX then truncate it. */ - if (len > MAX_MAC_LEN - 4) - len = MAX_MAC_LEN - 4; - memmove((u_char *)pkt + length + 4, digest, len); - return (len + 4); + if (len > MAX_MDG_LEN) + len = MAX_MDG_LEN; + memmove((u_char *)pkt + length + KEY_MAC_LEN, digest, len); + return (len + KEY_MAC_LEN); } @@ -89,14 +89,15 @@ MD5authdecrypt( EVP_DigestFinal(ctx, digest, &len); EVP_MD_CTX_free(ctx); /* If the MAC is longer than the MAX then truncate it. */ - if (len > MAX_MAC_LEN - 4) - len = MAX_MAC_LEN - 4; - if (size != (size_t)len + 4) { + if (len > MAX_MDG_LEN) + len = MAX_MDG_LEN; + if (size != (size_t)len + KEY_MAC_LEN) { msyslog(LOG_ERR, "MAC decrypt: MAC length error"); return (0); } - return !isc_tsmemcmp(digest, (u_char *)pkt + length + 4, len); + return !isc_tsmemcmp(digest, + (u_char *)pkt + length + KEY_MAC_LEN, len); } /* @@ -108,7 +109,7 @@ MD5authdecrypt( u_int32 addr2refid(sockaddr_u *addr) { - u_char digest[20]; + u_char digest[EVP_MAX_MD_SIZE]; u_int32 addr_refid; EVP_MD_CTX *ctx; u_int len; diff --git a/libntp/authreadkeys.c b/libntp/authreadkeys.c index e9273ad61..536b00482 100644 --- a/libntp/authreadkeys.c +++ b/libntp/authreadkeys.c @@ -218,13 +218,8 @@ authreadkeys( keytype = keytype_from_text(token, NULL); if (keytype == 0) { log_maybe(NULL, - "authreadkeys: invalid type for key %d", - keyno); - } else if (EVP_get_digestbynid(keytype) == NULL) { - log_maybe(NULL, - "authreadkeys: no algorithm for key %d", + "authreadkeys: invalid type/algorithm for key %d", keyno); - keytype = 0; } #else /* !OPENSSL follows */ /* diff --git a/libntp/ssl_init.c b/libntp/ssl_init.c index bebf6e175..4378263c0 100644 --- a/libntp/ssl_init.c +++ b/libntp/ssl_init.c @@ -63,7 +63,7 @@ void ssl_check_version(void) { u_long v; - + v = OpenSSL_version_num(); if ((v ^ OPENSSL_VERSION_NUMBER) & ~0xff0L) { msyslog(LOG_WARNING, @@ -88,65 +88,61 @@ ssl_check_version(void) */ int keytype_from_text( - const char *text, - size_t *pdigest_len + const char * text, + size_t * pdigest_len ) { int key_type; u_int digest_len; -#ifdef OPENSSL - const u_long max_digest_len = MAX_MAC_LEN - sizeof(keyid_t); - u_char digest[EVP_MAX_MD_SIZE]; + + /*----------------------------------------------------------- */ +#ifdef OPENSSL /* --*-- OpenSSL code --*-- */ + /*----------------------------------------------------------- */ + char * upcased; char * pch; + EVP_MD const * md; /* * OpenSSL digest short names are capitalized, so uppercase the * digest name before passing to OBJ_sn2nid(). If it is not - * recognized but begins with 'M' use NID_md5 to be consistent - * with past behavior. + * recognized but begins with 'M' or 'm' use NID_md5 to be + * consistent with past behavior. */ INIT_SSL(); + + /* get name in uppercase */ LIB_GETBUF(upcased); strlcpy(upcased, text, LIB_BUFLENGTH); - for (pch = upcased; '\0' != *pch; pch++) + for (pch = upcased; '\0' != *pch; ++pch) *pch = (char)toupper((unsigned char)*pch); - key_type = OBJ_sn2nid(upcased); -#else - key_type = 0; -#endif - if (!key_type && 'm' == tolower((unsigned char)text[0])) - key_type = NID_md5; + md = EVP_get_digestbyname(upcased); + if (NULL == md && 'M' == upcased[0]) + md = EVP_get_digestbyname("MD5"); + if (NULL == md) + return 0; + + key_type = EVP_MD_type(md); + digest_len = EVP_MD_size(md); - if (!key_type) + /*----------------------------------------------------------- */ +#else /* --*-- NON-SSL CODE --*-- */ + /*----------------------------------------------------------- */ + + if ('m' == tolower((unsigned char)text[0])) + key_type = NID_md5; + else return 0; - if (NULL != pdigest_len) { -#ifdef OPENSSL - EVP_MD_CTX *ctx; - - ctx = EVP_MD_CTX_new(); - EVP_DigestInit(ctx, EVP_get_digestbynid(key_type)); - EVP_DigestFinal(ctx, digest, &digest_len); - EVP_MD_CTX_free(ctx); - if (digest_len > max_digest_len) { - fprintf(stderr, - "key type %s %u octet digests are too big, max %lu\n", - keytype_name(key_type), digest_len, - max_digest_len); - msyslog(LOG_ERR, - "key type %s %u octet digests are too big, max %lu", - keytype_name(key_type), digest_len, - max_digest_len); - return 0; - } -#else - digest_len = 16; -#endif - *pdigest_len = digest_len; - } + digest_len = 16; + /*----------------------------------------------------------- */ +#endif /* --*-- NON-SSL CODE --*-- */ + /*----------------------------------------------------------- */ + + if (pdigest_len) + *pdigest_len = digest_len; return key_type; } diff --git a/ntpq/ntpq.c b/ntpq/ntpq.c index 547f3cc5d..f14a823cb 100644 --- a/ntpq/ntpq.c +++ b/ntpq/ntpq.c @@ -3596,64 +3596,55 @@ struct hstate { #define K_DELIM_STR ", " static void list_md_fn(const EVP_MD *m, const char *from, const char *to, void *arg ) { - size_t len, n; - const char *name, *cp, **seen; + size_t len, n, digest_len; + const char *name, **seen; struct hstate *hstate = arg; - EVP_MD_CTX *ctx; - u_int digest_len; - u_char digest[EVP_MAX_MD_SIZE]; if (!m) - return; /* Ignore aliases */ + return; /* Ignore aliases */ + /* There are duplicates. Discard if name has been seen. + * + * Names are capitalized in 'keytype_from_text()' in ssl_init.c; we + * have to make sure we do compare case-insensitive when checking + * for dupes... + */ name = EVP_MD_name(m); - - /* Lowercase names aren't accepted by keytype_from_text in ssl_init.c */ - - for( cp = name; *cp; cp++ ) { - if( islower((unsigned char)*cp) ) - return; - } - len = (cp - name) + 1; - - /* There are duplicates. Discard if name has been seen. */ - + len = strlen(name) + 1; for (seen = hstate->seen; *seen; seen++) - if (!strcmp(*seen, name)) + if (!strcasecmp(*seen, name)) + return; + + /* Discard MACs that NTP won't accept. + * Keep this consistent with keytype_from_text() in ssl_init.c, + * which is done most easily by using it... + */ + if (keytype_from_text(name, &digest_len) == 0) return; + n = (seen - hstate->seen) + 2; hstate->seen = erealloc(hstate->seen, n * sizeof(*seen)); hstate->seen[n-2] = name; hstate->seen[n-1] = NULL; - /* Discard MACs that NTP won't accept. - * Keep this consistent with keytype_from_text() in ssl_init.c. - */ - - ctx = EVP_MD_CTX_new(); - EVP_DigestInit(ctx, EVP_get_digestbyname(name)); - EVP_DigestFinal(ctx, digest, &digest_len); - EVP_MD_CTX_free(ctx); - if (digest_len > (MAX_MAC_LEN - sizeof(keyid_t))) - return; - if (hstate->list != NULL) - len += strlen(hstate->list); + len += strlen(hstate->list); len += (hstate->idx >= K_PER_LINE)? strlen(K_NL_PFX_STR): strlen(K_DELIM_STR); if (hstate->list == NULL) { - hstate->list = (char *)emalloc(len); - hstate->list[0] = '\0'; - } else - hstate->list = (char *)erealloc(hstate->list, len); - + hstate->list = (char *)emalloc(len); + hstate->list[0] = '\0'; + } else { + hstate->list = (char *)erealloc(hstate->list, len); + } + sprintf(hstate->list + strlen(hstate->list), "%s%s", ((hstate->idx >= K_PER_LINE)? K_NL_PFX_STR : K_DELIM_STR), name); if (hstate->idx >= K_PER_LINE) - hstate->idx = 1; + hstate->idx = 1; else - hstate->idx++; + hstate->idx++; } # endif #endif diff --git a/sntp/crypto.c b/sntp/crypto.c index e45b21360..25bdbf8db 100644 --- a/sntp/crypto.c +++ b/sntp/crypto.c @@ -17,24 +17,27 @@ make_mac( ) { u_int len = mac_size; - int key_type; EVP_MD_CTX * ctx; + u_char dbuf[EVP_MAX_MD_SIZE]; if (cmp_key->key_len > 64) return 0; if (pkt_size % 4 != 0) return 0; - INIT_SSL(); - key_type = keytype_from_text(cmp_key->type, NULL); - ctx = EVP_MD_CTX_new(); - EVP_DigestInit(ctx, EVP_get_digestbynid(key_type)); + EVP_DigestInit(ctx, EVP_get_digestbynid(cmp_key->typei)); EVP_DigestUpdate(ctx, (const u_char *)cmp_key->key_seq, (u_int)cmp_key->key_len); EVP_DigestUpdate(ctx, pkt_data, (u_int)pkt_size); - EVP_DigestFinal(ctx, digest, &len); + EVP_DigestFinal(ctx, dbuf, &len); EVP_MD_CTX_free(ctx); + /* if the digest is longer than permitted, truncate it. This is + * consistent with the behavior of NTPD/NTPQ/NTPDC... + */ + if (len > mac_size) + len = mac_size; + memcpy(digest, dbuf, len); return (int)len; } @@ -54,7 +57,7 @@ auth_md5( { int hash_len; int authentic; - char digest[20]; + char digest[MAX_MDG_LEN]; const u_char *pkt_ptr; if (mac_size > (int)sizeof(digest)) return 0; @@ -109,6 +112,8 @@ auth_init( char kbuf[200]; char keystring[129]; + INIT_SSL(); + if (keyf == NULL) { if (debug) printf("sntp auth_init: Couldn't open key file %s for reading!\n", keyfile); @@ -134,18 +139,17 @@ auth_init( if (octothorpe) *octothorpe = '\0'; act = emalloc(sizeof(*act)); - scan_cnt = sscanf(kbuf, "%d %9s %128s", &act->key_id, act->type, keystring); + scan_cnt = sscanf(kbuf, "%d %9s %128s", &act->key_id, act->typen, keystring); if (scan_cnt == 3) { int len = strlen(keystring); + goodline = 1; /* assume best for now */ if (len <= 20) { act->key_len = len; memcpy(act->key_seq, keystring, len + 1); - goodline = 1; } else if ((len & 1) != 0) { goodline = 0; /* it's bad */ } else { int j; - goodline = 1; act->key_len = len >> 1; for (j = 0; j < len; j+=2) { int val; @@ -158,6 +162,9 @@ auth_init( act->key_seq[j>>1] = (char)val; } } + act->typei = keytype_from_text(act->typen, NULL); + if (0 == act->typei) + goodline = 0; /* it's bad */ } if (goodline) { act->next = NULL; diff --git a/sntp/crypto.h b/sntp/crypto.h index 19cdbc45e..72a5a97de 100644 --- a/sntp/crypto.h +++ b/sntp/crypto.h @@ -20,7 +20,8 @@ struct key { struct key * next; int key_id; int key_len; - char type[10]; + int typei; + char typen[10]; char key_seq[64]; }; diff --git a/sntp/main.c b/sntp/main.c index 78ed7c270..b33d36454 100644 --- a/sntp/main.c +++ b/sntp/main.c @@ -1134,11 +1134,10 @@ generate_pkt ( set_li_vn_mode(x_pkt, LEAP_NOTINSYNC, ntpver, 3); if (pkt_key != NULL) { x_pkt->exten[0] = htonl(key_id); - mac_size = 20; /* max room for MAC */ - mac_size = make_mac(x_pkt, pkt_len, mac_size, + mac_size = make_mac(x_pkt, pkt_len, MAX_MDG_LEN, pkt_key, (char *)&x_pkt->exten[1]); if (mac_size > 0) - pkt_len += mac_size + 4; + pkt_len += mac_size + KEY_MAC_LEN; } return pkt_len; } diff --git a/sntp/tests/crypto.c b/sntp/tests/crypto.c index fb2dc6298..d2dbd208c 100644 --- a/sntp/tests/crypto.c +++ b/sntp/tests/crypto.c @@ -31,8 +31,9 @@ test_MakeMd5Mac(void) md5.key_id = 10; md5.key_len = 6; memcpy(&md5.key_seq, "md5seq", md5.key_len); - memcpy(&md5.type, "MD5", 4); - + strlcpy(md5.typen, "MD5", sizeof(md5.typen)); + md5.typei = keytype_from_text(md5.typen, NULL); + TEST_ASSERT_EQUAL(MD5_LENGTH, make_mac(PKT_DATA, PKT_LEN, MD5_LENGTH, &md5, actual)); @@ -57,7 +58,8 @@ test_MakeSHA1Mac(void) sha1.key_id = 20; sha1.key_len = 7; memcpy(&sha1.key_seq, "sha1seq", sha1.key_len); - memcpy(&sha1.type, "SHA1", 5); + strlcpy(sha1.typen, "SHA1", sizeof(sha1.typen)); + sha1.typei = keytype_from_text(sha1.typen, NULL); TEST_ASSERT_EQUAL(SHA1_LENGTH, make_mac(PKT_DATA, PKT_LEN, SHA1_LENGTH, &sha1, actual)); @@ -87,7 +89,8 @@ test_VerifyCorrectMD5(void) md5.key_id = 0; md5.key_len = 6; memcpy(&md5.key_seq, "md5key", md5.key_len); - memcpy(&md5.type, "MD5", 4); + strlcpy(md5.typen, "MD5", sizeof(md5.typen)); + md5.typei = keytype_from_text(md5.typen, NULL); TEST_ASSERT_TRUE(auth_md5(PKT_DATA, PKT_LEN, MD5_LENGTH, &md5)); } @@ -110,7 +113,8 @@ test_VerifySHA1(void) sha1.key_id = 0; sha1.key_len = 7; memcpy(&sha1.key_seq, "sha1key", sha1.key_len); - memcpy(&sha1.type, "SHA1", 5); + strlcpy(sha1.typen, "SHA1", sizeof(sha1.typen)); + sha1.typei = keytype_from_text(sha1.typen, NULL); TEST_ASSERT_TRUE(auth_md5(PKT_DATA, PKT_LEN, SHA1_LENGTH, &sha1)); @@ -139,7 +143,8 @@ test_VerifyFailure(void) md5.key_id = 0; md5.key_len = 6; memcpy(&md5.key_seq, "md5key", md5.key_len); - memcpy(&md5.type, "MD5", 4); + strlcpy(md5.typen, "MD5", sizeof(md5.typen)); + md5.typei = keytype_from_text(md5.typen, NULL); TEST_ASSERT_FALSE(auth_md5(PKT_DATA, PKT_LEN, MD5_LENGTH, &md5)); } @@ -157,7 +162,8 @@ test_PacketSizeNotMultipleOfFourBytes(void) md5.key_id = 10; md5.key_len = 6; memcpy(&md5.key_seq, "md5seq", md5.key_len); - memcpy(&md5.type, "MD5", 4); + strlcpy(md5.typen, "MD5", sizeof(md5.typen)); + md5.typei = keytype_from_text(md5.typen, NULL); TEST_ASSERT_EQUAL(0, make_mac(PKT_DATA, PKT_LEN, MD5_LENGTH, &md5, actual)); } diff --git a/sntp/tests/keyFile.c b/sntp/tests/keyFile.c index 395ca0dc1..af5acc703 100644 --- a/sntp/tests/keyFile.c +++ b/sntp/tests/keyFile.c @@ -32,9 +32,9 @@ CompareKeys( expected.key_len, actual.key_len); return FALSE; } - if (strcmp(expected.type, actual.type) != 0) { + if (strcmp(expected.typen, actual.typen) != 0) { printf("Expected key_type: %s but was: %s\n", - expected.type, actual.type); + expected.typen, actual.typen); return FALSE; } @@ -59,7 +59,7 @@ CompareKeysAlternative( temp.key_id = key_id; temp.key_len = key_len; - strlcpy(temp.type, type, sizeof(temp.type)); + strlcpy(temp.typen, type, sizeof(temp.typen)); memcpy(temp.key_seq, key_seq, key_len); return CompareKeys(temp, actual); diff --git a/sntp/tests/packetHandling.c b/sntp/tests/packetHandling.c index 595efa3b1..6787eeaa2 100644 --- a/sntp/tests/packetHandling.c +++ b/sntp/tests/packetHandling.c @@ -84,7 +84,8 @@ test_GenerateAuthenticatedPacket(void) testkey.key_id = 30; testkey.key_len = 9; memcpy(testkey.key_seq, "123456789", testkey.key_len); - memcpy(testkey.type, "MD5", 3); + strlcpy(testkey.typen, "MD5", sizeof(testkey.typen)); + testkey.typei = keytype_from_text(testkey.typen, NULL); GETTIMEOFDAY(&xmt, NULL); xmt.tv_sec += JAN_1970; @@ -106,7 +107,7 @@ test_GenerateAuthenticatedPacket(void) TEST_ASSERT_EQUAL(testkey.key_id, ntohl(testpkt.exten[0])); TEST_ASSERT_EQUAL(MAX_MD5_LEN - 4, /* Remove the key_id, only keep the mac. */ - make_mac(&testpkt, LEN_PKT_NOMAC, MAX_MD5_LEN, &testkey, expected_mac)); + make_mac(&testpkt, LEN_PKT_NOMAC, MAX_MD5_LEN-4, &testkey, expected_mac)); TEST_ASSERT_EQUAL_MEMORY(expected_mac, (char*)&testpkt.exten[1], MAX_MD5_LEN -4); } diff --git a/sntp/tests/packetProcessing.c b/sntp/tests/packetProcessing.c index 660b5b6e2..8437af929 100644 --- a/sntp/tests/packetProcessing.c +++ b/sntp/tests/packetProcessing.c @@ -76,7 +76,8 @@ PrepareAuthenticationTest( key_ptr->next = NULL; key_ptr->key_id = key_id; key_ptr->key_len = key_len; - memcpy(key_ptr->type, "MD5", 3); + strlcpy(key_ptr->typen, "MD5", sizeof(key_ptr->typen)); + key_ptr->typei = keytype_from_text(key_ptr->typen, NULL); TEST_ASSERT_TRUE(key_len < sizeof(key_ptr->key_seq)); @@ -231,7 +232,7 @@ test_AuthenticatedPacketInvalid(void) testpkt.p.exten[0] = htonl(50); int mac_len = make_mac(&testpkt.p, pkt_len, - MAX_MD5_LEN, key_ptr, + MAX_MD5_LEN - KEY_MAC_LEN, key_ptr, &testpkt.p.exten[1]); pkt_len += 4 + mac_len; @@ -259,9 +260,9 @@ test_AuthenticatedPacketUnknownKey(void) testpkt.p.exten[0] = htonl(50); int mac_len = make_mac(&testpkt.p, pkt_len, - MAX_MD5_LEN, key_ptr, + MAX_MD5_LEN - KEY_MAC_LEN, key_ptr, &testpkt.p.exten[1]); - pkt_len += 4 + mac_len; + pkt_len += KEY_MAC_LEN + mac_len; TEST_ASSERT_EQUAL(SERVER_AUTH_FAIL, process_pkt(&testpkt.p, &testsock, pkt_len, @@ -424,10 +425,10 @@ test_CorrectAuthenticatedPacketMD5(void) /* Prepare the packet. */ testpkt.p.exten[0] = htonl(10); int mac_len = make_mac(&testpkt.p, pkt_len, - MAX_MD5_LEN, key_ptr, + MAX_MD5_LEN - KEY_MAC_LEN, key_ptr, &testpkt.p.exten[1]); - pkt_len += 4 + mac_len; + pkt_len += KEY_MAC_LEN + mac_len; TEST_ASSERT_EQUAL(pkt_len, process_pkt(&testpkt.p, &testsock, pkt_len, @@ -446,12 +447,13 @@ test_CorrectAuthenticatedPacketSHA1(void) /* Prepare the packet. */ testpkt.p.exten[0] = htonl(20); int mac_len = make_mac(&testpkt.p, pkt_len, - MAX_MAC_LEN, key_ptr, + MAX_MDG_LEN, key_ptr, &testpkt.p.exten[1]); - pkt_len += 4 + mac_len; + pkt_len += KEY_MAC_LEN + mac_len; TEST_ASSERT_EQUAL(pkt_len, process_pkt(&testpkt.p, &testsock, pkt_len, MODE_SERVER, &testspkt.p, "UnitTest")); } +