From f1d178cce1ada81346fb864c8f95aa2163b37a56 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 23 Jul 2015 12:57:58 +0200 Subject: resolved: packet - fail on invalid zero-length data Most blobs (keys, signatures, ...) should have a specific size given by the relevant algorithm. However, as we don't use/verify the algorithms yet, let's just ensure that we don't read out zero-length data in cases where this does not make sense. The only exceptions, where zero-length data is allowed are in the NSEC3 salt field, and the generic data (which we don't know anything about, so better not make any assumptions). --- src/resolve/resolved-dns-packet.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 773691f066..955d513d7c 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1513,6 +1513,13 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { if (r < 0) goto fail; + if (rr->ds.digest_size <= 0) { + /* the accepted size depends on the algorithm, but for now + just ensure that the value is greater than zero */ + r = -EBADMSG; + goto fail; + } + break; case DNS_TYPE_SSHFP: r = dns_packet_read_uint8(p, &rr->sshfp.algorithm, NULL); @@ -1526,6 +1533,14 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { r = dns_packet_read_memdup(p, rdlength - 2, &rr->sshfp.key, &rr->sshfp.key_size, NULL); + + if (rr->sshfp.key_size <= 0) { + /* the accepted size depends on the algorithm, but for now + just ensure that the value is greater than zero */ + r = -EBADMSG; + goto fail; + } + break; case DNS_TYPE_DNSKEY: { @@ -1557,6 +1572,14 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { r = dns_packet_read_memdup(p, rdlength - 4, &rr->dnskey.key, &rr->dnskey.key_size, NULL); + + if (rr->dnskey.key_size <= 0) { + /* the accepted size depends on the algorithm, but for now + just ensure that the value is greater than zero */ + r = -EBADMSG; + goto fail; + } + break; } @@ -1596,6 +1619,14 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { r = dns_packet_read_memdup(p, offset + rdlength - p->rindex, &rr->rrsig.signature, &rr->rrsig.signature_size, NULL); + + if (rr->rrsig.signature_size <= 0) { + /* the accepted size depends on the algorithm, but for now + just ensure that the value is greater than zero */ + r = -EBADMSG; + goto fail; + } + break; case DNS_TYPE_NSEC: @@ -1626,6 +1657,7 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { if (r < 0) goto fail; + /* this may be zero */ r = dns_packet_read_uint8(p, &size, NULL); if (r < 0) goto fail; @@ -1638,6 +1670,11 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { if (r < 0) goto fail; + if (size <= 0) { + r = -EBADMSG; + goto fail; + } + r = dns_packet_read_memdup(p, size, &rr->nsec3.next_hashed_name, &rr->nsec3.next_hashed_name_size, NULL); if (r < 0) goto fail; -- cgit v1.2.3-54-g00ecf From 549c1a2564b56f2bb38f1203d59c747ea15817f3 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 23 Jul 2015 13:09:35 +0200 Subject: resolved: rr - SSHFP contains the fingerprint, not the key Rename the field to make this clearer. --- src/resolve/resolved-dns-packet.c | 6 +++--- src/resolve/resolved-dns-rr.c | 8 ++++---- src/resolve/resolved-dns-rr.h | 5 +++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 955d513d7c..af219ce4ff 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -761,7 +761,7 @@ int dns_packet_append_rr(DnsPacket *p, const DnsResourceRecord *rr, size_t *star if (r < 0) goto fail; - r = dns_packet_append_blob(p, rr->sshfp.key, rr->sshfp.key_size, NULL); + r = dns_packet_append_blob(p, rr->sshfp.fingerprint, rr->sshfp.fingerprint_size, NULL); break; case DNS_TYPE_DNSKEY: @@ -1531,10 +1531,10 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { goto fail; r = dns_packet_read_memdup(p, rdlength - 2, - &rr->sshfp.key, &rr->sshfp.key_size, + &rr->sshfp.fingerprint, &rr->sshfp.fingerprint_size, NULL); - if (rr->sshfp.key_size <= 0) { + if (rr->sshfp.fingerprint_size <= 0) { /* the accepted size depends on the algorithm, but for now just ensure that the value is greater than zero */ r = -EBADMSG; diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index 9efe4b3c08..2bc9f2b520 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -276,7 +276,7 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr) { break; case DNS_TYPE_SSHFP: - free(rr->sshfp.key); + free(rr->sshfp.fingerprint); break; case DNS_TYPE_DNSKEY: @@ -434,8 +434,8 @@ int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecor case DNS_TYPE_SSHFP: return a->sshfp.algorithm == b->sshfp.algorithm && a->sshfp.fptype == b->sshfp.fptype && - a->sshfp.key_size == b->sshfp.key_size && - memcmp(a->sshfp.key, b->sshfp.key, a->sshfp.key_size) == 0; + a->sshfp.fingerprint_size == b->sshfp.fingerprint_size && + memcmp(a->sshfp.fingerprint, b->sshfp.fingerprint, a->sshfp.fingerprint_size) == 0; case DNS_TYPE_DNSKEY: return a->dnskey.zone_key_flag == b->dnskey.zone_key_flag && @@ -687,7 +687,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { break; case DNS_TYPE_SSHFP: - t = hexmem(rr->sshfp.key, rr->sshfp.key_size); + t = hexmem(rr->sshfp.fingerprint, rr->sshfp.fingerprint_size); if (!t) return -ENOMEM; diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h index dc51126f97..0f40f3ceef 100644 --- a/src/resolve/resolved-dns-rr.h +++ b/src/resolve/resolved-dns-rr.h @@ -117,11 +117,12 @@ struct DnsResourceRecord { size_t digest_size; } ds; + /* https://tools.ietf.org/html/rfc4255#section-3.1 */ struct { uint8_t algorithm; uint8_t fptype; - void *key; - size_t key_size; + void *fingerprint; + size_t fingerprint_size; } sshfp; /* http://tools.ietf.org/html/rfc4034#section-2.1 */ -- cgit v1.2.3-54-g00ecf From 89492aaf99a59b2858e71ebc30f3e123e7c6734c Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 23 Jul 2015 13:13:43 +0200 Subject: resolved: rr - don't read past end of RR when parsing NSEC(3) We can never read past the end of the packet, so this seems impossible to exploit, but let's error out early as reading past the end of the current RR is clearly an error. Found by Lennart, based on patch by Daniel. --- src/resolve/resolved-dns-packet.c | 43 ++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index af219ce4ff..25035ed7ee 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1233,6 +1233,38 @@ fail: return r; } +static int dns_packet_read_type_windows(DnsPacket *p, Bitmap **types, size_t size, size_t *start) { + size_t saved_rindex; + int r; + + saved_rindex = p->rindex; + + while (p->rindex < saved_rindex + size) { + r = dns_packet_read_type_window(p, types, NULL); + if (r < 0) + goto fail; + + /* don't read past end of current RR */ + if (p->rindex > saved_rindex + size) { + r = -EBADMSG; + goto fail; + } + } + + if (p->rindex != saved_rindex + size) { + r = -EBADMSG; + goto fail; + } + + if (start) + *start = saved_rindex; + + return 0; +fail: + dns_packet_rewind(p, saved_rindex); + return r; +} + int dns_packet_read_key(DnsPacket *p, DnsResourceKey **ret, size_t *start) { _cleanup_free_ char *name = NULL; uint16_t class, type; @@ -1634,11 +1666,12 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { if (r < 0) goto fail; - while (p->rindex != offset + rdlength) { - r = dns_packet_read_type_window(p, &rr->nsec.types, NULL); - if (r < 0) - goto fail; - } + r = dns_packet_read_type_windows(p, &rr->nsec.types, offset + rdlength - p->rindex, NULL); + if (r < 0) + goto fail; + + /* NSEC RRs with empty bitmpas makes no sense, but the RFC does not explicitly forbid them + so we allow it */ break; -- cgit v1.2.3-54-g00ecf From 0bbd72b2f3d79b5f15ae1fa7d1fea6e579eeef97 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 23 Jul 2015 13:28:09 +0200 Subject: resolved: rr - fix parsing of NSEC3 We were appending rather than reading the bitmap. --- src/resolve/resolved-dns-packet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 25035ed7ee..b76f981906 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1712,10 +1712,12 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { if (r < 0) goto fail; - r = dns_packet_append_types(p, rr->nsec3.types, NULL); + r = dns_packet_read_type_windows(p, &rr->nsec.types, offset + rdlength - p->rindex, NULL); if (r < 0) goto fail; + /* empty non-terminals can have NSEC3 records, so empty bitmaps are allowed */ + break; } default: -- cgit v1.2.3-54-g00ecf From 8e6edc490c612d5c0f58a267b066e66a1ccaae2a Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Thu, 23 Jul 2015 13:48:56 +0200 Subject: resolved: rr - ignore pseudo types in NSEC(3) bitmaps --- src/resolve/dns-type.c | 5 +++++ src/resolve/dns-type.h | 1 + src/resolve/resolved-dns-packet.c | 5 ++++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c index a3e740896f..e1087b3219 100644 --- a/src/resolve/dns-type.c +++ b/src/resolve/dns-type.c @@ -43,3 +43,8 @@ int dns_type_from_string(const char *s) { return sc->id; } + +/* XXX: find an authorotative list of all pseudo types? */ +bool dns_type_is_pseudo(int n) { + return IN_SET(n, DNS_TYPE_ANY, DNS_TYPE_AXFR, DNS_TYPE_IXFR, DNS_TYPE_OPT); +} diff --git a/src/resolve/dns-type.h b/src/resolve/dns-type.h index 86951d233a..950af36ee3 100644 --- a/src/resolve/dns-type.h +++ b/src/resolve/dns-type.h @@ -25,6 +25,7 @@ const char *dns_type_to_string(int type); int dns_type_from_string(const char *s); +bool dns_type_is_pseudo(int n); /* DNS record types, taken from * http://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml. diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index b76f981906..2dd1f564fa 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1208,9 +1208,12 @@ static int dns_packet_read_type_window(DnsPacket *p, Bitmap **types, size_t *sta if (bitmap[i] & bitmask) { uint16_t n; - /* XXX: ignore pseudo-types? see RFC4034 section 4.1.2 */ n = (uint16_t) window << 8 | (uint16_t) bit; + /* Ignore pseudo-types. see RFC4034 section 4.1.2 */ + if (dns_type_is_pseudo(n)) + continue; + r = bitmap_set(*types, n); if (r < 0) goto fail; -- cgit v1.2.3-54-g00ecf From 0e03ade57ea327cdefc34aaeb0a47c985b238120 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Fri, 17 Jul 2015 23:42:18 +0200 Subject: resolved: packet - fix segfault in truncate() A size_t was being accessed as a char* due to the order of arguments being inverted. --- src/resolve/resolved-dns-packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 2dd1f564fa..649e8b74e1 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -275,7 +275,7 @@ static void dns_packet_truncate(DnsPacket *p, size_t sz) { if (p->size <= sz) return; - HASHMAP_FOREACH_KEY(s, n, p->names, i) { + HASHMAP_FOREACH_KEY(n, s, p->names, i) { if (PTR_TO_SIZE(n) < sz) continue; -- cgit v1.2.3-54-g00ecf From 6709eb94f9eae15f0b4ce87ad98de05b4182a30a Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Sun, 19 Jul 2015 21:42:52 +0200 Subject: resolve: transaction - stop processing packet when found to be invalid We were stopping the transaction, but we need to stop processing the packet alltogether. --- src/resolve/resolved-dns-transaction.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index e468f245f7..3d46c99df8 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -417,8 +417,10 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { /* Only consider responses with equivalent query section to the request */ if (!dns_question_is_superset(p->question, t->question) || - !dns_question_is_superset(t->question, p->question)) + !dns_question_is_superset(t->question, p->question)) { dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY); + return; + } /* According to RFC 4795, section 2.9. only the RRs from the answer section shall be cached */ dns_cache_put(&t->scope->cache, p->question, DNS_PACKET_RCODE(p), p->answer, DNS_PACKET_ANCOUNT(p), 0, p->family, &p->sender); -- cgit v1.2.3-54-g00ecf