Skip to content

Commit

Permalink
resolved: limit the number of signature validations in a transaction
Browse files Browse the repository at this point in the history
It has been demonstrated that tolerating an unbounded number of dnssec
signature validations is a bad idea. It is easy for a maliciously
crafted DNS reply to contain as many keytag collisions as desired,
causing us to iterate every dnskey and signature combination in vain.

The solution is to impose a maximum number of validations we will
tolerate. While collisions are not hard to craft, I still expect they
are unlikely in the wild so it should be safe to pick fairly small
values.

Here two limits are imposed: one on the maximum number of invalid
signatures encountered per rrset, and another on the total number of
validations performed per transaction.

(cherry picked from commit 67d0ce8843d612a2245d0966197d4f528b911b66)

Resolves: RHEL-26644
  • Loading branch information
jacekmigacz authored and github-actions[bot] committed Mar 7, 2024
1 parent ccaa361 commit 899e3c4
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 6 deletions.
16 changes: 14 additions & 2 deletions src/resolve/resolved-dns-dnssec.c
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,7 @@ int dnssec_verify_rrset_search(
DnsResourceRecord **ret_rrsig) {

bool found_rrsig = false, found_invalid = false, found_expired_rrsig = false, found_unsupported_algorithm = false;
unsigned nvalidations = 0;
DnsResourceRecord *rrsig;
int r;

Expand Down Expand Up @@ -1041,6 +1042,14 @@ int dnssec_verify_rrset_search(
if (realtime == USEC_INFINITY)
realtime = now(CLOCK_REALTIME);

/* Have we seen an unreasonable number of invalid signaures? */
if (nvalidations > DNSSEC_INVALID_MAX) {
if (ret_rrsig)
*ret_rrsig = NULL;
*result = DNSSEC_TOO_MANY_VALIDATIONS;
return (int) nvalidations;
}

/* Yay, we found a matching RRSIG with a matching
* DNSKEY, awesome. Now let's verify all entries of
* the RRSet against the RRSIG and DNSKEY
Expand All @@ -1050,6 +1059,8 @@ int dnssec_verify_rrset_search(
if (r < 0)
return r;

nvalidations++;

switch (one_result) {

case DNSSEC_VALIDATED:
Expand All @@ -1060,7 +1071,7 @@ int dnssec_verify_rrset_search(
*ret_rrsig = rrsig;

*result = one_result;
return 0;
return (int) nvalidations;

case DNSSEC_INVALID:
/* If the signature is invalid, let's try another
Expand Down Expand Up @@ -1107,7 +1118,7 @@ int dnssec_verify_rrset_search(
if (ret_rrsig)
*ret_rrsig = NULL;

return 0;
return (int) nvalidations;
}

int dnssec_has_rrsig(DnsAnswer *a, const DnsResourceKey *key) {
Expand Down Expand Up @@ -2301,6 +2312,7 @@ static const char* const dnssec_result_table[_DNSSEC_RESULT_MAX] = {
[DNSSEC_FAILED_AUXILIARY] = "failed-auxiliary",
[DNSSEC_NSEC_MISMATCH] = "nsec-mismatch",
[DNSSEC_INCOMPATIBLE_SERVER] = "incompatible-server",
[DNSSEC_TOO_MANY_VALIDATIONS] = "too-many-validations",
};
DEFINE_STRING_TABLE_LOOKUP(dnssec_result, DnssecResult);

Expand Down
9 changes: 8 additions & 1 deletion src/resolve/resolved-dns-dnssec.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ typedef enum DnssecVerdict DnssecVerdict;
#include "resolved-dns-rr.h"

enum DnssecResult {
/* These five are returned by dnssec_verify_rrset() */
/* These six are returned by dnssec_verify_rrset() */
DNSSEC_VALIDATED,
DNSSEC_VALIDATED_WILDCARD, /* Validated via a wildcard RRSIG, further NSEC/NSEC3 checks necessary */
DNSSEC_INVALID,
DNSSEC_SIGNATURE_EXPIRED,
DNSSEC_UNSUPPORTED_ALGORITHM,
DNSSEC_TOO_MANY_VALIDATIONS,

/* These two are added by dnssec_verify_rrset_search() */
DNSSEC_NO_SIGNATURE,
Expand Down Expand Up @@ -45,6 +46,12 @@ enum DnssecVerdict {
/* The longest digest we'll ever generate, of all digest algorithms we support */
#define DNSSEC_HASH_SIZE_MAX (MAX(20, 32))

/* The most invalid signatures we will tolerate for a single rrset */
#define DNSSEC_INVALID_MAX 5

/* The total number of signature validations we will tolerate for a single transaction */
#define DNSSEC_VALIDATION_MAX 64

int dnssec_rrsig_match_dnskey(DnsResourceRecord *rrsig, DnsResourceRecord *dnskey, bool revoked_ok);
int dnssec_key_match_rrsig(const DnsResourceKey *key, DnsResourceRecord *rrsig);

Expand Down
19 changes: 16 additions & 3 deletions src/resolve/resolved-dns-transaction.c
Original file line number Diff line number Diff line change
Expand Up @@ -2870,11 +2870,14 @@ static int dnssec_validate_records(
DnsTransaction *t,
Phase phase,
bool *have_nsec,
unsigned *nvalidations,
DnsAnswer **validated) {

DnsResourceRecord *rr;
int r;

assert(nvalidations);

/* Returns negative on error, 0 if validation failed, 1 to restart validation, 2 when finished. */

DNS_ANSWER_FOREACH(rr, t->answer) {
Expand Down Expand Up @@ -2909,6 +2912,7 @@ static int dnssec_validate_records(
r = dnssec_verify_rrset_search(t->answer, rr->key, t->validated_keys, USEC_INFINITY, &result, &rrsig);
if (r < 0)
return r;
*nvalidations += r;

log_debug("Looking at %s: %s", strna(dns_resource_record_to_string(rr)), dnssec_result_to_string(result));

Expand Down Expand Up @@ -3086,7 +3090,8 @@ static int dnssec_validate_records(
DNSSEC_SIGNATURE_EXPIRED,
DNSSEC_NO_SIGNATURE))
manager_dnssec_verdict(t->scope->manager, DNSSEC_BOGUS, rr->key);
else /* DNSSEC_MISSING_KEY or DNSSEC_UNSUPPORTED_ALGORITHM */
else /* DNSSEC_MISSING_KEY, DNSSEC_UNSUPPORTED_ALGORITHM,
or DNSSEC_TOO_MANY_VALIDATIONS */
manager_dnssec_verdict(t->scope->manager, DNSSEC_INDETERMINATE, rr->key);

/* This is a primary response to our question, and it failed validation.
Expand Down Expand Up @@ -3180,13 +3185,21 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
return r;

phase = DNSSEC_PHASE_DNSKEY;
for (;;) {
for (unsigned nvalidations = 0;;) {
bool have_nsec = false;

r = dnssec_validate_records(t, phase, &have_nsec, &validated);
r = dnssec_validate_records(t, phase, &have_nsec, &nvalidations, &validated);
if (r <= 0)
return r;

if (nvalidations > DNSSEC_VALIDATION_MAX) {
/* This reply requires an onerous number of signature validations to verify. Let's
* not waste our time trying, as this shouldn't happen for well-behaved domains
* anyway. */
t->answer_dnssec_result = DNSSEC_TOO_MANY_VALIDATIONS;
return 0;
}

/* Try again as long as we managed to achieve something */
if (r == 1)
continue;
Expand Down

0 comments on commit 899e3c4

Please sign in to comment.