From afae1b65e84f3ed4da98c28b6539fec747edda4e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 21 May 2023 12:59:38 -0700 Subject: [PATCH] prevent TSIG keys from being added to multiple rings it was possible to add a TSIG key to more than one TSIG keyring at a time, and this was in fact happening with the session key, which was generated once and then added to the keyrings for each view as it was configured. this has been corrected and a REQUIRE added to dns_tsigkeyring_add() to prevent it from happening again. --- bin/named/include/named/server.h | 2 +- bin/named/server.c | 65 ++++++++++++++++++++------------ lib/dns/include/dns/tsig.h | 4 +- lib/dns/tsig.c | 26 +++++++++---- 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/bin/named/include/named/server.h b/bin/named/include/named/server.h index 1422e0241b..075e2ec152 100644 --- a/bin/named/include/named/server.h +++ b/bin/named/include/named/server.h @@ -100,7 +100,7 @@ struct named_server { named_statschannellist_t statschannels; - dns_tsigkey_t *sessionkey; + dst_key_t *sessionkey; char *session_keyfile; dns_name_t *session_keyname; unsigned int session_keyalg; diff --git a/bin/named/server.c b/bin/named/server.c index a26d22fc6d..2f21fc5213 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4044,6 +4044,28 @@ minimal_cache_allowed(const cfg_obj_t *maps[4], static const char *const response_synonyms[] = { "response", NULL }; +static const dns_name_t * +algorithm_name(unsigned int alg) { + switch (alg) { + case DST_ALG_HMACMD5: + return (dns_tsig_hmacmd5_name); + case DST_ALG_HMACSHA1: + return (dns_tsig_hmacsha1_name); + case DST_ALG_HMACSHA224: + return (dns_tsig_hmacsha224_name); + case DST_ALG_HMACSHA256: + return (dns_tsig_hmacsha256_name); + case DST_ALG_HMACSHA384: + return (dns_tsig_hmacsha384_name); + case DST_ALG_HMACSHA512: + return (dns_tsig_hmacsha512_name); + case DST_ALG_GSSAPI: + return (dns_tsig_gssapi_name); + default: + UNREACHABLE(); + } +} + /* * Configure 'view' according to 'vconfig', taking defaults from * 'config' where values are missing in 'vconfig'. @@ -5076,8 +5098,18 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, */ CHECK(named_tsigkeyring_fromconfig(config, vconfig, view->mctx, &ring)); if (named_g_server->sessionkey != NULL) { - CHECK(dns_tsigkeyring_add(ring, named_g_server->session_keyname, - named_g_server->sessionkey)); + dns_tsigkey_t *tsigkey = NULL; + result = dns_tsigkey_createfromkey( + named_g_server->session_keyname, + algorithm_name(named_g_server->session_keyalg), + named_g_server->sessionkey, false, NULL, 0, 0, mctx, + NULL, &tsigkey); + if (result == ISC_R_SUCCESS) { + result = dns_tsigkeyring_add( + ring, named_g_server->session_keyname, tsigkey); + dns_tsigkey_detach(&tsigkey); + } + CHECK(result); } dns_view_setkeyring(view, ring); dns_tsigkeyring_detach(&ring); @@ -7535,7 +7567,7 @@ cleanup_session_key(named_server_t *server, isc_mem_t *mctx) { } if (server->sessionkey != NULL) { - dns_tsigkey_detach(&server->sessionkey); + dst_key_free(&server->sessionkey); } server->session_keyalg = DST_ALG_UNKNOWN; @@ -7545,9 +7577,8 @@ cleanup_session_key(named_server_t *server, isc_mem_t *mctx) { static isc_result_t generate_session_key(const char *filename, const char *keynamestr, const dns_name_t *keyname, const char *algstr, - const dns_name_t *algname, unsigned int algtype, - uint16_t bits, isc_mem_t *mctx, bool first_time, - dns_tsigkey_t **tsigkeyp) { + unsigned int algtype, uint16_t bits, isc_mem_t *mctx, + bool first_time, dst_key_t **keyp) { isc_result_t result = ISC_R_SUCCESS; dst_key_t *key = NULL; isc_buffer_t key_txtbuffer; @@ -7555,8 +7586,6 @@ generate_session_key(const char *filename, const char *keynamestr, char key_txtsecret[256]; char key_rawsecret[64]; isc_region_t key_rawregion; - isc_stdtime_t now; - dns_tsigkey_t *tsigkey = NULL; FILE *fp = NULL; isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, @@ -7572,8 +7601,7 @@ generate_session_key(const char *filename, const char *keynamestr, } /* - * Dump the key to the buffer for later use. Should be done before - * we transfer the ownership of key to tsigkey. + * Dump the key to the buffer for later use. */ isc_buffer_init(&key_rawbuffer, &key_rawsecret, sizeof(key_rawsecret)); CHECK(dst_key_tobuffer(key, &key_rawbuffer)); @@ -7582,11 +7610,6 @@ generate_session_key(const char *filename, const char *keynamestr, isc_buffer_init(&key_txtbuffer, &key_txtsecret, sizeof(key_txtsecret)); CHECK(isc_base64_totext(&key_rawregion, -1, "", &key_txtbuffer)); - /* Store the key in tsigkey. */ - isc_stdtime_get(&now); - CHECK(dns_tsigkey_createfromkey(dst_key_name(key), algname, key, false, - NULL, now, now, mctx, NULL, &tsigkey)); - /* Dump the key to the key file. */ fp = named_os_openfile(filename, S_IRUSR | S_IWUSR, first_time); if (fp == NULL) { @@ -7611,10 +7634,7 @@ generate_session_key(const char *filename, const char *keynamestr, goto cleanup; } - dst_key_free(&key); - - *tsigkeyp = tsigkey; - + *keyp = key; return (ISC_R_SUCCESS); cleanup: @@ -7627,9 +7647,6 @@ cleanup: (void)isc_stdio_close(fp); (void)isc_file_remove(filename); } - if (tsigkey != NULL) { - dns_tsigkey_detach(&tsigkey); - } if (key != NULL) { dst_key_free(&key); } @@ -7734,8 +7751,8 @@ configure_session_key(const cfg_obj_t **maps, named_server_t *server, server->session_keybits = bits; CHECK(generate_session_key(keyfile, keynamestr, keyname, algstr, - algname, algtype, bits, mctx, - first_time, &server->sessionkey)); + algtype, bits, mctx, first_time, + &server->sessionkey)); } return (result); diff --git a/lib/dns/include/dns/tsig.h b/lib/dns/include/dns/tsig.h index 48292a1da4..c66fc399db 100644 --- a/lib/dns/include/dns/tsig.h +++ b/lib/dns/include/dns/tsig.h @@ -264,7 +264,9 @@ dns_tsigkeyring_add(dns_tsig_keyring_t *ring, const dns_name_t *name, * Place a TSIG key onto a key ring. * * Requires: - *\li 'ring', 'name' and 'tkey' are not NULL + *\li 'name' and 'ring' are not NULL + *\li 'tkey' is a valid TSIG key, which has not been + * added to any other keyrings * * Returns: *\li #ISC_R_SUCCESS diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index b70099a629..857ec4cfcd 100644 --- a/lib/dns/tsig.c +++ b/lib/dns/tsig.c @@ -223,16 +223,22 @@ keyring_add(dns_tsig_keyring_t *ring, const dns_name_t *name, } result = dns_rbt_addname(ring->keys, name, tkey); - if (result == ISC_R_SUCCESS && tkey->generated) { - /* - * Add the new key to the LRU list and remove the least - * recently used key if there are too many keys on the list. - */ - ISC_LIST_APPEND(ring->lru, tkey, link); - if (ring->generated++ > ring->maxgenerated) { - remove_fromring(ISC_LIST_HEAD(ring->lru)); + if (result == ISC_R_SUCCESS) { + if (tkey->generated) { + /* + * Add the new key to the LRU list and remove the + * least recently used key if there are too many + * keys on the list. + */ + ISC_LIST_APPEND(ring->lru, tkey, link); + if (ring->generated++ > ring->maxgenerated) { + remove_fromring(ISC_LIST_HEAD(ring->lru)); + } } + + tkey->ring = ring; } + RWUNLOCK(&ring->lock, isc_rwlocktype_write); return (result); @@ -1857,6 +1863,10 @@ dns_tsigkeyring_add(dns_tsig_keyring_t *ring, const dns_name_t *name, dns_tsigkey_t *tkey) { isc_result_t result; + REQUIRE(VALID_TSIG_KEY(tkey)); + REQUIRE(tkey->ring == NULL); + REQUIRE(name != NULL); + result = keyring_add(ring, name, tkey); if (result == ISC_R_SUCCESS) { isc_refcount_increment(&tkey->refs);