From: TCY16 Date: Wed, 20 Jul 2022 13:36:58 +0000 (+0300) Subject: implement @wcawijngaards' review comments X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b5acecc1f6df563e96594db7a0c9e472012b7f85;p=thirdparty%2Funbound.git implement @wcawijngaards' review comments --- diff --git a/services/cache/infra.c b/services/cache/infra.c index beabca6a7..e1160301c 100644 --- a/services/cache/infra.c +++ b/services/cache/infra.c @@ -393,8 +393,7 @@ data_entry_init(struct infra_cache* infra, struct lruhash_entry* e, uint8_t cookie[8] = {0,0,0,0,0,0,0,0}; for (i = 0; i < 8; i++) { - cookie[i] = ub_random_max(infra->random_state, - 255); // @TODO is 255 correct? macro-ify it? + cookie[i] = ub_random_max(infra->random_state, 256); } data = (struct infra_data*)e->data; @@ -405,7 +404,7 @@ data_entry_init(struct infra_cache* infra, struct lruhash_entry* e, data->probedelay = 0; /* set EDNS cookie to zero, as this also sets the starting state*/ memset(&data->cookie, 0, sizeof(struct edns_cookie)); - memcpy(data->cookie.data.components.client, cookie, 8); + memcpy(data->cookie.data.cookie, cookie, 8); data->isdnsseclame = 0; data->rec_lame = 0; data->lame_type_A = 0; @@ -706,10 +705,10 @@ infra_edns_update(struct infra_cache* infra, struct sockaddr_storage* addr, return 1; } -struct edns_cookie* +int infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, socklen_t addrlen, uint8_t* name, size_t namelen, - time_t timenow) + time_t timenow, struct edns_cookie* cookie) { struct lruhash_entry* e = infra_lookup_nottl(infra, addr, addrlen, name, namelen, 1); @@ -718,29 +717,30 @@ infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, if(!e) { if(!(e = new_entry(infra, addr, addrlen, name, namelen, timenow))) { - return NULL; + return 0; } needtoinsert = 1; } else if(((struct infra_data*)e->data)->ttl < timenow) { /* EDNS cookies have their own timeout logic controlled by the * upstream, so we just copy the cookie from the old cache entry */ struct edns_cookie c = ((struct infra_data*)e->data)->cookie; - /* @TODO create logic for saving server cookie? -> not sure, find out */ - /* create new cookie if the cache TTL expired */ + /* create new cookie if the cache TTL expired, keep the cookie */ data_entry_init(infra, e, timenow); ((struct infra_data*)e->data)->cookie = c; } data = (struct infra_data*) e->data; + memcpy(cookie, &data->cookie, sizeof(struct edns_cookie)); + if(needtoinsert) { slabhash_insert(infra->hosts, e->hash, e, e->data, NULL); } else { lock_rw_unlock(&e->lock); } - return &data->cookie; + return 1; } int @@ -755,8 +755,13 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr assert(cookie->opt_len == 24); /* the client cookie was set on the outgoing upstream, so the entry - * must exists here */ - assert(e); + * should exists here. This can be false if the cookie has fallen + * out of cache */ + if (!(e)) { + /* No need to insert a new cookie/entry here, this will be + * done with an outgoing request */ + return 0; + } data = (struct infra_data*) e->data; @@ -764,25 +769,25 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr /* we known this upstream doesn't support cookies; the state * remains unchanged */ lock_rw_unlock(&e->lock); - return 0; + return 1; } else if (data->cookie.state == SERVER_COOKIE_LEARNED) { /* wrong client cookie; don't store the server cookie */ - if (!(memcmp(data->cookie.data.components.client, + if (!(memcmp(data->cookie.data.cookie, cookie->opt_data+4, 8))) { /* the state of the cookie remains unchanged as we will * drop this upstream response */ - verbose(VERB_ALGO, "wrong client cookie from upstream with " - "previously seen cookie"); + verbose(VERB_ALGO, "wrong client cookie from upstream" + " with previously seen cookie"); lock_rw_unlock(&e->lock); return -1; } /* the server cookie has changed, but the client cookie has not * so we update the server cookie */ - if (memcmp(data->cookie.data.complete+8, + if (memcmp(data->cookie.data.cookie+8, cookie->opt_data+12, 16) != 0) { - memcpy(data->cookie.data.complete, cookie->opt_data, 24); + memcpy(data->cookie.data.cookie, cookie->opt_data, 24); /* the cookie state remains unchanged*/ verbose(VERB_ALGO, "update new server cookie from upstream"); @@ -792,13 +797,14 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr /* both the complete cookies are identical, so the state * remains unchanged */ - verbose(VERB_ALGO, "correctly received indentical cookie from upstream; don't update"); + verbose(VERB_ALGO, "correctly received indentical cookie from" + " upstream; don't update"); lock_rw_unlock(&e->lock); - return 0; + return 1; } else { /* cookie state == SERVER_COOKIE_UNKNOWN */ /* wrong client cookie; don't store the server cookie */ - if (!(memcmp(data->cookie.data.components.client, + if (!(memcmp(data->cookie.data.cookie, cookie->opt_data+4, 8))) { /* the state of the cookie remains unchanged as we will * drop this upstream response */ @@ -808,8 +814,8 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr return -1; } - /* else; store the client cookie */ - memcpy(data->cookie.data.complete, cookie->opt_data, 24); + /* store the server cookie */ + memcpy(data->cookie.data.cookie, cookie->opt_data, 24); data->cookie.state = SERVER_COOKIE_LEARNED; verbose(VERB_QUERY, "storing received server cookie from upstream"); diff --git a/services/cache/infra.h b/services/cache/infra.h index 93a6c9c9a..faa421351 100644 --- a/services/cache/infra.h +++ b/services/cache/infra.h @@ -55,17 +55,21 @@ struct config_file; /* COOKIE @TODO move this to correct spot */ /** - * The actual EDNS cookie data + * The actual EDNS cookie data. Note that the cookie can be filled with the + * just 'client' section, or with the 'complete' cookie depending on the state + * governed by the edns_cookie_state. + * The commented struct provides insight on how the bytes in the struct are + * structured. */ -union edns_cookie_data { - uint8_t complete[24]; - struct { +struct edns_cookie_data { + uint8_t cookie[24]; + /* struct { uint8_t client[8]; uint8_t version; uint8_t reserved[3]; uint32_t timestamp; uint8_t hash[8]; - } components; + } components; */ }; /** @@ -83,7 +87,7 @@ enum edns_cookie_state */ struct edns_cookie { enum edns_cookie_state state; - union edns_cookie_data data; + struct edns_cookie_data data; }; @@ -368,18 +372,18 @@ int infra_edns_update(struct infra_cache* infra, * @param name: name of zone * @param namelen: length of name * @param timenow: what time it is now. - * @return: struct ends_cookie* + * @param cookie: the cookie that is retrieved from cache on success. + * @return: 0 on error, cookie pointer remains unchanged then. */ -struct edns_cookie* -infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, +int infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, socklen_t addrlen, uint8_t* name, size_t namelen, - time_t timenow); + time_t timenow, struct edns_cookie* cookie); /** - * Find the cookie entry in the cache and update it with to make a complete - * cookie (RFC9018). This function asserts that the cookie param contains a - * complete cookie with a length of 24 bytes. It also asserts that a previous - * entry in the cache already exists, as it will update the entry. + * Find the cookie entry in the cache and update it with to make a 'complete cookie' + * (client+server) (RFC9018). This function asserts that the cookie param contains + * a complete cookie with a length of 24 bytes. If the cache entry isn't found + * a new one will be inserted. * @param infra: infrastructure cache. * @param addr: host address. * @param addrlen: length of addr. @@ -387,7 +391,9 @@ infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, * @param namelen: length of name * @param timenow: what time it is now. * @param cookie: the EDNS cookie option we want to store. - * @return true if the complete cookie is stored, and false if it is not + * @return -1 if the wrong client cookie is found, 0 if the entry isn't found in + * the cache and a new one is inserted, 1 if the complete cookie is inserted + * or unchanged. */ int infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr, socklen_t addrlen, uint8_t* name, size_t namelen, struct edns_option* cookie); diff --git a/services/outside_network.c b/services/outside_network.c index 86c91b3ee..8108db6be 100644 --- a/services/outside_network.c +++ b/services/outside_network.c @@ -3349,7 +3349,7 @@ outnet_serviced_query(struct outside_network* outnet, struct service_callback* cb; struct edns_string_addr* client_string_addr; struct regional* region; - struct edns_cookie* cookie; + struct edns_cookie cookie; struct edns_option* backed_up_opt_list = qstate->edns_opts_back_out; struct edns_option* per_upstream_opt_list = NULL; time_t timenow = 0; @@ -3389,18 +3389,18 @@ outnet_serviced_query(struct outside_network* outnet, client_string_addr->string, region); } - if (env->cfg->upstream_cookies) { - cookie = infra_get_cookie(env->infra_cache, addr, addrlen, zone, - zonelen, *env->now); + if (env->cfg->upstream_cookies && + infra_get_cookie(env->infra_cache, addr, addrlen, zone, zonelen, + *env->now, &cookie)) { - if (cookie->state == SERVER_COOKIE_LEARNED) { + if (cookie.state == SERVER_COOKIE_LEARNED) { /* We known the complete cookie, so we attach it */ edns_opt_list_append(&per_upstream_opt_list, LDNS_EDNS_COOKIE, - 24, cookie->data.complete, region); - } else if (cookie->state == SERVER_COOKIE_UNKNOWN) { + 24, cookie.data.cookie, region); + } else if (cookie.state == SERVER_COOKIE_UNKNOWN) { /* We know just client cookie, so we attach it */ edns_opt_list_append(&per_upstream_opt_list, LDNS_EDNS_COOKIE, - 8, cookie->data.components.client, region); + 8, cookie.data.cookie, region); } /* We ignore COOKIE_NOT_SUPPORTED */ } diff --git a/testcode/fake_event.c b/testcode/fake_event.c index 785370b54..504ba6b9d 100644 --- a/testcode/fake_event.c +++ b/testcode/fake_event.c @@ -1226,7 +1226,7 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet, struct edns_option* backed_up_opt_list = qstate->edns_opts_back_out; struct edns_option* per_upstream_opt_list = NULL; - struct edns_cookie* cookie; + struct edns_cookie cookie; /* If we have an already populated EDNS option list make a copy * since we may now add upstream specific EDNS options. */ if(qstate->edns_opts_back_out) { @@ -1256,20 +1256,20 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet, client_string_addr->string, qstate->region); } - if (qstate->env->cfg->upstream_cookies) { - cookie = infra_get_cookie(env->infra_cache, addr, addrlen, - zone, zonelen, *env->now); + if (qstate->env->cfg->upstream_cookies && + infra_get_cookie(env->infra_cache, addr, addrlen, + zone, zonelen, *env->now, &cookie)) { - if (cookie->state == SERVER_COOKIE_LEARNED) { + if (cookie.state == SERVER_COOKIE_LEARNED) { /* We known the complete cookie, so we attach it */ edns_opt_list_append(&per_upstream_opt_list, - LDNS_EDNS_COOKIE, 24, cookie->data.complete, + LDNS_EDNS_COOKIE, 24, cookie.data.cookie, qstate->region); - } else if (cookie->state == SERVER_COOKIE_UNKNOWN) { + } else if (cookie.state == SERVER_COOKIE_UNKNOWN) { /* We know just client cookie, so we attach it */ edns_opt_list_append(&per_upstream_opt_list, LDNS_EDNS_COOKIE, 8, - cookie->data.components.client, + cookie.data.cookie, qstate->region); } /* We ignore COOKIE_NOT_SUPPORTED */ } diff --git a/testcode/testpkts.c b/testcode/testpkts.c index af23c9dc2..fb4828cee 100644 --- a/testcode/testpkts.c +++ b/testcode/testpkts.c @@ -1529,8 +1529,14 @@ match_random_client_cookie(uint8_t* query, size_t query_len) } /* class + ttl + rdlen = 8 */ - if(walk_query_len <= 8) { - verbose(3, "No edns opt, no cookie"); + if (walk_query_len <= 8) { + verbose(3, "No correct EDNS record found, so no cookie"); + return 0; + } + + /* class + ttl + rdlen + opt_code + opt_len = 12 */ + if (walk_query_len < 12) { + verbose(3, "No EDNS opt found, so no cookie"); return 0; } @@ -1561,13 +1567,19 @@ match_random_complete_cookie(uint8_t* query, size_t query_len, struct entry* p) /* class + ttl + rdlen = 8 */ if(walk_query_len <= 8) { - verbose(3, "No edns opt, no cookie"); + verbose(3, "No correct EDNS record , so no cookie"); return 0; } walk_query += 8; walk_query_len -= 8; + /* opt_code + opt_len = 4 */ + if (walk_query_len < 4) { + verbose(3, "No EDNS opt found, so no cookie"); + return 0; + } + if (sldns_read_uint16(walk_query) != 10 /* LDNS_EDNS_COOKIE */) { verbose(3, "EDNS option is not a cookie"); return 0; @@ -1577,6 +1589,12 @@ match_random_complete_cookie(uint8_t* query, size_t query_len, struct entry* p) return 0; } + /* opt_code + opt_len + cookie_data = 28 */ + if (walk_query_len < 28) { + verbose(3, "No complete cookie found in the packet"); + return 0; + } + if (p->match_random_complete_cookie_renewed) { uint8_t renewed_cookie[16]; @@ -1881,18 +1899,16 @@ adjust_packet(struct entry* match, uint8_t** answer_pkt, size_t *answer_len, if (sldns_read_uint16(walk_query+2) == 8) { /* copy the EDNS client cookie from the query * packet to the response */ - memcpy(walk_response, walk_query, 12); + memmove(walk_response, walk_query, 12); /* add the server cookie to the client cookie to make it * 'complete'. we fake the siphash specified in RFC9018 * by hardcoding the server cookie */ - memcpy(walk_response+12, hardcoded_server_cookie, 16); + memmove(walk_response+12, hardcoded_server_cookie, 16); /* update the RDLEN and OPTLEN */ sldns_write_uint16(rdlen_ptr_response, 28); sldns_write_uint16(walk_response+2, 24); - - reslen = origlen + 28; } else if (sldns_read_uint16(walk_query+2) == 24) { /* update the RDLEN */ sldns_write_uint16(rdlen_ptr_response, 28); @@ -1903,11 +1919,11 @@ adjust_packet(struct entry* match, uint8_t** answer_pkt, size_t *answer_len, if (match->server_cookie_renew) { /* copy the cookie from the response but add a * different cookie (by reshuffeling server cookie) */ - memcpy(walk_response, walk_query, 12); - memcpy(walk_response+12, walk_query+12+8, 8); - memcpy(walk_response+12+8, walk_query+12, 8); + memmove(walk_response, walk_query, 12); + memmove(walk_response+12, walk_query+12+8, 8); + memmove(walk_response+12+8, walk_query+12, 8); } else { - memcpy(walk_response, walk_query, 28); + memmove(walk_response, walk_query, 28); } } else { log_err("testbound: the incoming EDNS cookie has the wrong length"); diff --git a/testcode/unitmain.c b/testcode/unitmain.c index fd906df96..b4c7d7fd1 100644 --- a/testcode/unitmain.c +++ b/testcode/unitmain.c @@ -529,6 +529,7 @@ infra_test(void) infra_delete(slab); config_delete(cfg); + ub_randfree(rnd); } #include "util/random.h"