From: George Thessalonikefs Date: Wed, 14 Jun 2023 09:40:59 +0000 (+0200) Subject: - Properly handle all return values of worker_check_request during X-Git-Tag: release-1.18.0rc1~24^2~29 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0f1ea7e490381b64e62a2fe9dfafadec8c57ba98;p=thirdparty%2Funbound.git - Properly handle all return values of worker_check_request during early EDE code. - Do not check the incoming request more than once. --- diff --git a/daemon/worker.c b/daemon/worker.c index ef9624b8b..d70e08e8f 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -289,61 +289,83 @@ worker_err_ratelimit(struct worker* worker, int err) return err; } +/** + * Structure holding the result of the worker_check_request function. + * Based on configuration it could be called up to four times; ideally should + * be called once. + */ +struct check_request_result { + int checked; + int value; +}; /** check request sanity. * @param pkt: the wire packet to examine for sanity. * @param worker: parameters for checking. - * @return error code, 0 OK, or -1 discard. + * @param out: struct to update with the result. */ -static int -worker_check_request(sldns_buffer* pkt, struct worker* worker) +static void +worker_check_request(sldns_buffer* pkt, struct worker* worker, + struct check_request_result* out) { + if(out->checked) return; + out->checked = 1; if(sldns_buffer_limit(pkt) < LDNS_HEADER_SIZE) { verbose(VERB_QUERY, "request too short, discarded"); - return -1; + out->value = -1; + return; } if(sldns_buffer_limit(pkt) > NORMAL_UDP_SIZE && worker->daemon->cfg->harden_large_queries) { verbose(VERB_QUERY, "request too large, discarded"); - return -1; + out->value = -1; + return; } if(LDNS_QR_WIRE(sldns_buffer_begin(pkt))) { verbose(VERB_QUERY, "request has QR bit on, discarded"); - return -1; + out->value = -1; + return; } if(LDNS_TC_WIRE(sldns_buffer_begin(pkt))) { LDNS_TC_CLR(sldns_buffer_begin(pkt)); verbose(VERB_QUERY, "request bad, has TC bit on"); - return worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + return; } if(LDNS_OPCODE_WIRE(sldns_buffer_begin(pkt)) != LDNS_PACKET_QUERY && LDNS_OPCODE_WIRE(sldns_buffer_begin(pkt)) != LDNS_PACKET_NOTIFY) { verbose(VERB_QUERY, "request unknown opcode %d", LDNS_OPCODE_WIRE(sldns_buffer_begin(pkt))); - return worker_err_ratelimit(worker, LDNS_RCODE_NOTIMPL); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_NOTIMPL); + return; } if(LDNS_QDCOUNT(sldns_buffer_begin(pkt)) != 1) { verbose(VERB_QUERY, "request wrong nr qd=%d", LDNS_QDCOUNT(sldns_buffer_begin(pkt))); - return worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + return; } if(LDNS_ANCOUNT(sldns_buffer_begin(pkt)) != 0 && (LDNS_ANCOUNT(sldns_buffer_begin(pkt)) != 1 || LDNS_OPCODE_WIRE(sldns_buffer_begin(pkt)) != LDNS_PACKET_NOTIFY)) { verbose(VERB_QUERY, "request wrong nr an=%d", LDNS_ANCOUNT(sldns_buffer_begin(pkt))); - return worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + return; } if(LDNS_NSCOUNT(sldns_buffer_begin(pkt)) != 0) { verbose(VERB_QUERY, "request wrong nr ns=%d", LDNS_NSCOUNT(sldns_buffer_begin(pkt))); - return worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + return; } if(LDNS_ARCOUNT(sldns_buffer_begin(pkt)) > 1) { verbose(VERB_QUERY, "request wrong nr ar=%d", LDNS_ARCOUNT(sldns_buffer_begin(pkt))); - return worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + out->value = worker_err_ratelimit(worker, LDNS_RCODE_FORMERR); + return; } - return 0; + out->value = 0; + return; } void @@ -1058,7 +1080,8 @@ static int deny_refuse(struct comm_point* c, enum acl_access acl, enum acl_access deny, enum acl_access refuse, struct worker* worker, struct comm_reply* repinfo, - struct acl_addr* acladdr, int ede) + struct acl_addr* acladdr, int ede, + struct check_request_result* check_result) { if(acl == deny) { if(verbosity >= VERB_ALGO) { @@ -1081,9 +1104,16 @@ deny_refuse(struct comm_point* c, enum acl_access acl, if(worker->stats.extended) worker->stats.unwanted_queries++; - if(worker_check_request(c->buffer, worker) == -1) { + worker_check_request(c->buffer, worker, check_result); + if(check_result->value != 0) { + if(check_result->value != -1) { + LDNS_QR_SET(sldns_buffer_begin(c->buffer)); + LDNS_RCODE_SET(sldns_buffer_begin(c->buffer), + check_result->value); + return 1; + } comm_point_drop_reply(repinfo); - return 0; /* discard this */ + return 0; } /* worker_check_request() above guarantees that the buffer contains at * least a header and that qdcount == 1 @@ -1237,7 +1267,8 @@ deny_refuse(struct comm_point* c, enum acl_access acl, static int deny_refuse_all(struct comm_point* c, enum acl_access* acl, struct worker* worker, struct comm_reply* repinfo, - struct acl_addr** acladdr, int ede, int check_proxy) + struct acl_addr** acladdr, int ede, int check_proxy, + struct check_request_result* check_result) { if(check_proxy) { *acladdr = acl_addr_lookup(worker->daemon->acl, @@ -1252,16 +1283,17 @@ deny_refuse_all(struct comm_point* c, enum acl_access* acl, } *acl = acl_get_control(*acladdr); return deny_refuse(c, *acl, acl_deny, acl_refuse, worker, repinfo, - *acladdr, ede); + *acladdr, ede, check_result); } static int deny_refuse_non_local(struct comm_point* c, enum acl_access acl, struct worker* worker, struct comm_reply* repinfo, - struct acl_addr* acladdr, int ede) + struct acl_addr* acladdr, int ede, + struct check_request_result* check_result) { return deny_refuse(c, acl, acl_deny_non_local, acl_refuse_non_local, - worker, repinfo, acladdr, ede); + worker, repinfo, acladdr, ede, check_result); } int @@ -1292,6 +1324,7 @@ worker_handle_request(struct comm_point* c, void* arg, int error, struct query_info qinfo_tmp; /* placeholder for lookup_qinfo */ struct respip_client_info* cinfo = NULL, cinfo_tmp; struct timeval wait_time; + struct check_request_result check_result = {0,0}; memset(&qinfo, 0, sizeof(qinfo)); if((error != NETEVENT_NOERROR && error != NETEVENT_DONE)|| !repinfo) { @@ -1322,7 +1355,8 @@ worker_handle_request(struct comm_point* c, void* arg, int error, if(c->dnscrypt && !repinfo->is_dnscrypted) { char buf[LDNS_MAX_DOMAINLEN+1]; /* Check if this is unencrypted and asking for certs */ - if(worker_check_request(c->buffer, worker) != 0) { + worker_check_request(c->buffer, worker, &check_result); + if(check_result.value != 0) { verbose(VERB_ALGO, "dnscrypt: worker check request: bad query."); log_addr(VERB_CLIENT,"from",&repinfo->client_addr, @@ -1371,25 +1405,27 @@ worker_handle_request(struct comm_point* c, void* arg, int error, /* Check deny/refuse ACLs */ if(repinfo->is_proxied) { if((ret=deny_refuse_all(c, &acl, worker, repinfo, &acladdr, - worker->env.cfg->ede, 1)) != -1) { + worker->env.cfg->ede, 1, &check_result)) != -1) { if(ret == 1) goto send_reply; return ret; } } if((ret=deny_refuse_all(c, &acl, worker, repinfo, &acladdr, - worker->env.cfg->ede, 0)) != -1) { + worker->env.cfg->ede, 0, &check_result)) != -1) { if(ret == 1) goto send_reply; return ret; } - if((ret=worker_check_request(c->buffer, worker)) != 0) { + worker_check_request(c->buffer, worker, &check_result); + if(check_result.value != 0) { verbose(VERB_ALGO, "worker check request: bad query."); log_addr(VERB_CLIENT,"from",&repinfo->client_addr, repinfo->client_addrlen); - if(ret != -1) { + if(check_result.value != -1) { LDNS_QR_SET(sldns_buffer_begin(c->buffer)); - LDNS_RCODE_SET(sldns_buffer_begin(c->buffer), ret); + LDNS_RCODE_SET(sldns_buffer_begin(c->buffer), + check_result.value); return 1; } comm_point_drop_reply(repinfo); @@ -1612,7 +1648,7 @@ worker_handle_request(struct comm_point* c, void* arg, int error, /* We've looked in our local zones. If the answer isn't there, we * might need to bail out based on ACLs now. */ if((ret=deny_refuse_non_local(c, acl, worker, repinfo, acladdr, - worker->env.cfg->ede)) != -1) + worker->env.cfg->ede, &check_result)) != -1) { regional_free_all(worker->scratchpad); if(ret == 1) diff --git a/doc/Changelog b/doc/Changelog index 7b3b65bb9..dc3013854 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,8 @@ +14 June 2023: George + - Properly handle all return values of worker_check_request during + early EDE code. + - Do not check the incoming request more than once. + 12 June 2023: Wouter - Merge #896: Fix: #895: pythonmodule: add all site-packages directories to sys.path.