diff --git a/CHANGES b/CHANGES index 076989f185..c39458f765 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6022. [performance] The decompression implementation in dns_name_fromwire() + is now smaller and faster. [GL #3655] + 6021. [bug] Use the current domain name when checking answers from a dual-stack-server. [GL #3607] diff --git a/lib/dns/include/dns/name.h b/lib/dns/include/dns/name.h index 036a151f30..423f6fea09 100644 --- a/lib/dns/include/dns/name.h +++ b/lib/dns/include/dns/name.h @@ -689,9 +689,6 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx, * Notes: * \li Decompression policy is controlled by 'dctx'. * - * \li If DNS_NAME_DOWNCASE is set, any uppercase letters in 'source' will be - * downcased when they are copied into 'target'. - * * Security: * * \li *** WARNING *** @@ -712,13 +709,12 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx, * * \li 'dctx' is a valid decompression context. * + * \li DNS_NAME_DOWNCASE is not set. + * * Ensures: * * If result is success: - * \li If 'target' is not NULL, 'name' is attached to it. - * - * \li Uppercase letters are downcased in the copy iff - * DNS_NAME_DOWNCASE is set in options. + * \li If 'target' is not NULL, 'name' is attached to it. * * \li The current location in source is advanced, and the used space * in target is updated. @@ -731,7 +727,6 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx, * \li Bad Form: Compression type not allowed * \li Bad Form: Bad compression pointer * \li Bad Form: Input too short - * \li Resource Limit: Too many compression pointers * \li Resource Limit: Not enough space in buffer */ diff --git a/lib/dns/name.c b/lib/dns/name.c index 96586def8c..5ce5969815 100644 --- a/lib/dns/name.c +++ b/lib/dns/name.c @@ -47,8 +47,6 @@ typedef enum { ft_at } ft_state; -typedef enum { fw_start = 0, fw_ordinary, fw_newcurrent } fw_state; - #define INIT_OFFSETS(name, var, default_offsets) \ if ((name)->offsets != NULL) \ var = (name)->offsets; \ @@ -1520,175 +1518,174 @@ set_offsets(const dns_name_t *name, unsigned char *offsets, } isc_result_t -dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx, - unsigned int options, isc_buffer_t *target) { - unsigned char *cdata, *ndata; - unsigned int cused; /* Bytes of compressed name data used */ - unsigned int nused, labels, n, nmax; - unsigned int current, new_current, biggest_pointer; - bool done; - fw_state state = fw_start; - unsigned int c; - unsigned char *offsets; - dns_offsets_t odata; - bool downcase; - bool seen_pointer; - +dns_name_fromwire(dns_name_t *const name, isc_buffer_t *const source, + const dns_decompress_t dctx, unsigned int options, + isc_buffer_t *target) { /* - * Copy the possibly-compressed name at source into target, - * decompressing it. Loop prevention is performed by checking - * the new pointer against biggest_pointer. + * Copy the name at source into target, decompressing it. + * + * *** WARNING *** + * + * dns_name_fromwire() deals with raw network data. An error in this + * routine could result in the failure or hijacking of the server. + * + * The description of name compression in RFC 1035 section 4.1.4 is + * subtle wrt certain edge cases. The first important sentence is: + * + * > In this scheme, an entire domain name or a list of labels at the + * > end of a domain name is replaced with a pointer to a prior + * > occurance of the same name. + * + * The key word is "prior". This says that compression pointers must + * point strictly earlier in the message (before our "marker" variable), + * which is enough to prevent DoS attacks due to compression loops. + * + * The next important sentence is: + * + * > If a domain name is contained in a part of the message subject to a + * > length field (such as the RDATA section of an RR), and compression + * > is used, the length of the compressed name is used in the length + * > calculation, rather than the length of the expanded name. + * + * When decompressing, this means that the amount of the source buffer + * that we consumed (which is checked wrt the container's length field) + * is the length of the compressed name. A compressed name is defined as + * a sequence of labels ending with the root label or a compression + * pointer, that is, the segment of the name that dns_name_fromwire() + * examines first. + * + * This matters when handling names that play dirty tricks, like: + * + * +---+---+---+---+---+---+ + * | 4 | 1 |'a'|192| 0 | 0 | + * +---+---+---+---+---+---+ + * + * We start at octet 1. There is an ordinary single character label "a", + * followed by a compression pointer that refers back to octet zero. + * Here there is a label of length 4, which weirdly re-uses the octets + * we already examined as the data for the label. It is followed by the + * root label, + * + * The specification says that the compressed name ends after the first + * zero octet (after the compression pointer) not the second zero octet, + * even though the second octet is later in the message. This shows the + * correct way to set our "consumed" variable. */ + REQUIRE((options & DNS_NAME_DOWNCASE) == 0); REQUIRE(VALID_NAME(name)); + REQUIRE(BINDABLE(name)); REQUIRE((target != NULL && ISC_BUFFER_VALID(target)) || (target == NULL && ISC_BUFFER_VALID(name->buffer))); - downcase = ((options & DNS_NAME_DOWNCASE) != 0); - if (target == NULL && name->buffer != NULL) { target = name->buffer; isc_buffer_clear(target); } - REQUIRE(BINDABLE(name)); + uint8_t *const name_buf = isc_buffer_used(target); + const uint32_t name_max = ISC_MIN(DNS_NAME_MAXWIRE, + isc_buffer_availablelength(target)); + uint32_t name_len = 0; + MAKE_EMPTY(name); /* in case of failure */ + dns_offsets_t odata; + uint8_t *offsets = NULL; + uint32_t labels = 0; INIT_OFFSETS(name, offsets, odata); /* - * Make 'name' empty in case of failure. + * After chasing a compression pointer, these variables refer to the + * source buffer as follows: + * + * sb --- mr --- cr --- st --- cd --- sm + * + * sb = source_buf (const) + * mr = marker + * cr = cursor + * st = start (const) + * cd = consumed + * sm = source_max (const) + * + * The marker hops backwards for each pointer. + * The cursor steps forwards for each label. + * The amount of the source we consumed is set once. */ - MAKE_EMPTY(name); + const uint8_t *const source_buf = isc_buffer_base(source); + const uint8_t *const source_max = isc_buffer_used(source); + const uint8_t *const start = isc_buffer_current(source); + const uint8_t *marker = start; + const uint8_t *cursor = start; + const uint8_t *consumed = NULL; /* - * Initialize things to make the compiler happy; they're not required. + * One iteration per label. */ - n = 0; - new_current = 0; - - /* - * Set up. - */ - labels = 0; - done = false; - - ndata = isc_buffer_used(target); - nused = 0; - seen_pointer = false; - - /* - * Find the maximum number of uncompressed target name - * bytes we are willing to generate. This is the smaller - * of the available target buffer length and the - * maximum legal domain name length (255). - */ - nmax = isc_buffer_availablelength(target); - if (nmax > DNS_NAME_MAXWIRE) { - nmax = DNS_NAME_MAXWIRE; - } - - cdata = isc_buffer_current(source); - cused = 0; - - current = source->current; - biggest_pointer = current; - - /* - * Note: The following code is not optimized for speed, but - * rather for correctness. Speed will be addressed in the future. - */ - - while (current < source->active && !done) { - c = *cdata++; - current++; - if (!seen_pointer) { - cused++; - } - - switch (state) { - case fw_start: - if (c < 64) { - offsets[labels] = nused; - labels++; - if (nused + c + 1 > nmax) { - goto full; - } - nused += c + 1; - *ndata++ = c; - if (c == 0) { - done = true; - } - n = c; - state = fw_ordinary; - } else if (c >= 192) { - /* - * 14-bit compression pointer - */ - if (!dns_decompress_getpermitted(dctx)) { - return (DNS_R_DISALLOWED); - } - new_current = c & 0x3F; - state = fw_newcurrent; - } else { - return (DNS_R_BADLABELTYPE); + while (cursor < source_max) { + const uint8_t label_len = *cursor++; + if (label_len < 64) { + /* + * Normal label: record its offset, and check bounds on + * the name length, which also ensures we don't overrun + * the offsets array. Don't touch any source bytes yet! + * The source bounds check will happen when we loop. + */ + offsets[labels++] = name_len; + /* and then a step to the ri-i-i-i-i-ight */ + cursor += label_len; + name_len += label_len + 1; + if (name_len > name_max) { + return (name_max == DNS_NAME_MAXWIRE + ? DNS_R_NAMETOOLONG + : ISC_R_NOSPACE); + } else if (label_len == 0) { + goto root_label; } - break; - case fw_ordinary: - if (downcase) { - c = isc_ascii_tolower(c); - } - *ndata++ = c; - n--; - if (n == 0) { - state = fw_start; - } - break; - case fw_newcurrent: - new_current *= 256; - new_current += c; - if (new_current >= biggest_pointer) { + } else if (label_len < 192) { + return (DNS_R_BADLABELTYPE); + } else if (!dns_decompress_getpermitted(dctx)) { + return (DNS_R_DISALLOWED); + } else if (cursor < source_max) { + /* + * Compression pointer. Ensure it does not loop. + * + * Copy multiple labels in one go, to make the most of + * memmove() performance. Start at the marker and finish + * just before the pointer's hi+lo bytes, before the + * cursor. Bounds were already checked. + */ + const uint32_t hi = label_len & 0x3F; + const uint32_t lo = *cursor++; + const uint8_t *pointer = source_buf + (256 * hi + lo); + if (pointer >= marker) { return (DNS_R_BADPOINTER); } - biggest_pointer = new_current; - current = new_current; - cdata = (unsigned char *)source->base + current; - seen_pointer = true; - state = fw_start; - break; - default: - FATAL_ERROR("Unknown state %d", state); - /* Does not return. */ + const uint32_t copy_len = (cursor - 2) - marker; + uint8_t *const dest = name_buf + name_len - copy_len; + memmove(dest, marker, copy_len); + consumed = consumed != NULL ? consumed : cursor; + /* it's just a jump to the left */ + cursor = marker = pointer; } } + return (ISC_R_UNEXPECTEDEND); +root_label:; + /* + * Copy labels almost like we do for compression pointers, + * from the marker up to and including the root label. + */ + const uint32_t copy_len = cursor - marker; + memmove(name_buf + name_len - copy_len, marker, copy_len); + consumed = consumed != NULL ? consumed : cursor; + isc_buffer_forward(source, consumed - start); - if (!done) { - return (ISC_R_UNEXPECTEDEND); - } - - name->ndata = (unsigned char *)target->base + target->used; - name->labels = labels; - name->length = nused; name->attributes.absolute = true; - - isc_buffer_forward(source, cused); - isc_buffer_add(target, name->length); + name->ndata = name_buf; + name->labels = labels; + name->length = name_len; + isc_buffer_add(target, name_len); return (ISC_R_SUCCESS); - -full: - if (nmax == DNS_NAME_MAXWIRE) { - /* - * The name did not fit even though we had a buffer - * big enough to fit a maximum-length name. - */ - return (DNS_R_NAMETOOLONG); - } else { - /* - * The name might fit if only the caller could give us a - * big enough buffer. - */ - return (ISC_R_NOSPACE); - } } isc_result_t