From: Willem Toorop Date: Tue, 25 Sep 2012 11:58:14 +0000 (+0000) Subject: Final code review thingies: X-Git-Tag: release-1.6.14rc1~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e5822572592c6c47f516e99c0ca99e211461859d;p=thirdparty%2Fldns.git Final code review thingies: Bufferoverflow in data_buffer2wire in ldns-testpkts.c Print unknown rcodes in ldns_axfr_next in resolver.c Handle errors in main in ldns-keyfetcher.c Continue in correct loop in ldns_resolver_new_frm_fp_l in resolver.c Skip set delimeters (to del) when tokenread i.s.o. delim in ldns_bget_token and ldns_fget_token in parse.c Test reply when verifying tsig in ldns_send_buffer in net.c Assert that verify_next_hashed_name is only called with nsecs in the zone with ldns-verify-zone.c Set inception and expiration on keys after they are read from the engine in main in ldns-signzone.c Gracefully return from ldns_dnssec_zone_add_empty_nonterminals in a broken rbtree. Check if tree->rr is null before use in ldns_dnssec_trust_tree_contains_keys in dnssec_verify.c Check data for null before use in ldns_dnssec_create_nsec_bitmap in dnssec.c Dead code in ldns_str2rdf_wks, ldns_resolver_query, examples/ldns_testpkts/data_buffer2wire, drill/work/packetbuffromfile & ldns_dnssec_verify_denial_nsec3_match. --- diff --git a/dnssec.c b/dnssec.c index 5469fb18..37fd6e97 100644 --- a/dnssec.c +++ b/dnssec.c @@ -743,7 +743,10 @@ ldns_dnssec_create_nsec_bitmap(ldns_rr_type rr_type_list[], memcpy(data + cur_data_size + 2, cur_data, cur_window_max+1); cur_data_size += cur_window_max + 3; } - + if (! data) { + LDNS_FREE(bitmap); + return NULL; + } bitmap_rdf = ldns_rdf_new_frm_data(LDNS_RDF_TYPE_NSEC, cur_data_size, data); diff --git a/dnssec_verify.c b/dnssec_verify.c index a8844300..d435eedf 100644 --- a/dnssec_verify.c +++ b/dnssec_verify.c @@ -1078,7 +1078,8 @@ ldns_dnssec_trust_tree_contains_keys(ldns_dnssec_trust_tree *tree, if (tree->parent_status[i] != LDNS_STATUS_OK) { result = tree->parent_status[i]; } else { - if (ldns_rr_get_type(tree->rr) + if (tree->rr && + ldns_rr_get_type(tree->rr) == LDNS_RR_TYPE_NSEC && parent_result == LDNS_STATUS_OK ) { @@ -1613,7 +1614,7 @@ ldns_dnssec_verify_denial_nsec3_match( ldns_rr *rr ldns_rr_get_type(rr), nsecs); if(!closest_encloser) { - result = LDNS_STATUS_NSEC3_ERR; + result = LDNS_STATUS_DNSSEC_NSEC_RR_NOT_COVERED; goto done; } @@ -1637,16 +1638,14 @@ ldns_dnssec_verify_denial_nsec3_match( ldns_rr *rr ldns_rdf_deep_free(hashed_wildcard_name); } - ldns_rdf_deep_free(closest_encloser); - ldns_rdf_deep_free(wildcard); - - if (!wildcard_covered) { + if (! wildcard_covered) { result = LDNS_STATUS_DNSSEC_NSEC_WILDCARD_NOT_COVERED; - } else if (closest_encloser && wildcard_covered) { - result = LDNS_STATUS_OK; } else { - result = LDNS_STATUS_DNSSEC_NSEC_RR_NOT_COVERED; + result = LDNS_STATUS_OK; } + ldns_rdf_deep_free(closest_encloser); + ldns_rdf_deep_free(wildcard); + } else if (packet_nodata && packet_qtype != LDNS_RR_TYPE_DS) { /* section 8.5 */ hashed_name = ldns_nsec3_hash_name_frm_nsec3( diff --git a/dnssec_zone.c b/dnssec_zone.c index 464a1943..df71a23c 100644 --- a/dnssec_zone.c +++ b/dnssec_zone.c @@ -1012,7 +1012,9 @@ ldns_dnssec_zone_add_empty_nonterminals(ldns_dnssec_zone *zone) if (next_node == LDNS_RBTREE_NULL) { next_node = ldns_rbtree_first(zone->names); } - + if (! cur_node->data || ! next_node->data) { + return LDNS_STATUS_ERR; + } cur_name = ((ldns_dnssec_name *)cur_node->data)->name; next_name = ((ldns_dnssec_name *)next_node->data)->name; cur_label_count = ldns_dname_label_count(cur_name); diff --git a/drill/drill_util.c b/drill/drill_util.c index 4efb1f00..950cffbc 100644 --- a/drill/drill_util.c +++ b/drill/drill_util.c @@ -20,7 +20,7 @@ read_line(FILE *input, char *line, size_t len) char c; for (i = 0; i < len-1; i++) { - c = getc(input); + c = (int)getc(input); if (c == EOF) { return -1; } else if (c != '\n') { diff --git a/drill/work.c b/drill/work.c index 150cd7ed..339907b0 100644 --- a/drill/work.c +++ b/drill/work.c @@ -122,11 +122,6 @@ packetbuffromfile(char *filename, uint8_t *wire) hexbuf[hexbufpos] = (uint8_t) c; hexbufpos++; break; - default: - warning("unknown state while reading %s", filename); - xfree(hexbuf); - return 0; - break; } c = fgetc(fp); } diff --git a/examples/ldns-keyfetcher.c b/examples/ldns-keyfetcher.c index 267822f8..ee06aea9 100644 --- a/examples/ldns-keyfetcher.c +++ b/examples/ldns-keyfetcher.c @@ -649,10 +649,21 @@ main(int argc, char *argv[]) fprintf(stderr, "Warning: Unable to create stub resolver from /etc/resolv.conf:\n"); fprintf(stderr, "%s\n", ldns_get_errorstr_by_id(status)); fprintf(stderr, "defaulting to nameserver at 127.0.0.1 for separate nameserver name lookups\n"); - res = ldns_resolver_new(); - ns = ldns_rdf_new_frm_str(LDNS_RDF_TYPE_A, "127.0.0.1"); - status = ldns_resolver_push_nameserver(res, ns); - if (status != LDNS_STATUS_OK) { + for (;;) { + res = ldns_resolver_new(); + if (res) { + ns = ldns_rdf_new_frm_str(LDNS_RDF_TYPE_A, + "127.0.0.1"); + if (ns) { + status = ldns_resolver_push_nameserver( + res, ns); + if (status == LDNS_STATUS_OK) { + break; + } + ldns_rdf_deep_free(ns); + } + ldns_resolver_free(res); + } fprintf(stderr, "Unable to create stub resolver: %s\n", ldns_get_errorstr_by_id(status)); exit(EXIT_FAILURE); } diff --git a/examples/ldns-read-zone.c b/examples/ldns-read-zone.c index 6e00a3fc..efe187e6 100644 --- a/examples/ldns-read-zone.c +++ b/examples/ldns-read-zone.c @@ -60,7 +60,7 @@ main(int argc, char **argv) } break; case 'h': - printf("Usage: %s [-c] [-v] [-z] \n", argv[0]); + printf("Usage: %s [OPTIONS] \n", argv[0]); printf("\tReads the zonefile and prints it.\n"); printf("\tThe RR count of the zone is printed to stderr.\n"); printf("\t-b include bubblebabble of DS's.\n"); diff --git a/examples/ldns-signzone.c b/examples/ldns-signzone.c index 34615039..25ece3a6 100644 --- a/examples/ldns-signzone.c +++ b/examples/ldns-signzone.c @@ -513,13 +513,6 @@ main(int argc, char *argv[]) printf("Engine key id: %s, algo %d\n", eng_key_id, eng_key_algo); - if (expiration != 0) { - ldns_key_set_expiration(key, expiration); - } - if (inception != 0) { - ldns_key_set_inception(key, inception); - } - s = ldns_key_new_frm_engine(&key, engine, eng_key_id, eng_key_algo); if (s == LDNS_STATUS_OK) { /* must be dnssec key */ @@ -544,6 +537,14 @@ main(int argc, char *argv[]) fprintf(stderr, "Warning, key not suitable for signing, ignoring key with algorithm %u\n", ldns_key_algorithm(key)); break; } + if (expiration != 0) { + ldns_key_set_expiration(key, + expiration); + } + if (inception != 0) { + ldns_key_set_inception(key, + inception); + } } else { printf("Error reading key '%s' from engine: %s\n", eng_key_id, ldns_get_errorstr_by_id(s)); #ifdef HAVE_SSL diff --git a/examples/ldns-testpkts.c b/examples/ldns-testpkts.c index 033ea15d..7a6c020e 100644 --- a/examples/ldns-testpkts.c +++ b/examples/ldns-testpkts.c @@ -340,6 +340,12 @@ data_buffer2wire(ldns_buffer *data_buffer) (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') ) { + if (hexbufpos >= LDNS_MAX_PACKETLEN) { + error("buffer overflow"); + LDNS_FREE(hexbuf); + return 0; + + } hexbuf[hexbufpos] = (uint8_t) c; hexbufpos++; } else if (c == ';') { @@ -354,14 +360,14 @@ data_buffer2wire(ldns_buffer *data_buffer) } break; case 2: + if (hexbufpos >= LDNS_MAX_PACKETLEN) { + error("buffer overflow"); + LDNS_FREE(hexbuf); + return 0; + } hexbuf[hexbufpos] = (uint8_t) c; hexbufpos++; break; - default: - error("unknown state while reading"); - LDNS_FREE(hexbuf); - return 0; - break; } } @@ -371,6 +377,11 @@ data_buffer2wire(ldns_buffer *data_buffer) /* lenient mode: length must be multiple of 2 */ if (hexbufpos % 2 != 0) { + if (hexbufpos >= LDNS_MAX_PACKETLEN) { + error("buffer overflow"); + LDNS_FREE(hexbuf); + return 0; + } hexbuf[hexbufpos] = (uint8_t) '0'; hexbufpos++; } diff --git a/examples/ldns-verify-zone.c b/examples/ldns-verify-zone.c index 7829c43d..373d4701 100644 --- a/examples/ldns-verify-zone.c +++ b/examples/ldns-verify-zone.c @@ -309,6 +309,9 @@ verify_next_hashed_name(ldns_dnssec_zone* zone, ldns_dnssec_name *name) if (!cur_next_name) { cur_next_name = cur_first_name; } + assert(cur_next_name); /* Because this function is called on nsec + * occurrence, it must be there! + */ next_owner_str = ldns_rdf2str(ldns_nsec3_next_owner(name->nsec)); next_owner_dname = ldns_dname_new_frm_str(next_owner_str); diff --git a/net.c b/net.c index 6241d2d1..6b444da6 100644 --- a/net.c +++ b/net.c @@ -206,7 +206,7 @@ ldns_send_buffer(ldns_pkt **result, ldns_resolver *r, ldns_buffer *qb, ldns_rdf return LDNS_STATUS_RES_NO_NS; } #ifdef HAVE_SSL - if (tsig_mac && reply_bytes) { + if (tsig_mac && reply && reply_bytes) { if (!ldns_pkt_tsig_verify(reply, reply_bytes, reply_size, diff --git a/parse.c b/parse.c index ac9bdbdd..ea5ffad0 100644 --- a/parse.c +++ b/parse.c @@ -161,7 +161,7 @@ ldns_fget_token_l(FILE *f, char *token, const char *delim, size_t limit, int *li return (ssize_t)i; tokenread: - ldns_fskipcs_l(f, delim, line_nr); + ldns_fskipcs_l(f, del, line_nr); *t = '\0'; if (p != 0) { return -1; @@ -331,7 +331,7 @@ ldns_bget_token(ldns_buffer *b, char *token, const char *delim, size_t limit) return (ssize_t)i; tokenread: - ldns_bskipcs(b, delim); + ldns_bskipcs(b, del); *t = '\0'; if (p != 0) { diff --git a/resolver.c b/resolver.c index 08681453..2cee9fff 100644 --- a/resolver.c +++ b/resolver.c @@ -809,8 +809,7 @@ ldns_resolver_new_frm_fp_l(ldns_resolver **res, FILE *fp, int *line_nr) gtr -= bgtr; if(word[0] == '#') { expect = LDNS_RESOLV_KEYWORD; - ldns_buffer_free(b); - continue; + break; } tmp = ldns_rdf_new_frm_str(LDNS_RDF_TYPE_DNAME, word); if (!tmp) { @@ -826,8 +825,10 @@ ldns_resolver_new_frm_fp_l(ldns_resolver **res, FILE *fp, int *line_nr) (size_t) gtr + 1); } ldns_buffer_free(b); - gtr = 1; - expect = LDNS_RESOLV_KEYWORD; + if (expect != LDNS_RESOLV_KEYWORD) { + gtr = 1; + expect = LDNS_RESOLV_KEYWORD; + } break; case LDNS_RESOLV_SORTLIST: gtr = ldns_fget_token_l(fp, word, LDNS_PARSE_SKIP_SPACE, 0, line_nr); @@ -1024,9 +1025,6 @@ ldns_resolver_query(const ldns_resolver *r, const ldns_rdf *name, newname = ldns_dname_cat_clone((const ldns_rdf*)name, ldns_resolver_domain(r)); if (!newname) { - if (pkt) { - ldns_pkt_free(pkt); - } return NULL; } @@ -1303,7 +1301,14 @@ ldns_axfr_next(ldns_resolver *resolver) return NULL; } else if (ldns_pkt_get_rcode(resolver->_cur_axfr_pkt) != 0) { rcode = ldns_lookup_by_id(ldns_rcodes, (int) ldns_pkt_get_rcode(resolver->_cur_axfr_pkt)); - fprintf(stderr, "Error in AXFR: %s\n", rcode->name); + if (rcode) { + fprintf(stderr, "Error in AXFR: %s\n", + rcode->name); + } else { + fprintf(stderr, "Error in AXFR: %d\n", + (int) ldns_pkt_get_rcode( + resolver->_cur_axfr_pkt)); + } /* RoRi: we must now also close the socket, otherwise subsequent uses of the same resolver structure will fail because the link is still open or diff --git a/str2host.c b/str2host.c index b5e4be10..51357cc3 100644 --- a/str2host.c +++ b/str2host.c @@ -1105,8 +1105,6 @@ ldns_str2rdf_wks(ldns_rdf **rd, const char *str) data[0] = (uint8_t) proto->p_proto; } else if (proto_str) { data[0] = (uint8_t) atoi(proto_str); - } else { - data[0] = 0; } memcpy(data + 1, bitmap, (size_t) bm_len);