From 2cc718a86d92abef7363384d7eac48190271f676 Mon Sep 17 00:00:00 2001 From: Greg Hudson Date: Wed, 21 May 2025 17:11:07 -0400 Subject: [PATCH] Refactor dnsglue functions using k5input Use k5input to make the dnsglue.c and dnssrv.c functions simpler and more robust. Rename the dnsglue functions to use the now-preferred k5_ prefix instead of krb5int_. Use stdint type names instead of unsigned char and unsigned short. Remove some out-of-date comments. Change the query loop in k5_dns_init() to continue expanding the buffer if the reported answer length is exactly equal to the buffer size, as that is the observed behavior from the glibc res_nquery() when the buffer size is too small for the answer. --- src/lib/krb5/os/dnsglue.c | 319 ++++++++++++++++---------------------- src/lib/krb5/os/dnsglue.h | 61 +++----- src/lib/krb5/os/dnssrv.c | 55 ++++--- 3 files changed, 177 insertions(+), 258 deletions(-) diff --git a/src/lib/krb5/os/dnsglue.c b/src/lib/krb5/os/dnsglue.c index fd403aa752..b422d8f488 100644 --- a/src/lib/krb5/os/dnsglue.c +++ b/src/lib/krb5/os/dnsglue.c @@ -36,35 +36,25 @@ #include #endif -/* - * Only use res_ninit() if there's also a res_ndestroy(), to avoid - * memory leaks (Linux & Solaris) and outright corruption (AIX 4.x, - * 5.x). While we're at it, make sure res_nsearch() is there too. - * - * In any case, it is probable that platforms having broken - * res_ninit() will have thread safety hacks for res_init() and _res. - */ - /* * Opaque handle */ -struct krb5int_dns_state { +struct k5_dns_state { int nclass; int ntype; - void *ansp; - int anslen; - int ansmax; + uint8_t *ans; + uint8_t *ansend; #if HAVE_NS_INITPARSE int cur_ans; ns_msg msg; #else - unsigned char *ptr; + struct k5input in; unsigned short nanswers; #endif }; #if !HAVE_NS_INITPARSE -static int initparse(struct krb5int_dns_state *); +static int initparse(struct k5_dns_state *); #endif /* @@ -110,35 +100,25 @@ static int initparse(struct krb5int_dns_state *); #endif -/* - * krb5int_dns_init() - * - * Initialize an opaque handle. Do name lookup and initial parsing of - * reply, skipping question section. Prepare to iterate over answer - * section. Returns -1 on error, 0 on success. - */ int -krb5int_dns_init(struct krb5int_dns_state **dsp, - char *host, int nclass, int ntype) +k5_dns_init(const char *host, int nclass, int ntype, + struct k5_dns_state **ds_out) { - struct krb5int_dns_state *ds; - int len, ret; - size_t nextincr, maxincr; - unsigned char *p; + struct k5_dns_state *ds; + int bufsize, len, ret = -1; + const int maxlen = 64 * 1024; + uint8_t *p; DECLARE_HANDLE(h); - *dsp = ds = malloc(sizeof(*ds)); + *ds_out = NULL; + + ds = malloc(sizeof(*ds)); if (ds == NULL) return -1; - ret = -1; ds->nclass = nclass; ds->ntype = ntype; - ds->ansp = NULL; - ds->anslen = 0; - ds->ansmax = 0; - nextincr = 4096; - maxincr = INT_MAX; + ds->ans = ds->ansend = NULL; #if HAVE_NS_INITPARSE ds->cur_ans = 0; @@ -147,78 +127,61 @@ krb5int_dns_init(struct krb5int_dns_state **dsp, if (!INIT_HANDLE(h)) return -1; + len = 0; + bufsize = 4096; do { - p = (ds->ansp == NULL) - ? malloc(nextincr) : realloc(ds->ansp, nextincr); + /* Use a buffer size larger than the previously returned length. (We + * may go up to 128K to confirm that a 64K answer wasn't truncated.) */ + while (bufsize <= len && bufsize <= maxlen) + bufsize *= 2; - if (p == NULL) { - ret = -1; - goto errout; - } - ds->ansp = p; - ds->ansmax = nextincr; + p = realloc(ds->ans, bufsize); + if (p == NULL) + goto cleanup; + ds->ans = p; - len = SEARCH(h, host, ds->nclass, ds->ntype, ds->ansp, ds->ansmax); - if ((size_t) len > maxincr) { - ret = -1; - goto errout; - } - while (nextincr < (size_t) len) - nextincr *= 2; - if (len < 0 || nextincr > maxincr) { - ret = -1; - goto errout; - } - } while (len > ds->ansmax); + len = SEARCH(h, host, ds->nclass, ds->ntype, ds->ans, bufsize); + if (len < 0 || len > maxlen) + goto cleanup; + + /* A length equal to bufsize may indicate a truncated reply. */ + } while (len >= bufsize); - ds->anslen = len; + ds->ansend = ds->ans + len; #if HAVE_NS_INITPARSE - ret = ns_initparse(ds->ansp, ds->anslen, &ds->msg); + ret = ns_initparse(ds->ans, len, &ds->msg); #else ret = initparse(ds); #endif if (ret < 0) - goto errout; + goto cleanup; + *ds_out = ds; + ds = NULL; ret = 0; -errout: +cleanup: DESTROY_HANDLE(h); - if (ret < 0) { - if (ds->ansp != NULL) { - free(ds->ansp); - ds->ansp = NULL; - } - } - + k5_dns_fini(ds); return ret; } #if HAVE_NS_INITPARSE -/* - * krb5int_dns_nextans - get next matching answer record - * - * Sets pp to NULL if no more records. Returns -1 on error, 0 on - * success. - */ int -krb5int_dns_nextans(struct krb5int_dns_state *ds, - const unsigned char **pp, int *lenp) +k5_dns_nextans(struct k5_dns_state *ds, struct k5input *ans_out) { int len; ns_rr rr; - *pp = NULL; - *lenp = 0; + k5_input_init(ans_out, NULL, 0); while (ds->cur_ans < ns_msg_count(ds->msg, ns_s_an)) { len = ns_parserr(&ds->msg, ns_s_an, ds->cur_ans, &rr); if (len < 0) return -1; ds->cur_ans++; - if (ds->nclass == (int)ns_rr_class(rr) - && ds->ntype == (int)ns_rr_type(rr)) { - *pp = ns_rr_rdata(rr); - *lenp = ns_rr_rdlen(rr); + if (ds->nclass == (int)ns_rr_class(rr) && + ds->ntype == (int)ns_rr_type(rr)) { + k5_input_init(ans_out, ns_rr_rdata(rr), ns_rr_rdlen(rr)); return 0; } } @@ -226,35 +189,33 @@ krb5int_dns_nextans(struct krb5int_dns_state *ds, } #endif -/* - * krb5int_dns_expand - wrapper for dn_expand() - */ int -krb5int_dns_expand(struct krb5int_dns_state *ds, const unsigned char *p, - char *buf, int len) +k5_dns_expand(struct k5_dns_state *ds, struct k5input *in, char *buf, int len) { + int clen; + + if (in->status) + return -1; + /* Uncompress the name into buf. */ #if HAVE_NS_NAME_UNCOMPRESS - return ns_name_uncompress(ds->ansp, - (unsigned char *)ds->ansp + ds->anslen, - p, buf, (size_t)len); + clen = ns_name_uncompress(ds->ans, ds->ansend, in->ptr, buf, (size_t)len); #else - return dn_expand(ds->ansp, - (unsigned char *)ds->ansp + ds->anslen, - p, buf, len); + clen = dn_expand(ds->ans, ds->ansend, in->ptr, buf, len); #endif + + /* Advance in past the compressed name. */ + (void)k5_input_get_bytes(in, clen); + + return in->status ? -1 : 0; } -/* - * Free stuff. - */ void -krb5int_dns_fini(struct krb5int_dns_state *ds) +k5_dns_fini(struct k5_dns_state *ds) { if (ds == NULL) return; - if (ds->ansp != NULL) - free(ds->ansp); + free(ds->ans); free(ds); } @@ -263,102 +224,83 @@ krb5int_dns_fini(struct krb5int_dns_state *ds) */ #if !HAVE_NS_INITPARSE -/* - * initparse - * - * Skip header and question section of reply. Set a pointer to the - * beginning of the answer section, and prepare to iterate over - * answer records. - */ static int -initparse(struct krb5int_dns_state *ds) +namelen(const uint8_t *ptr, const uint8_t *ans, const uint8_t *ansend) { - HEADER *hdr; - unsigned char *p; - unsigned short nqueries, nanswers; - int len; -#if !HAVE_DN_SKIPNAME +#if HAVE_DN_SKIPNAME + return dn_skipname(ptr, ansend); +#else char host[MAXDNAME]; + + return dn_expand(ans, ansend, ptr, host, sizeof(host)); #endif +} - if ((size_t) ds->anslen < sizeof(HEADER)) - return -1; +static void +skipname(struct k5_dns_state *ds) +{ + int len; - hdr = (HEADER *)ds->ansp; - p = ds->ansp; - nqueries = ntohs((unsigned short)hdr->qdcount); - nanswers = ntohs((unsigned short)hdr->ancount); - p += sizeof(HEADER); + if (ds->in.status) + return; + len = namelen(ds->in.ptr, ds->ans, ds->ansend); + if (len < 0) + k5_input_set_status(&ds->in, EINVAL); + else + (void)k5_input_get_bytes(&ds->in, len); +} - /* - * Skip query records. - */ +/* Prepare to iterate over answer records by reading the reply header and + * advancing past the questions section. */ +static int +initparse(struct k5_dns_state *ds) +{ + uint16_t nqueries, nanswers; + + k5_input_init(&ds->in, ds->ans, ds->ansend - ds->ans); + + /* Skip id and flags. */ + (void)k5_input_get_bytes(&ds->in, 4); + + nqueries = k5_input_get_uint16_be(&ds->in); + nanswers = k5_input_get_uint16_be(&ds->in); + + /* Skip nscount and arcount. */ + (void)k5_input_get_bytes(&ds->in, 4); + + /* Skip query records. */ while (nqueries--) { -#if HAVE_DN_SKIPNAME - len = dn_skipname(p, (unsigned char *)ds->ansp + ds->anslen); -#else - len = dn_expand(ds->ansp, (unsigned char *)ds->ansp + ds->anslen, - p, host, sizeof(host)); -#endif - if (len < 0 || !INCR_OK(ds->ansp, ds->anslen, p, len + 4)) - return -1; - p += len + 4; + /* Skip qname, then qtype and qclass. */ + skipname(ds); + (void)k5_input_get_bytes(&ds->in, 4); } - ds->ptr = p; ds->nanswers = nanswers; - return 0; + return ds->in.status ? -1 : 0; } -/* - * krb5int_dns_nextans() - get next answer record - * - * Sets pp to NULL if no more records. - */ int -krb5int_dns_nextans(struct krb5int_dns_state *ds, - const unsigned char **pp, int *lenp) +k5_dns_nextans(struct k5_dns_state *ds, struct k5input *ans_out) { - int len; - unsigned char *p; - unsigned short ntype, nclass, rdlen; -#if !HAVE_DN_SKIPNAME - char host[MAXDNAME]; -#endif - - *pp = NULL; - *lenp = 0; - p = ds->ptr; + uint16_t ntype, nclass, rdlen; + const uint8_t *rdata; + k5_input_init(ans_out, NULL, 0); while (ds->nanswers--) { -#if HAVE_DN_SKIPNAME - len = dn_skipname(p, (unsigned char *)ds->ansp + ds->anslen); -#else - len = dn_expand(ds->ansp, (unsigned char *)ds->ansp + ds->anslen, - p, host, sizeof(host)); -#endif - if (len < 0 || !INCR_OK(ds->ansp, ds->anslen, p, len)) - return -1; - p += len; - SAFE_GETUINT16(ds->ansp, ds->anslen, p, 2, ntype, out); - /* Also skip 4 bytes of TTL */ - SAFE_GETUINT16(ds->ansp, ds->anslen, p, 6, nclass, out); - SAFE_GETUINT16(ds->ansp, ds->anslen, p, 2, rdlen, out); - - if (!INCR_OK(ds->ansp, ds->anslen, p, rdlen)) - return -1; - if (rdlen > INT_MAX) + skipname(ds); + ntype = k5_input_get_uint16_be(&ds->in); + nclass = k5_input_get_uint16_be(&ds->in); + /* Skip TTL. */ + (void)k5_input_get_bytes(&ds->in, 4); + rdlen = k5_input_get_uint16_be(&ds->in); + rdata = k5_input_get_bytes(&ds->in, rdlen); + if (rdata == NULL) return -1; if (nclass == ds->nclass && ntype == ds->ntype) { - *pp = p; - *lenp = rdlen; - ds->ptr = p + rdlen; + k5_input_init(ans_out, rdata, rdlen); return 0; } - p += rdlen; } return 0; -out: - return -1; } #endif /* !HAVE_NS_INITPARSE */ @@ -451,10 +393,11 @@ k5_try_realm_txt_rr(krb5_context context, const char *prefix, const char *name, char **realm) { krb5_error_code retval = KRB5_ERR_HOST_REALM_UNKNOWN; - const unsigned char *p, *base; + const uint8_t *p; char *txtname = NULL; - int ret, rdlen, len; - struct krb5int_dns_state *ds = NULL; + int ret, len; + struct k5_dns_state *ds = NULL; + struct k5input in = { 0 }; /* * Form our query, and send it via DNS @@ -463,33 +406,31 @@ k5_try_realm_txt_rr(krb5_context context, const char *prefix, const char *name, txtname = txt_lookup_name(prefix, name); if (txtname == NULL) return ENOMEM; - ret = krb5int_dns_init(&ds, txtname, C_IN, T_TXT); + ret = k5_dns_init(txtname, C_IN, T_TXT, &ds); if (ret < 0) { TRACE_TXT_LOOKUP_NOTFOUND(context, txtname); goto errout; } - ret = krb5int_dns_nextans(ds, &base, &rdlen); - if (ret < 0 || rdlen < 2 || *base == 0 || *base > rdlen - 1) + ret = k5_dns_nextans(ds, &in); + if (ret < 0 || in.len < 2) goto errout; - p = base; - len = *p++; - *realm = malloc((size_t)len + 1); - if (*realm == NULL) { - retval = ENOMEM; + len = k5_input_get_byte(&in); + p = k5_input_get_bytes(&in, len); + if (len == 0 || p == NULL) + goto errout; + *realm = k5memdup0(p, len, &retval); + if (*realm == NULL) goto errout; - } - strncpy(*realm, (const char *)p, (size_t)len); - (*realm)[len] = '\0'; /* Avoid a common error. */ - if ( (*realm)[len-1] == '.' ) - (*realm)[len-1] = '\0'; - retval = 0; + if ((*realm)[len - 1] == '.') + (*realm)[len - 1] = '\0'; + TRACE_TXT_LOOKUP_SUCCESS(context, txtname, *realm); errout: - krb5int_dns_fini(ds); + k5_dns_fini(ds); free(txtname); return retval; } diff --git a/src/lib/krb5/os/dnsglue.h b/src/lib/krb5/os/dnsglue.h index 9e98735249..acca5dcf20 100644 --- a/src/lib/krb5/os/dnsglue.h +++ b/src/lib/krb5/os/dnsglue.h @@ -31,16 +31,9 @@ */ /* - * BIND 4 doesn't have the ns_initparse() API, so we need to do some - * manual parsing via the HEADER struct. BIND 8 does have - * ns_initparse(), but has enums for the various protocol constants - * rather than the BIND 4 macros. BIND 9 (at least on macOS 10.3) - * appears to disable res_nsearch() if BIND_8_COMPAT is defined - * (which is necessary to obtain the HEADER struct). - * - * We use ns_initparse() if available at all, and never define - * BIND_8_COMPAT. If there is no ns_initparse(), we do manual parsing - * by using the HEADER struct. + * BIND 4 doesn't have the ns_initparse() API, so we need to do some manual + * parsing. BIND 8 does have ns_initparse(), but has enums for the various + * protocol constants rather than the BIND 4 macros. */ #ifndef KRB5_DNSGLUE_H @@ -50,6 +43,7 @@ #ifdef KRB5_DNS_LOOKUP #include "k5-int.h" +#include "k5-input.h" #include "os-proto.h" #include #include @@ -117,40 +111,27 @@ #define T_URI 256 #endif -/* - * INCR_OK - * - * Given moving pointer PTR offset from BASE, return true if adding - * INCR to PTR doesn't move it PTR than MAX bytes from BASE. - */ -#define INCR_OK(base, max, ptr, incr) \ - ((incr) <= (max) - ((const unsigned char *)(ptr) \ - - (const unsigned char *)(base))) +struct k5_dns_state; + +/* Perform a name lookup and set *ds_out to a handle for iteration over the + * answers. Return -1 on error, 0 on success. */ +int k5_dns_init(const char *host, int nclass, int ntype, + struct k5_dns_state **ds_out); /* - * SAFE_GETUINT16 - * - * Given PTR offset from BASE, if at least INCR bytes are safe to - * read, get network byte order uint16 into S, and increment PTR. On - * failure, goto LABEL. + * Initialize *ans_out with the record data of the next matching answer from + * ds, or with the empty string if there are no more matching answers. Return + * -1 on error, 0 on success. */ +int k5_dns_nextans(struct k5_dns_state *ds, struct k5input *ans_out); + +/* Expand a compressed name from in into buf (a buffer with at least len bytes + * of space) and advance past it. Return -1 on error, 0 on success. */ +int k5_dns_expand(struct k5_dns_state *ds, struct k5input *in, + char *buf, int len); -#define SAFE_GETUINT16(base, max, ptr, incr, s, label) \ - do { \ - if (!INCR_OK(base, max, ptr, incr)) goto label; \ - (s) = (unsigned short)(ptr)[0] << 8 \ - | (unsigned short)(ptr)[1]; \ - (ptr) += (incr); \ - } while (0) - -struct krb5int_dns_state; - -int krb5int_dns_init(struct krb5int_dns_state **, char *, int, int); -int krb5int_dns_nextans(struct krb5int_dns_state *, - const unsigned char **, int *); -int krb5int_dns_expand(struct krb5int_dns_state *, - const unsigned char *, char *, int); -void krb5int_dns_fini(struct krb5int_dns_state *); +/* Release ds. */ +void k5_dns_fini(struct k5_dns_state *ds); #endif /* KRB5_DNS_LOOKUP */ #endif /* !defined(KRB5_DNSGLUE_H) */ diff --git a/src/lib/krb5/os/dnssrv.c b/src/lib/krb5/os/dnssrv.c index 2eb6389889..9930799dd8 100644 --- a/src/lib/krb5/os/dnssrv.c +++ b/src/lib/krb5/os/dnssrv.c @@ -189,12 +189,12 @@ k5_make_uri_query(krb5_context context, const krb5_data *realm, const char *service, const char *sitename, struct srv_dns_entry **answers) { - const unsigned char *p = NULL, *base = NULL; char *name = NULL; - int size, ret, rdlen; - unsigned short priority, weight; - struct krb5int_dns_state *ds = NULL; + int size, ret; + uint16_t priority, weight; + struct k5_dns_state *ds = NULL; struct srv_dns_entry *head = NULL, *uri = NULL; + struct k5input in = { 0 }; *answers = NULL; @@ -205,7 +205,7 @@ k5_make_uri_query(krb5_context context, const krb5_data *realm, TRACE_DNS_URI_SEND(context, name); - size = krb5int_dns_init(&ds, name, C_IN, T_URI); + size = k5_dns_init(name, C_IN, T_URI, &ds); if (size < 0 && sitename != NULL) { /* Try again without the site name. */ free(name); @@ -215,14 +215,14 @@ k5_make_uri_query(krb5_context context, const krb5_data *realm, goto out; for (;;) { - ret = krb5int_dns_nextans(ds, &base, &rdlen); - if (ret < 0 || base == NULL) + ret = k5_dns_nextans(ds, &in); + if (ret < 0 || in.len == 0) goto out; - p = base; - - SAFE_GETUINT16(base, rdlen, p, 2, priority, out); - SAFE_GETUINT16(base, rdlen, p, 2, weight, out); + priority = k5_input_get_uint16_be(&in); + weight = k5_input_get_uint16_be(&in); + if (in.status) + goto out; uri = k5alloc(sizeof(*uri), &ret); if (uri == NULL) @@ -230,8 +230,7 @@ k5_make_uri_query(krb5_context context, const krb5_data *realm, uri->priority = priority; uri->weight = weight; - /* rdlen - 4 bytes remain after the priority and weight. */ - uri->host = k5memdup0(p, rdlen - 4, &ret); + uri->host = k5memdup0(in.ptr, in.len, &ret); if (uri->host == NULL) { free(uri); goto out; @@ -242,7 +241,7 @@ k5_make_uri_query(krb5_context context, const krb5_data *realm, } out: - krb5int_dns_fini(ds); + k5_dns_fini(ds); free(name); *answers = head; return 0; @@ -261,12 +260,12 @@ krb5int_make_srv_query_realm(krb5_context context, const krb5_data *realm, const char *sitename, struct srv_dns_entry **answers) { - const unsigned char *p = NULL, *base = NULL; char *name = NULL, host[MAXDNAME]; - int size, ret, rdlen, nlen; - unsigned short priority, weight, port; - struct krb5int_dns_state *ds = NULL; + int size, ret; + uint16_t priority, weight, port; + struct k5_dns_state *ds = NULL; struct srv_dns_entry *head = NULL, *srv = NULL; + struct k5input in = { 0 }; /* * First off, build a query of the form: @@ -285,7 +284,7 @@ krb5int_make_srv_query_realm(krb5_context context, const krb5_data *realm, TRACE_DNS_SRV_SEND(context, name); - size = krb5int_dns_init(&ds, name, C_IN, T_SRV); + size = k5_dns_init(name, C_IN, T_SRV, &ds); if (size < 0 && sitename) { /* Try again without the site name. */ free(name); @@ -296,22 +295,20 @@ krb5int_make_srv_query_realm(krb5_context context, const krb5_data *realm, goto out; for (;;) { - ret = krb5int_dns_nextans(ds, &base, &rdlen); - if (ret < 0 || base == NULL) + ret = k5_dns_nextans(ds, &in); + if (ret < 0 || in.len == 0) goto out; - p = base; - - SAFE_GETUINT16(base, rdlen, p, 2, priority, out); - SAFE_GETUINT16(base, rdlen, p, 2, weight, out); - SAFE_GETUINT16(base, rdlen, p, 2, port, out); + priority = k5_input_get_uint16_be(&in); + weight = k5_input_get_uint16_be(&in); + port = k5_input_get_uint16_be(&in); /* * RFC 2782 says the target is never compressed in the reply; * do we believe that? We need to flatten it anyway, though. */ - nlen = krb5int_dns_expand(ds, p, host, sizeof(host)); - if (nlen < 0 || !INCR_OK(base, rdlen, p, nlen)) + ret = k5_dns_expand(ds, &in, host, sizeof(host)); + if (ret < 0) goto out; /* @@ -340,7 +337,7 @@ krb5int_make_srv_query_realm(krb5_context context, const krb5_data *realm, } out: - krb5int_dns_fini(ds); + k5_dns_fini(ds); free(name); *answers = head; return 0; -- 2.47.2