From c4768e43cb45716223bb28569e164d407c9cc5ec Mon Sep 17 00:00:00 2001 From: Ryan Fowler Date: Sat, 8 Aug 2026 10:22:06 +0000 Subject: [PATCH] fix(dns): classify extended response codes --- src/dns/doh.rs | 186 +++++++++++++--------- src/dns/error.rs | 54 +++++++ src/dns/inspect.rs | 4 +- src/dns/mod.rs | 1 + src/dns/resolver.rs | 236 ++++++++++++++++++++-------- src/dns/svcb.rs | 2 +- src/dns/transport.rs | 63 +++++--- src/dns/wire.rs | 359 +++++++++++++++++++++++++++++++------------ tests/network.rs | 8 +- tests/websocket.rs | 6 +- 10 files changed, 656 insertions(+), 263 deletions(-) create mode 100644 src/dns/error.rs diff --git a/src/dns/doh.rs b/src/dns/doh.rs index 90178f66..6a7f94b4 100644 --- a/src/dns/doh.rs +++ b/src/dns/doh.rs @@ -31,19 +31,36 @@ const APPLICATION_DNS_MESSAGE: &str = "application/dns-message"; const APPLICATION_DNS_JSON: &str = "application/dns-json"; #[derive(Debug, Clone, PartialEq, Eq)] -pub struct DnsError(String); +pub struct DnsError { + kind: crate::dns::error::DnsErrorKind, + detail: Option, +} impl fmt::Display for DnsError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) + match &self.detail { + Some(detail) => f.write_str(detail), + None => self.kind.fmt(f), + } } } impl std::error::Error for DnsError {} impl DnsError { + fn dns(kind: crate::dns::error::DnsErrorKind) -> Self { + Self { kind, detail: None } + } + + fn other(detail: impl Into) -> Self { + Self { + kind: crate::dns::error::DnsErrorKind::Other, + detail: Some(detail.into()), + } + } + pub(crate) fn is_nxdomain(&self) -> bool { - self.0 == "no such host: NXDomain" + self.kind == crate::dns::error::DnsErrorKind::NxDomain } } @@ -79,7 +96,7 @@ pub async fn lookup_doh( lookup_doh_type_with_client(&client, server_url, host, "A", DNS_TYPE_A), lookup_doh_type_with_client(&client, server_url, host, "AAAA", DNS_TYPE_AAAA), crate::net::HAPPY_EYEBALLS_RESOLUTION_DELAY, - DnsError("no such host".to_string()), + DnsError::dns(crate::dns::error::DnsErrorKind::NoData), ) .await .map(|records| records.into_iter().map(|record| record.ip).collect()) @@ -111,14 +128,13 @@ pub(crate) fn client_with_budget_and_tls_config( let budget = dns_transaction_budget(budget); let tls_config = match tls_config { Some(config) => config, - None => { - crate::tls::rustls_platform_client_config().map_err(|err| DnsError(err.to_string()))? - } + None => crate::tls::rustls_platform_client_config() + .map_err(|err| DnsError::other(err.to_string()))?, }; let client = Client::builder() .tls_config(tls_config) .build() - .map_err(|err| DnsError(err.to_string()))?; + .map_err(|err| DnsError::other(err.to_string()))?; Ok(DohClient { budget, client }) } @@ -160,7 +176,7 @@ async fn lookup_doh_wire_records_with_client( // HTTP cache reuse and avoid carrying unnecessary entropy in the query. const DOH_QUERY_ID: u16 = 0; let query = wire::build_query(DOH_QUERY_ID, host, dns_type) - .map_err(|err| WireDohError::Fatal(DnsError(err.to_string())))?; + .map_err(|err| WireDohError::Fatal(DnsError::other(err.to_string())))?; let mut headers = HeaderMap::new(); headers.insert(ACCEPT, HeaderValue::from_static(APPLICATION_DNS_MESSAGE)); @@ -255,24 +271,34 @@ impl DohClient { ) -> Result { validate_doh_endpoint(&url)?; let wire_request = method == Method::POST; - self.budget - .run(Box::pin(async { - let mut request = self.client.request(method, url).headers(headers); - if let Some(body) = body { - request = request.body(body); - } - let response = Box::pin(request.send()).await?; - if wire_request && wire_status_may_support_json(response.status()) { - return Ok(DohResponseBody { - status: response.status(), - headers: response.headers().clone(), - body: Bytes::new(), - }); - } - Box::pin(buffer_doh_response(response, wire_request)).await - })) - .await - .map_err(|err| DnsError(err.to_string())) + let operation = Box::pin(async { + let mut request = self.client.request(method, url).headers(headers); + if let Some(body) = body { + request = request.body(body); + } + let response = Box::pin(request.send()).await?; + if wire_request && wire_status_may_support_json(response.status()) { + return Ok(DohResponseBody { + status: response.status(), + headers: response.headers().clone(), + body: Bytes::new(), + }); + } + Box::pin(buffer_doh_response(response, wire_request)).await + }); + let remaining = self + .budget + .remaining() + .map_err(|_| DnsError::dns(crate::dns::error::DnsErrorKind::Timeout))?; + match remaining { + Some(remaining) => tokio::time::timeout(remaining, operation) + .await + .map_err(|_| DnsError::dns(crate::dns::error::DnsErrorKind::Timeout))? + .map_err(|err| DnsError::other(err.to_string())), + None => operation + .await + .map_err(|err| DnsError::other(err.to_string())), + } } } @@ -312,7 +338,7 @@ fn validate_doh_endpoint(url: &Url) -> Result<(), DnsError> { { return Ok(()); } - Err(DnsError("DoH endpoints must use HTTPS".to_string())) + Err(DnsError::other("DoH endpoints must use HTTPS".to_string())) } async fn buffer_doh_response( @@ -366,24 +392,30 @@ fn doh_records_from_json_response( raw: &[u8], expected_name: &str, ) -> Result, DnsError> { - let body = - serde_json::from_slice::(raw).map_err(|err| DnsError(err.to_string()))?; + let body = serde_json::from_slice::(raw) + .map_err(|err| DnsError::other(err.to_string()))?; if body.status != 0 { - let name = rcode_name(body.status); - if name.is_empty() { - return Err(DnsError("no such host".to_string())); - } - return Err(DnsError(format!("no such host: {name}"))); + let rcode = u16::try_from(body.status) + .map_err(|_| DnsError::dns(crate::dns::error::DnsErrorKind::Malformed))?; + let kind = if rcode == 16 { + // JSON responses do not carry an OPT record. In this context, + // extended RCODE 16 is the TSIG status BADSIG, not EDNS BADVERS. + crate::dns::error::DnsErrorKind::BadSig + } else { + crate::dns::error::DnsErrorKind::from_rcode(rcode) + .expect("nonzero RCODE has an error kind") + }; + return Err(DnsError::dns(kind)); } - let expected = - wire::parse_presentation_name(expected_name).map_err(|err| DnsError(err.to_string()))?; + let expected = wire::parse_presentation_name(expected_name) + .map_err(|err| DnsError::other(err.to_string()))?; let owners = body .answer .iter() .map(|answer| wire::parse_presentation_name(&answer.name)) .collect::, _>>() - .map_err(|err| DnsError(err.to_string()))?; + .map_err(|err| DnsError::other(err.to_string()))?; let types = body .answer .iter() @@ -393,7 +425,7 @@ fn doh_records_from_json_response( wire::parse_presentation_name(&body.answer[index].data) .map_err(|_| wire::malformed_rdata(wire::TYPE_CNAME)) }) - .map_err(|err| DnsError(err.to_string()))?; + .map_err(|err| DnsError::other(err.to_string()))?; Ok(body .answer @@ -443,7 +475,7 @@ fn ip_records(records: Vec, answer_type: u16) -> Result Result, DnsError> { let records = wire::parse_response(raw, expected_id, expected_name, expected_type, DNS_CLASS_IN) - .map_err(|err| DnsError(err.to_string()))?; + .map_err(DnsError::from)?; let mut out = Vec::new(); for record in records { if record.class != DNS_CLASS_IN { @@ -474,7 +506,7 @@ fn doh_records_from_wire_response( fn wire_record_data(packet: &[u8], record: &wire::ResourceRecord<'_>) -> Result { let decoded = wire::decode_rdata(packet, record.typ, record.data_offset, record.data.len()) - .map_err(|err| DnsError(err.to_string()))?; + .map_err(|err| DnsError::other(err.to_string()))?; let value = match decoded { wire::DecodedRdata::Address(address) => address.to_string(), wire::DecodedRdata::Name(name) => name, @@ -524,14 +556,14 @@ fn doh_status_error(response: &DohResponseBody) -> DnsError { if let Ok(err_response) = serde_json::from_slice::(response.body()) && let Some(message) = err_response.error.filter(|message| !message.is_empty()) { - return DnsError(format!( + return DnsError::other(format!( "{}: {}", status.as_u16(), doh_error_excerpt(&message), )); } let body = String::from_utf8_lossy(response.body()); - DnsError(format!("{}: {}", status.as_u16(), doh_error_excerpt(&body),)) + DnsError::other(format!("{}: {}", status.as_u16(), doh_error_excerpt(&body),)) } fn doh_error_excerpt(body: &str) -> String { @@ -602,7 +634,10 @@ fn wire_status_may_support_json(status: StatusCode) -> bool { } fn is_dns_wire_error(err: &DnsError) -> bool { - err.0.starts_with("no such host") || err.0 == "DNS response was truncated" + !matches!( + err.kind, + crate::dns::error::DnsErrorKind::Other | crate::dns::error::DnsErrorKind::Malformed + ) } fn dns_type_code(dns_type: &str) -> Option { @@ -642,29 +677,17 @@ enum WireDohError { Fatal(DnsError), } -fn rcode_name(code: i32) -> &'static str { - match code { - 0 => "NoError", - 1 => "FormErr", - 2 => "ServFail", - 3 => "NXDomain", - 4 => "NotImp", - 5 => "Refused", - 6 => "YXDomain", - 7 => "YXRRSet", - 8 => "NXRRSet", - 9 => "NotAuth", - 10 => "NotZone", - 11 => "DSOTYPENI", - 16 => "BADSIG", - 17 => "BADKEY", - 18 => "BADTIME", - 19 => "BADMODE", - 20 => "BADNAME", - 21 => "BADALG", - 22 => "BADTRUNC", - 23 => "BADCOOKIE", - _ => "", +impl From for DnsError { + fn from(error: wire::WireError) -> Self { + let kind = error.kind(); + match kind { + crate::dns::error::DnsErrorKind::Other => Self::other(error.to_string()), + crate::dns::error::DnsErrorKind::Malformed => Self { + kind, + detail: Some(error.to_string()), + }, + _ => Self::dns(kind), + } } } @@ -1108,7 +1131,10 @@ mod tests { let err = doh_records_from_wire_response(&response, 0, "example.com", wire::TYPE_A).unwrap_err(); - assert_eq!(err.to_string(), "mismatched DNS response ID"); + assert_eq!( + err.to_string(), + "malformed DNS response: mismatched response ID" + ); } #[test] @@ -1130,6 +1156,20 @@ mod tests { assert!(records.is_empty()); } + #[test] + fn json_response_status_codes_use_json_context() { + let cases = [ + (16, crate::dns::error::DnsErrorKind::BadSig), + (99, crate::dns::error::DnsErrorKind::OtherRcode(99)), + ]; + + for (status, expected) in cases { + let raw = format!(r#"{{"Status":{status}}}"#); + let error = doh_records_from_json_response(raw.as_bytes(), "example.com").unwrap_err(); + assert_eq!(error.kind, expected); + } + } + #[test] fn json_response_rejects_unrelated_address_owner() { let raw = @@ -1407,7 +1447,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "request timed out after 5s"); + assert_eq!(err.to_string(), "DNS lookup timed out"); assert!( start.elapsed() < Duration::from_secs(7), "DoH response headers exceeded the default timeout" @@ -1424,7 +1464,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "request timed out after 5s"); + assert_eq!(err.to_string(), "DNS lookup timed out"); assert!( start.elapsed() < Duration::from_secs(7), "DoH response body exceeded the default timeout" @@ -1443,7 +1483,7 @@ mod tests { .unwrap_err(); let elapsed = start.elapsed(); - assert_eq!(err.to_string(), "request timed out after 250ms"); + assert_eq!(err.to_string(), "DNS lookup timed out"); assert!( elapsed < Duration::from_millis(350), "lookup took {elapsed:?}, expected timeout to cover POST and JSON fallback" @@ -1485,7 +1525,7 @@ mod tests { let err = lookup_doh(&url, "missing.example", None).await.unwrap_err(); - assert!(err.to_string().contains("NXDomain")); + assert!(err.to_string().contains("NXDOMAIN")); task.abort(); } diff --git a/src/dns/error.rs b/src/dns/error.rs new file mode 100644 index 00000000..f4f19dea --- /dev/null +++ b/src/dns/error.rs @@ -0,0 +1,54 @@ +use std::fmt; + +/// A DNS result category that callers can use without parsing display text. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum DnsErrorKind { + NxDomain, + NoData, + ServFail, + Refused, + FormErr, + NotImp, + BadVers, + BadSig, + Truncated, + Timeout, + Malformed, + OtherRcode(u16), + Other, +} + +impl DnsErrorKind { + pub(crate) fn from_rcode(rcode: u16) -> Option { + match rcode { + 0 => None, + 1 => Some(Self::FormErr), + 2 => Some(Self::ServFail), + 3 => Some(Self::NxDomain), + 4 => Some(Self::NotImp), + 5 => Some(Self::Refused), + 16 => Some(Self::BadVers), + other => Some(Self::OtherRcode(other)), + } + } +} + +impl fmt::Display for DnsErrorKind { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(match self { + Self::NxDomain => "DNS name does not exist (NXDOMAIN)", + Self::NoData => "DNS response contains no matching records (NODATA)", + Self::ServFail => "DNS server failure (SERVFAIL)", + Self::Refused => "DNS query was refused (REFUSED)", + Self::FormErr => "DNS server reported a format error (FORMERR)", + Self::NotImp => "DNS query is not implemented (NOTIMP)", + Self::BadVers => "DNS server rejected the EDNS version (BADVERS)", + Self::BadSig => "DNS signature validation failed (BADSIG)", + Self::Truncated => "DNS response was truncated", + Self::Timeout => "DNS lookup timed out", + Self::Malformed => "malformed DNS response", + Self::OtherRcode(code) => return write!(formatter, "DNS server returned RCODE {code}"), + Self::Other => "DNS lookup failed", + }) + } +} diff --git a/src/dns/inspect.rs b/src/dns/inspect.rs index f997dcd0..c8748435 100644 --- a/src/dns/inspect.rs +++ b/src/dns/inspect.rs @@ -1196,7 +1196,7 @@ mod tests { out.contains("warning: DNS queries for AAAA"), "output: {out}" ); - assert!(out.contains("NXDomain"), "output: {out}"); + assert!(out.contains("NXDOMAIN"), "output: {out}"); assert!(out.contains("results are incomplete"), "output: {out}"); task.abort(); } @@ -1839,7 +1839,7 @@ mod tests { .await .unwrap_err(); - assert!(err.to_string().contains("NXDomain")); + assert!(err.to_string().contains("NXDOMAIN")); task.abort(); } diff --git a/src/dns/mod.rs b/src/dns/mod.rs index dff72498..ca8d79a8 100644 --- a/src/dns/mod.rs +++ b/src/dns/mod.rs @@ -1,5 +1,6 @@ pub(crate) mod custom; pub mod doh; +pub(crate) mod error; pub mod inspect; pub mod resolver; pub(crate) mod svcb; diff --git a/src/dns/resolver.rs b/src/dns/resolver.rs index 1d414f63..901e29c7 100644 --- a/src/dns/resolver.rs +++ b/src/dns/resolver.rs @@ -17,19 +17,36 @@ const DNS_CLASS_IN: u16 = wire::CLASS_IN; pub(crate) const DOQ_MESSAGE_ID: u16 = 0; #[derive(Debug, Clone, PartialEq, Eq)] -pub struct ResolverError(String); +pub struct ResolverError { + kind: crate::dns::error::DnsErrorKind, + detail: Option, +} impl fmt::Display for ResolverError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) + match &self.detail { + Some(detail) => f.write_str(detail), + None => self.kind.fmt(f), + } } } impl std::error::Error for ResolverError {} impl ResolverError { + fn dns(kind: crate::dns::error::DnsErrorKind) -> Self { + Self { kind, detail: None } + } + + fn other(detail: impl Into) -> Self { + Self { + kind: crate::dns::error::DnsErrorKind::Other, + detail: Some(detail.into()), + } + } + pub(crate) fn is_nxdomain(&self) -> bool { - self.0 == "no such host: NXDomain" + self.kind == crate::dns::error::DnsErrorKind::NxDomain } } @@ -66,10 +83,18 @@ pub(crate) async fn lookup_udp_addr( let budget = TimeoutBudget::new(timeout); resolve_address_families( - lookup_udp_type_with_budget(addr, host, DNS_TYPE_A, budget), - lookup_udp_type_with_budget(addr, host, DNS_TYPE_AAAA, budget), + async { + query_udp_type(addr, host, DNS_TYPE_A, budget) + .await + .map(|records| collect_ip_records(records, DNS_TYPE_A)) + }, + async { + query_udp_type(addr, host, DNS_TYPE_AAAA, budget) + .await + .map(|records| collect_ip_records(records, DNS_TYPE_AAAA)) + }, crate::net::HAPPY_EYEBALLS_RESOLUTION_DELAY, - ResolverError("no such host".to_string()), + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), ) .await .map(|records| records.into_iter().map(|record| record.ip).collect()) @@ -92,7 +117,7 @@ async fn lookup_udp_type_with_budget( budget: TimeoutBudget, ) -> Result, ResolverError> { let records = query_udp_type(server_addr, host, dns_type, budget).await?; - Ok(ip_records(records, dns_type)) + ip_records(records, dns_type) } pub(crate) async fn query_udp_type( @@ -103,20 +128,20 @@ pub(crate) async fn query_udp_type( ) -> Result, ResolverError> { let budget = dns_transaction_budget(budget); let id = dns_query_id(); - let raw = wire::build_query(id, host, dns_type).map_err(resolver_error)?; + let raw = wire::build_query(id, host, dns_type).map_err(ResolverError::from)?; let matcher = wire::ResponseMatcher::new(id, host, dns_type, DNS_CLASS_IN); let response = crate::dns::transport::query_udp(*server_addr, &raw, &matcher, budget) .await - .map_err(resolver_error)?; + .map_err(ResolverError::from)?; match wire_records_from_response(&response, id, host, dns_type) { Ok(records) => Ok(records), Err(err) if err.is_truncated() => { let response = crate::dns::transport::query_tcp(*server_addr, &raw, budget) .await - .map_err(resolver_error)?; - wire_records_from_response(&response, id, host, dns_type).map_err(resolver_error) + .map_err(ResolverError::from)?; + wire_records_from_response(&response, id, host, dns_type).map_err(ResolverError::from) } - Err(err) => Err(resolver_error(err)), + Err(err) => Err(ResolverError::from(err)), } } @@ -133,15 +158,15 @@ pub(crate) async fn lookup_tcp_addr( async { query_tcp_type(addr, host, DNS_TYPE_A, budget) .await - .map(|records| ip_records(records, DNS_TYPE_A)) + .map(|records| collect_ip_records(records, DNS_TYPE_A)) }, async { query_tcp_type(addr, host, DNS_TYPE_AAAA, budget) .await - .map(|records| ip_records(records, DNS_TYPE_AAAA)) + .map(|records| collect_ip_records(records, DNS_TYPE_AAAA)) }, crate::net::HAPPY_EYEBALLS_RESOLUTION_DELAY, - ResolverError("no such host".to_string()), + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), ) .await .map(|records| records.into_iter().map(|record| record.ip).collect()) @@ -169,7 +194,7 @@ pub(crate) async fn lookup_tls( insecure, ) .await - .map(|records| ip_records(records, DNS_TYPE_A)) + .map(|records| collect_ip_records(records, DNS_TYPE_A)) }, async { query_tls_type( @@ -181,10 +206,10 @@ pub(crate) async fn lookup_tls( insecure, ) .await - .map(|records| ip_records(records, DNS_TYPE_AAAA)) + .map(|records| collect_ip_records(records, DNS_TYPE_AAAA)) }, crate::net::HAPPY_EYEBALLS_RESOLUTION_DELAY, - ResolverError("no such host".to_string()), + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), ) .await .map(|records| records.into_iter().map(|record| record.ip).collect()) @@ -201,7 +226,11 @@ pub(crate) async fn lookup_quic( return Ok(vec![ip]); } let budget = TimeoutBudget::new(timeout); - let connect_timeout = udp_dns_timeout(budget.remaining().map_err(resolver_error)?); + let connect_timeout = udp_dns_timeout( + budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?, + ); let connection = crate::dns::transport::quic_connection( server_name, server_addrs, @@ -209,7 +238,7 @@ pub(crate) async fn lookup_quic( insecure, ) .await - .map_err(resolver_error)?; + .map_err(ResolverError::from)?; run_quic_lookup(&connection, host, budget).await } @@ -223,7 +252,7 @@ pub(crate) async fn lookup_tcp_type( return Ok(vec![DnsRecord { ip, ttl: None }]); } let records = query_tcp_type(addr, host, dns_type, TimeoutBudget::new(Some(timeout))).await?; - Ok(ip_records(records, dns_type)) + ip_records(records, dns_type) } pub(crate) async fn query_tcp_type( @@ -232,10 +261,14 @@ pub(crate) async fn query_tcp_type( dns_type: u16, budget: TimeoutBudget, ) -> Result, ResolverError> { - let connect_timeout = udp_dns_timeout(budget.remaining().map_err(resolver_error)?); + let connect_timeout = udp_dns_timeout( + budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?, + ); let mut stream = crate::dns::transport::tcp_connection(addr, connect_timeout) .await - .map_err(resolver_error)?; + .map_err(ResolverError::from)?; query_stream_type(&mut stream, host, dns_type, budget).await } @@ -259,7 +292,7 @@ pub(crate) async fn lookup_tls_type( insecure, ) .await?; - Ok(ip_records(records, dns_type)) + ip_records(records, dns_type) } pub(crate) async fn query_tls_type( @@ -270,11 +303,15 @@ pub(crate) async fn query_tls_type( budget: TimeoutBudget, insecure: bool, ) -> Result, ResolverError> { - let connect_timeout = udp_dns_timeout(budget.remaining().map_err(resolver_error)?); + let connect_timeout = udp_dns_timeout( + budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?, + ); let mut stream = crate::dns::transport::tls_connection(server_name, server_addrs, connect_timeout, insecure) .await - .map_err(resolver_error)?; + .map_err(ResolverError::from)?; query_stream_type(&mut stream, host, dns_type, budget).await } @@ -298,7 +335,7 @@ pub(crate) async fn lookup_quic_type( insecure, ) .await?; - Ok(ip_records(records, dns_type)) + ip_records(records, dns_type) } pub(crate) async fn query_quic_type( @@ -309,7 +346,11 @@ pub(crate) async fn query_quic_type( budget: TimeoutBudget, insecure: bool, ) -> Result, ResolverError> { - let connect_timeout = udp_dns_timeout(budget.remaining().map_err(resolver_error)?); + let connect_timeout = udp_dns_timeout( + budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?, + ); let connection = crate::dns::transport::quic_connection( server_name, server_addrs, @@ -317,16 +358,19 @@ pub(crate) async fn query_quic_type( insecure, ) .await - .map_err(resolver_error)?; - let query = wire::build_query(DOQ_MESSAGE_ID, host, dns_type).map_err(resolver_error)?; - let timeout = budget.remaining().map_err(resolver_error)?; + .map_err(ResolverError::from)?; + let query = wire::build_query(DOQ_MESSAGE_ID, host, dns_type).map_err(ResolverError::from)?; + let timeout = budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?; let response = with_optional_timeout(timeout, async { crate::dns::transport::quic_query(&connection, &query) .await - .map_err(resolver_error) + .map_err(ResolverError::from) }) .await?; - wire_records_from_response(&response, DOQ_MESSAGE_ID, host, dns_type).map_err(resolver_error) + wire_records_from_response(&response, DOQ_MESSAGE_ID, host, dns_type) + .map_err(ResolverError::from) } async fn query_stream_type( @@ -336,21 +380,25 @@ async fn query_stream_type( budget: TimeoutBudget, ) -> Result, ResolverError> { let id = dns_query_id(); - let query = wire::build_query(id, host, dns_type).map_err(resolver_error)?; - let timeout = budget.remaining().map_err(resolver_error)?; + let query = wire::build_query(id, host, dns_type).map_err(ResolverError::from)?; + let timeout = budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?; let response = with_optional_timeout(timeout, async { crate::dns::transport::write_framed_query(stream, &query) .await - .map_err(resolver_error)?; + .map_err(ResolverError::from)?; crate::dns::transport::read_framed_response(stream) .await - .map_err(resolver_error) + .map_err(ResolverError::from) }) .await?; if response.len() < 2 { - return Err(ResolverError("short DNS response".to_string())); + return Err(ResolverError::dns( + crate::dns::error::DnsErrorKind::Malformed, + )); } - wire_records_from_response(&response, id, host, dns_type).map_err(resolver_error) + wire_records_from_response(&response, id, host, dns_type).map_err(ResolverError::from) } async fn run_quic_lookup( @@ -359,17 +407,20 @@ async fn run_quic_lookup( budget: TimeoutBudget, ) -> Result, ResolverError> { // RFC 9250 requires DNS message ID 0 for DoQ. - let query_a = wire::build_query(DOQ_MESSAGE_ID, host, DNS_TYPE_A).map_err(resolver_error)?; + let query_a = + wire::build_query(DOQ_MESSAGE_ID, host, DNS_TYPE_A).map_err(ResolverError::from)?; let query_aaaa = - wire::build_query(DOQ_MESSAGE_ID, host, DNS_TYPE_AAAA).map_err(resolver_error)?; + wire::build_query(DOQ_MESSAGE_ID, host, DNS_TYPE_AAAA).map_err(ResolverError::from)?; - let timeout = budget.remaining().map_err(resolver_error)?; + let timeout = budget + .remaining() + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))?; with_optional_timeout(timeout, async { resolve_address_families( quic_single_query(connection, query_a, DOQ_MESSAGE_ID, host, DNS_TYPE_A), quic_single_query(connection, query_aaaa, DOQ_MESSAGE_ID, host, DNS_TYPE_AAAA), crate::net::HAPPY_EYEBALLS_RESOLUTION_DELAY, - ResolverError("no such host".to_string()), + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), ) .await .map(|records| records.into_iter().map(|record| record.ip).collect()) @@ -386,7 +437,7 @@ where { tokio::time::timeout(udp_dns_timeout(timeout), fut) .await - .map_err(|_| ResolverError("DNS lookup timed out".to_string()))? + .map_err(|_| ResolverError::dns(crate::dns::error::DnsErrorKind::Timeout))? } async fn quic_single_query( @@ -398,14 +449,16 @@ async fn quic_single_query( ) -> Result, ResolverError> { let response = crate::dns::transport::quic_query(connection, &query) .await - .map_err(resolver_error)?; - dns_records_from_response(&response, expected_id, host, dns_type).map_err(resolver_error) + .map_err(ResolverError::from)?; + let records = wire_records_from_response(&response, expected_id, host, dns_type) + .map_err(ResolverError::from)?; + Ok(collect_ip_records(records, dns_type)) } fn parse_normalized_addr(server: &str) -> Result { normalize_udp_dns_server(server)? .parse::() - .map_err(|err| ResolverError(format!("invalid DNS server address: {err}"))) + .map_err(|err| ResolverError::other(format!("invalid DNS server address: {err}"))) } pub fn normalize_udp_dns_server(server: &str) -> Result { @@ -425,21 +478,21 @@ pub fn normalize_udp_dns_server(server: &str) -> Result { } fn dns_server_value_error(server: &str) -> ResolverError { - ResolverError(format!( + ResolverError::other(format!( "invalid value '{server}' for option '--dns-server': must be in the format " )) } +#[cfg(test)] fn dns_records_from_response( raw: &[u8], expected_id: u16, expected_name: &str, expected_type: u16, -) -> Result, wire::WireError> { - Ok(ip_records( - wire_records_from_response(raw, expected_id, expected_name, expected_type)?, - expected_type, - )) +) -> Result, ResolverError> { + let records = wire_records_from_response(raw, expected_id, expected_name, expected_type) + .map_err(ResolverError::from)?; + ip_records(records, expected_type) } fn wire_records_from_response( @@ -461,7 +514,19 @@ fn wire_records_from_response( ) } -fn ip_records(records: Vec, expected_type: u16) -> Vec { +fn ip_records( + records: Vec, + expected_type: u16, +) -> Result, ResolverError> { + let records = collect_ip_records(records, expected_type); + if records.is_empty() { + Err(ResolverError::dns(crate::dns::error::DnsErrorKind::NoData)) + } else { + Ok(records) + } +} + +fn collect_ip_records(records: Vec, expected_type: u16) -> Vec { records .into_iter() .filter(|record| record.typ == expected_type) @@ -490,8 +555,29 @@ fn ip_record(record: WireDnsRecord) -> Option { }) } -fn resolver_error(err: impl ToString) -> ResolverError { - ResolverError(err.to_string()) +impl From for ResolverError { + fn from(error: crate::dns::transport::DnsTransportError) -> Self { + match error.kind() { + crate::dns::error::DnsErrorKind::Timeout => { + Self::dns(crate::dns::error::DnsErrorKind::Timeout) + } + _ => Self::other(error.to_string()), + } + } +} + +impl From for ResolverError { + fn from(error: wire::WireError) -> Self { + let kind = error.kind(); + match kind { + crate::dns::error::DnsErrorKind::Other => Self::other(error.to_string()), + crate::dns::error::DnsErrorKind::Malformed => Self { + kind, + detail: Some(error.to_string()), + }, + _ => Self::dns(kind), + } + } } #[cfg(test)] @@ -512,6 +598,34 @@ mod tests { use tokio::net::TcpStream as TokioTcpStream; use tokio_rustls::{TlsAcceptor, server::TlsStream as ServerTlsStream}; + #[tokio::test] + async fn address_family_nodata_does_not_hide_other_family_errors() { + for expected in [ + crate::dns::error::DnsErrorKind::Timeout, + crate::dns::error::DnsErrorKind::ServFail, + ] { + let error = resolve_address_families( + std::future::ready(Ok::, ResolverError>(Vec::new())), + std::future::ready(Err(ResolverError::dns(expected))), + Duration::ZERO, + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), + ) + .await + .unwrap_err(); + assert_eq!(error.kind, expected); + } + + let error = resolve_address_families( + std::future::ready(Ok::, ResolverError>(Vec::new())), + std::future::ready(Ok(Vec::new())), + Duration::ZERO, + ResolverError::dns(crate::dns::error::DnsErrorKind::NoData), + ) + .await + .unwrap_err(); + assert_eq!(error.kind, crate::dns::error::DnsErrorKind::NoData); + } + #[tokio::test] async fn lookup_udp_returns_a_and_aaaa() { let (addr, stop) = start_udp_server(DnsServerMode::Success); @@ -629,7 +743,7 @@ mod tests { .await .unwrap_err(); - assert!(err.to_string().contains("NXDomain")); + assert!(err.to_string().contains("NXDOMAIN")); stop(); } @@ -651,9 +765,9 @@ mod tests { let response = test_response_with_answers(&[("unrelated.example", DNS_TYPE_A, vec![192, 0, 2, 1])]); - let records = - dns_records_from_response(&response, 0x1234, "example.com", DNS_TYPE_A).unwrap(); - assert!(records.is_empty()); + let error = + dns_records_from_response(&response, 0x1234, "example.com", DNS_TYPE_A).unwrap_err(); + assert_eq!(error.kind, crate::dns::error::DnsErrorKind::NoData); } #[test] @@ -1237,7 +1351,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "short DNS response"); + assert_eq!(err.to_string(), "malformed DNS response"); *done.lock().unwrap() = true; let _ = StdTcpStream::connect(addr); handle.join().unwrap(); diff --git a/src/dns/svcb.rs b/src/dns/svcb.rs index 6f092156..250c2a3b 100644 --- a/src/dns/svcb.rs +++ b/src/dns/svcb.rs @@ -1588,7 +1588,7 @@ mod tests { response.extend_from_slice(&query[..2]); response.extend_from_slice(&0x8183u16.to_be_bytes()); response.extend_from_slice(&1u16.to_be_bytes()); - response.extend_from_slice(&0u32.to_be_bytes()); + response.extend_from_slice(&[0; 6]); response.extend_from_slice(&query[12..question_end]); stream .write_all(&(response.len() as u16).to_be_bytes()) diff --git a/src/dns/transport.rs b/src/dns/transport.rs index e7dbcdae..a6c1120a 100644 --- a/src/dns/transport.rs +++ b/src/dns/transport.rs @@ -15,16 +15,42 @@ use crate::duration::TimeoutBudget; use crate::error::FetchError; #[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct DnsTransportError(String); +pub(crate) struct DnsTransportError { + kind: crate::dns::error::DnsErrorKind, + detail: Option, +} impl fmt::Display for DnsTransportError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) + match &self.detail { + Some(detail) => f.write_str(detail), + None => self.kind.fmt(f), + } } } impl std::error::Error for DnsTransportError {} +impl DnsTransportError { + fn timeout() -> Self { + Self { + kind: crate::dns::error::DnsErrorKind::Timeout, + detail: None, + } + } + + fn other(detail: impl Into) -> Self { + Self { + kind: crate::dns::error::DnsErrorKind::Other, + detail: Some(detail.into()), + } + } + + pub(crate) fn kind(&self) -> crate::dns::error::DnsErrorKind { + self.kind + } +} + const UDP_MAX_ATTEMPTS: usize = 3; const UDP_INITIAL_RETRANSMIT_DELAY: Duration = Duration::from_millis(100); const UDP_MAX_RETRANSMIT_DELAY: Duration = Duration::from_millis(250); @@ -120,7 +146,7 @@ fn udp_remaining(budget: TimeoutBudget) -> Result { } fn udp_timeout_error() -> DnsTransportError { - DnsTransportError("DNS lookup timed out".to_string()) + DnsTransportError::timeout() } pub(crate) async fn query_tcp( @@ -136,7 +162,7 @@ pub(crate) async fn query_tcp( read_framed_response(&mut stream).await }) .await - .map_err(|_| DnsTransportError("DNS lookup timed out".to_string()))? + .map_err(|_| DnsTransportError::timeout())? } pub(crate) async fn tcp_connection( @@ -145,7 +171,7 @@ pub(crate) async fn tcp_connection( ) -> Result { tokio::time::timeout(timeout, TcpStream::connect(server_addr)) .await - .map_err(|_| DnsTransportError("DNS lookup timed out".to_string()))? + .map_err(|_| DnsTransportError::timeout())? .map_err(transport_error) } @@ -154,7 +180,7 @@ pub(crate) async fn write_framed_query( query: &[u8], ) -> Result<(), DnsTransportError> { if query.len() > usize::from(u16::MAX) { - return Err(DnsTransportError("DNS query is too large".to_string())); + return Err(DnsTransportError::other("DNS query is too large")); } let mut framed = Vec::with_capacity(query.len() + 2); framed.extend_from_slice(&(query.len() as u16).to_be_bytes()); @@ -209,8 +235,8 @@ pub(crate) async fn tls_connection( ), ) .await - .map_err(|_| DnsTransportError("DNS lookup timed out".to_string()))? - .map_err(|err| DnsTransportError(err.to_string())) + .map_err(|_| DnsTransportError::timeout())? + .map_err(|err| DnsTransportError::other(err.to_string())) } async fn tls_connector(insecure: bool) -> Result { @@ -223,7 +249,7 @@ async fn tls_connector(insecure: bool) -> Result Result { @@ -281,7 +308,7 @@ fn quinn_client_endpoint() -> Result { Err(err) => { let fallback_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 0); quinn::Endpoint::client(fallback_addr).map_err(|fallback_err| { - DnsTransportError(format!( + DnsTransportError::other(format!( "failed to bind QUIC endpoint to {local_addr}: {err}; \ IPv4 fallback {fallback_addr} also failed: {fallback_err}" )) @@ -305,15 +332,15 @@ pub(crate) async fn quic_query( let (mut send, mut recv) = connection .open_bi() .await - .map_err(|err| DnsTransportError(format!("dns over quic open stream: {err}")))?; + .map_err(|err| DnsTransportError::other(format!("dns over quic open stream: {err}")))?; write_framed_query(&mut send, query).await?; send.finish() - .map_err(|err| DnsTransportError(format!("dns over quic finish stream: {err}")))?; + .map_err(|err| DnsTransportError::other(format!("dns over quic finish stream: {err}")))?; read_framed_response(&mut recv).await } fn transport_error(err: impl ToString) -> DnsTransportError { - DnsTransportError(err.to_string()) + DnsTransportError::other(err.to_string()) } #[cfg(test)] diff --git a/src/dns/wire.rs b/src/dns/wire.rs index 8b88bb8b..1d61587c 100644 --- a/src/dns/wire.rs +++ b/src/dns/wire.rs @@ -18,7 +18,6 @@ pub(crate) const TYPE_OPT: u16 = 41; pub(crate) const CLASS_IN: u16 = 1; pub(crate) const EDNS_UDP_PAYLOAD_SIZE: u16 = 1232; -const TRUNCATED_RESPONSE: &str = "DNS response was truncated"; const FLAG_RESPONSE: u16 = 0x8000; const FLAG_OPCODE: u16 = 0x7800; const FLAG_TRUNCATED: u16 = 0x0200; @@ -29,19 +28,50 @@ const MAX_NAME_LABELS: usize = 127; const MAX_NAME_POINTER_DEPTH: usize = 16; #[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct WireError(String); +pub(crate) enum WireError { + Response(crate::dns::error::DnsErrorKind), + Malformed(String), + Other(String), +} impl fmt::Display for WireError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) + match self { + Self::Response(kind) => kind.fmt(f), + Self::Malformed(detail) => write!(f, "malformed DNS response: {detail}"), + Self::Other(detail) => f.write_str(detail), + } } } impl std::error::Error for WireError {} impl WireError { + fn other(message: String) -> Self { + Self::Other(message) + } + + fn malformed(message: impl Into) -> Self { + Self::Malformed(message.into()) + } + + pub(crate) fn kind(&self) -> crate::dns::error::DnsErrorKind { + match self { + Self::Response(kind) => *kind, + Self::Malformed(_) => crate::dns::error::DnsErrorKind::Malformed, + Self::Other(_) => crate::dns::error::DnsErrorKind::Other, + } + } + pub(crate) fn is_truncated(&self) -> bool { - self.0 == TRUNCATED_RESPONSE + self.kind() == crate::dns::error::DnsErrorKind::Truncated + } + + fn into_response_error(self) -> Self { + match self { + Self::Other(detail) => Self::Malformed(detail), + error => error, + } } } @@ -96,7 +126,7 @@ pub(crate) fn decode_rdata<'a>( let end = offset .checked_add(len) .filter(|&end| end <= packet.len()) - .ok_or_else(|| WireError("short DNS resource".to_string()))?; + .ok_or_else(|| WireError::other("short DNS resource".to_string()))?; let raw = &packet[offset..end]; let mut reader = RdataReader { packet, @@ -166,7 +196,7 @@ pub(crate) fn decode_rdata<'a>( TYPE_CAA => Err(malformed_rdata(typ)), TYPE_SVCB | TYPE_HTTPS => crate::dns::svcb::parse_rdata(raw) .map(|_| DecodedRdata::Raw(raw)) - .map_err(|err| WireError(format!("malformed DNS RDATA for type {typ}: {err}"))), + .map_err(|err| WireError::other(format!("malformed DNS RDATA for type {typ}: {err}"))), _ => Ok(DecodedRdata::Raw(raw)), } } @@ -199,7 +229,7 @@ impl RdataReader<'_> { .pos .checked_add(len) .filter(|&end| end <= self.end) - .ok_or_else(|| WireError("short DNS RDATA".to_string()))?; + .ok_or_else(|| WireError::other("short DNS RDATA".to_string()))?; let bytes = &self.packet[self.pos..end]; self.pos = end; Ok(bytes) @@ -215,7 +245,7 @@ impl RdataReader<'_> { } pub(crate) fn malformed_rdata(typ: u16) -> WireError { - WireError(format!("malformed DNS RDATA for type {typ}")) + WireError::malformed(format!("DNS RDATA for type {typ}")) } fn parse_txt_rdata(raw: &[u8], typ: u16) -> Result { @@ -276,10 +306,25 @@ impl ResponseMatcher { let Ok(flags) = read_u16(raw, 2) else { return false; }; - if flags & FLAG_RESPONSE == 0 - || flags & FLAG_OPCODE != 0 - || !read_u16(raw, 4).is_ok_and(|count| count == 1) - { + if flags & FLAG_RESPONSE == 0 || flags & FLAG_OPCODE != 0 { + return false; + } + let Ok(question_count) = read_u16(raw, 4) else { + return false; + }; + if question_count == 0 { + // Some servers omit the question from an error response that they + // could not parse. Parse all sections so an OPT-only extended + // RCODE is also bound to this transaction. + return parse_response(raw, self.id, ".", self.typ, self.class).is_err_and(|error| { + !matches!( + error.kind(), + crate::dns::error::DnsErrorKind::Malformed + | crate::dns::error::DnsErrorKind::Other + ) + }); + } + if question_count != 1 { return false; } let Ok(question_name) = read_parsed_name_bounded(raw, 12, raw.len(), true) else { @@ -307,6 +352,7 @@ pub(crate) fn parse_response<'a>( expected_type, expected_class, ) + .map_err(WireError::into_response_error) } #[cfg(test)] @@ -317,6 +363,7 @@ pub(crate) fn parse_response_without_id<'a>( expected_class: u16, ) -> Result>, WireError> { parse_response_inner(raw, None, expected_name, expected_type, expected_class) + .map_err(WireError::into_response_error) } pub(crate) fn parse_standalone_resource_record( @@ -332,7 +379,7 @@ pub(crate) fn parse_standalone_resource_record( let end = data_offset .checked_add(rdlen) .filter(|end| *end == raw.len()) - .ok_or_else(|| WireError("malformed standalone DNS resource".to_string()))?; + .ok_or_else(|| WireError::other("malformed standalone DNS resource".to_string()))?; Ok(ResourceRecord { canonical_name: name.canonical, typ, @@ -351,56 +398,56 @@ fn parse_response_inner<'a>( expected_class: u16, ) -> Result>, WireError> { if raw.len() < 12 { - return Err(WireError("short DNS response".to_string())); + return Err(WireError::malformed("short header")); } if expected_id.is_some_and(|expected_id| read_u16(raw, 0).is_ok_and(|id| id != expected_id)) { - return Err(WireError("mismatched DNS response ID".to_string())); + return Err(WireError::malformed("mismatched response ID")); } let flags = read_u16(raw, 2)?; if flags & FLAG_RESPONSE == 0 { - return Err(WireError("DNS message is not a response".to_string())); + return Err(WireError::malformed("message is not a response")); } if flags & FLAG_OPCODE != 0 { - return Err(WireError("unexpected DNS response opcode".to_string())); + return Err(WireError::malformed("unexpected response opcode")); } if flags & FLAG_TRUNCATED != 0 { - return Err(WireError(TRUNCATED_RESPONSE.to_string())); - } - let rcode = i32::from(flags & 0x000f); - if rcode != 0 { - let name = rcode_name(rcode); - if name.is_empty() { - return Err(WireError("no such host".to_string())); - } - return Err(WireError(format!("no such host: {name}"))); + return Err(WireError::Response( + crate::dns::error::DnsErrorKind::Truncated, + )); } let question_count = usize::from(read_u16(raw, 4)?); let answer_count = usize::from(read_u16(raw, 6)?); - if question_count != 1 { - return Err(WireError("unexpected DNS question count".to_string())); - } - if answer_count > MAX_ANSWER_RECORDS { - return Err(WireError( - "DNS response has too many answer records".to_string(), - )); + let authority_count = usize::from(read_u16(raw, 8)?); + let additional_count = usize::from(read_u16(raw, 10)?); + if question_count > 1 { + return Err(WireError::malformed("unexpected question count")); } + let record_count = answer_count + .checked_add(authority_count) + .and_then(|count| count.checked_add(additional_count)) + .filter(|count| *count <= MAX_ANSWER_RECORDS) + .ok_or_else(|| WireError::malformed("too many resource records"))?; + let mut offset = 12; - let question_name = read_parsed_name_bounded(raw, offset, raw.len(), true)?; - offset = question_name.next; - let question_type = read_u16(raw, offset)?; - let question_class = read_u16(raw, offset + 2)?; - offset += 4; let expected_canonical = parse_presentation_name(expected_name)?; - if question_name.canonical != expected_canonical - || question_type != expected_type - || question_class != expected_class - { - return Err(WireError("mismatched DNS response question".to_string())); + if question_count == 1 { + let question_name = read_parsed_name_bounded(raw, offset, raw.len(), true)?; + offset = question_name.next; + let question_type = read_u16(raw, offset)?; + let question_class = read_u16(raw, offset + 2)?; + offset += 4; + if question_name.canonical != expected_canonical + || question_type != expected_type + || question_class != expected_class + { + return Err(WireError::malformed("mismatched response question")); + } } - let mut records = Vec::new(); - for _ in 0..answer_count { + let mut records = Vec::with_capacity(answer_count); + let mut extended_rcode = None; + for index in 0..record_count { let name = read_parsed_name_bounded(raw, offset, raw.len(), true)?; offset = name.next; let typ = read_u16(raw, offset)?; @@ -408,19 +455,42 @@ fn parse_response_inner<'a>( let ttl = read_u32(raw, offset + 4)?; let rdlen = usize::from(read_u16(raw, offset + 8)?); offset += 10; - if offset + rdlen > raw.len() { - return Err(WireError("short DNS resource".to_string())); + let end = offset + .checked_add(rdlen) + .filter(|end| *end <= raw.len()) + .ok_or_else(|| WireError::malformed("short resource record"))?; + + let in_answer = index < answer_count; + let in_additional = index >= answer_count + authority_count; + if typ == TYPE_OPT { + if !in_additional || !name.canonical.0.is_empty() || extended_rcode.is_some() { + return Err(WireError::malformed("invalid OPT record")); + } + extended_rcode = Some((ttl >> 24) as u8); + } else if in_answer { + records.push(ResourceRecord { + canonical_name: name.canonical, + typ, + class, + ttl, + data_offset: offset, + data: &raw[offset..end], + }); } - let data_offset = offset; - offset += rdlen; - records.push(ResourceRecord { - canonical_name: name.canonical, - typ, - class, - ttl, - data_offset, - data: &raw[data_offset..data_offset + rdlen], - }); + offset = end; + } + if offset != raw.len() { + return Err(WireError::malformed("trailing bytes")); + } + + let rcode = (flags & 0x000f) | (u16::from(extended_rcode.unwrap_or(0)) << 4); + if let Some(kind) = crate::dns::error::DnsErrorKind::from_rcode(rcode) { + return Err(WireError::Response(kind)); + } + if question_count == 0 { + return Err(WireError::malformed( + "successful response omitted the question", + )); } let owners = records @@ -478,7 +548,7 @@ pub(crate) fn reachable_answer_names( ) -> Result, WireError> { assert_eq!(owners.len(), types.len()); if owners.len() > MAX_ANSWER_RECORDS { - return Err(WireError( + return Err(WireError::other( "DNS response has too many answer records".to_string(), )); } @@ -511,19 +581,21 @@ pub(crate) fn reachable_answer_names( continue; } if owner.has_other_data { - return Err(WireError( + return Err(WireError::other( "DNS CNAME owner has conflicting answer data".to_string(), )); } if depth == MAX_CNAME_DEPTH { - return Err(WireError("DNS CNAME chain exceeds depth limit".to_string())); + return Err(WireError::other( + "DNS CNAME chain exceeds depth limit".to_string(), + )); } let mut target = None; for &index in &owner.cname_records { let parsed = cname_target(index)?; if target.as_ref().is_some_and(|prior| prior != &parsed) { - return Err(WireError( + return Err(WireError::other( "DNS CNAME owner has conflicting targets".to_string(), )); } @@ -532,7 +604,9 @@ pub(crate) fn reachable_answer_names( let target = target.expect("CNAME owner has at least one record"); if !reachable.insert(target.clone()) { - return Err(WireError("DNS CNAME chain contains a cycle".to_string())); + return Err(WireError::other( + "DNS CNAME chain contains a cycle".to_string(), + )); } pending.push_back(target); depth += 1; @@ -545,7 +619,7 @@ pub(crate) fn parse_presentation_name(name: &str) -> Result Result { if label.is_empty() { - return Err(WireError("invalid DNS name".to_string())); + return Err(WireError::other("invalid DNS name".to_string())); } labels.push(std::mem::take(&mut label)); offset += 1; @@ -567,7 +641,7 @@ pub(crate) fn parse_presentation_name(name: &str) -> Result { offset += 1; if offset == bytes.len() { - return Err(WireError("invalid DNS name escape".to_string())); + return Err(WireError::other("invalid DNS name escape".to_string())); } if offset + 2 < bytes.len() && bytes[offset..offset + 3].iter().all(u8::is_ascii_digit) @@ -576,7 +650,7 @@ pub(crate) fn parse_presentation_name(name: &str) -> Result u16::from(u8::MAX) { - return Err(WireError("invalid DNS name escape".to_string())); + return Err(WireError::other("invalid DNS name escape".to_string())); } label.push(value as u8); offset += 3; @@ -591,7 +665,7 @@ pub(crate) fn parse_presentation_name(name: &str) -> Result 63 { - return Err(WireError("invalid DNS name label".to_string())); + return Err(WireError::other("invalid DNS name label".to_string())); } } if !label.is_empty() { @@ -601,7 +675,7 @@ pub(crate) fn parse_presentation_name(name: &str) -> Result MAX_NAME_LABELS || labels.iter().map(|label| label.len() + 1).sum::() + 1 > MAX_ENCODED_NAME_LEN { - return Err(WireError("invalid DNS name".to_string())); + return Err(WireError::other("invalid DNS name".to_string())); } Ok(CanonicalName( @@ -625,7 +699,7 @@ fn read_parsed_name_bounded( allow_compression: bool, ) -> Result { if offset > end || end > packet.len() { - return Err(WireError("short DNS name".to_string())); + return Err(WireError::other("short DNS name".to_string())); } let mut labels = Vec::new(); let mut pos = offset; @@ -636,15 +710,17 @@ fn read_parsed_name_bounded( loop { if pos >= packet.len() || (!jumped && pos >= end) { - return Err(WireError("short DNS name".to_string())); + return Err(WireError::other("short DNS name".to_string())); } let len = packet[pos]; if len & 0xc0 == 0xc0 { if !allow_compression { - return Err(WireError("compressed DNS name is not allowed".to_string())); + return Err(WireError::other( + "compressed DNS name is not allowed".to_string(), + )); } if pos + 1 >= end { - return Err(WireError("short DNS name pointer".to_string())); + return Err(WireError::other("short DNS name pointer".to_string())); } if !jumped { next = pos + 2; @@ -654,14 +730,14 @@ fn read_parsed_name_bounded( jumped = true; pointer_depth += 1; if pointer_depth > MAX_NAME_POINTER_DEPTH { - return Err(WireError( + return Err(WireError::other( "DNS name pointer depth exceeds limit".to_string(), )); } continue; } if len & 0xc0 != 0 { - return Err(WireError("invalid DNS name label".to_string())); + return Err(WireError::other("invalid DNS name label".to_string())); } pos += 1; if len == 0 { @@ -672,14 +748,16 @@ fn read_parsed_name_bounded( } let len = usize::from(len); if pos + len > end { - return Err(WireError("short DNS name label".to_string())); + return Err(WireError::other("short DNS name label".to_string())); } if labels.len() == MAX_NAME_LABELS { - return Err(WireError("DNS name exceeds label count limit".to_string())); + return Err(WireError::other( + "DNS name exceeds label count limit".to_string(), + )); } expanded_len += len + 1; if expanded_len > MAX_ENCODED_NAME_LEN { - return Err(WireError( + return Err(WireError::other( "DNS name exceeds expanded length limit".to_string(), )); } @@ -738,27 +816,27 @@ fn format_label(label: &[u8]) -> String { pub(crate) fn read_u16(raw: &[u8], offset: usize) -> Result { let bytes = raw .get(offset..offset + 2) - .ok_or_else(|| WireError("short DNS message".to_string()))?; + .ok_or_else(|| WireError::other("short DNS message".to_string()))?; Ok(u16::from_be_bytes([bytes[0], bytes[1]])) } pub(crate) fn read_u32(raw: &[u8], offset: usize) -> Result { let bytes = raw .get(offset..offset + 4) - .ok_or_else(|| WireError("short DNS message".to_string()))?; + .ok_or_else(|| WireError::other("short DNS message".to_string()))?; Ok(u32::from_be_bytes([bytes[0], bytes[1], bytes[2], bytes[3]])) } pub(crate) fn write_name(raw: &mut Vec, host: &str) -> Result<(), WireError> { if host.is_empty() { - return Err(WireError("invalid DNS name: empty name".to_string())); + return Err(WireError::other("invalid DNS name: empty name".to_string())); } if host == "." { raw.push(0); return Ok(()); } let name = parse_presentation_name(host) - .map_err(|_| WireError(format!("invalid DNS name: {host}")))?; + .map_err(|_| WireError::other(format!("invalid DNS name: {host}")))?; for label in name.0 { raw.push(label.len() as u8); raw.extend_from_slice(&label); @@ -775,17 +853,6 @@ fn write_opt_record(raw: &mut Vec) { raw.extend_from_slice(&0u16.to_be_bytes()); } -fn rcode_name(status: i32) -> &'static str { - match status { - 1 => "FormatError", - 2 => "ServerFailure", - 3 => "NXDomain", - 4 => "NotImplemented", - 5 => "Refused", - _ => "", - } -} - #[cfg(test)] mod tests { use super::*; @@ -794,7 +861,10 @@ mod tests { fn rejects_query_packet_as_response() { let query = build_query(0x1234, "example.com", TYPE_A).unwrap(); let err = parse_response(&query, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); - assert_eq!(err.to_string(), "DNS message is not a response"); + assert_eq!( + err.to_string(), + "malformed DNS response: message is not a response" + ); } #[test] @@ -802,7 +872,10 @@ mod tests { let mut response = build_query(0x1234, "other.example", TYPE_A).unwrap(); response[2..4].copy_from_slice(&0x8180u16.to_be_bytes()); let err = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); - assert_eq!(err.to_string(), "mismatched DNS response question"); + assert_eq!( + err.to_string(), + "malformed DNS response: mismatched response question" + ); } #[test] @@ -900,7 +973,7 @@ mod tests { ]); let err = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); - assert!(err.to_string().contains("malformed DNS RDATA")); + assert!(err.to_string().contains("malformed DNS response")); } #[test] @@ -1027,7 +1100,10 @@ mod tests { let err = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); - assert_eq!(err.to_string(), "DNS CNAME chain exceeds depth limit"); + assert_eq!( + err.to_string(), + "malformed DNS response: DNS CNAME chain exceeds depth limit" + ); } #[test] @@ -1038,7 +1114,10 @@ mod tests { let err = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); - assert_eq!(err.to_string(), "DNS response has too many answer records"); + assert_eq!( + err.to_string(), + "malformed DNS response: too many resource records" + ); } fn cname_chain(depth: usize) -> Vec<(Vec, u16, Vec)> { @@ -1055,6 +1134,92 @@ mod tests { } } + #[test] + fn response_codes_are_structured_and_include_edns_extended_bits() { + let cases: [(u16, crate::dns::error::DnsErrorKind); 7] = [ + (1, crate::dns::error::DnsErrorKind::FormErr), + (2, crate::dns::error::DnsErrorKind::ServFail), + (3, crate::dns::error::DnsErrorKind::NxDomain), + (4, crate::dns::error::DnsErrorKind::NotImp), + (5, crate::dns::error::DnsErrorKind::Refused), + (16, crate::dns::error::DnsErrorKind::BadVers), + (23, crate::dns::error::DnsErrorKind::OtherRcode(23)), + ]; + + for (rcode, expected) in cases { + let query = build_query(0x1234, "example.com", TYPE_A).unwrap(); + let (_, question_end) = read_name(&query, 12).unwrap(); + let mut response = Vec::new(); + response.extend_from_slice(&0x1234u16.to_be_bytes()); + response.extend_from_slice(&(0x8180 | (rcode & 0x0f)).to_be_bytes()); + response.extend_from_slice(&1u16.to_be_bytes()); + response.extend_from_slice(&0u16.to_be_bytes()); + response.extend_from_slice(&0u16.to_be_bytes()); + response.extend_from_slice(&1u16.to_be_bytes()); + response.extend_from_slice(&query[12..question_end + 4]); + response.push(0); + response.extend_from_slice(&TYPE_OPT.to_be_bytes()); + response.extend_from_slice(&EDNS_UDP_PAYLOAD_SIZE.to_be_bytes()); + response.extend_from_slice(&(u32::from(rcode >> 4) << 24).to_be_bytes()); + response.extend_from_slice(&0u16.to_be_bytes()); + + let error = + parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); + assert_eq!(error.kind(), expected, "RCODE {rcode}"); + } + } + + #[test] + fn error_response_can_omit_the_question() { + let mut response = vec![0; 12]; + response[0..2].copy_from_slice(&0x1234u16.to_be_bytes()); + response[2..4].copy_from_slice(&0x8181u16.to_be_bytes()); + + let error = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); + assert_eq!(error.kind(), crate::dns::error::DnsErrorKind::FormErr); + assert!(ResponseMatcher::new(0x1234, "example.com", TYPE_A, CLASS_IN).matches(&response)); + + response[2..4].copy_from_slice(&0x8180u16.to_be_bytes()); + response[10..12].copy_from_slice(&1u16.to_be_bytes()); + response.push(0); + response.extend_from_slice(&TYPE_OPT.to_be_bytes()); + response.extend_from_slice(&EDNS_UDP_PAYLOAD_SIZE.to_be_bytes()); + response.extend_from_slice(&(1u32 << 24).to_be_bytes()); + response.extend_from_slice(&0u16.to_be_bytes()); + let error = parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); + assert_eq!(error.kind(), crate::dns::error::DnsErrorKind::BadVers); + assert!(ResponseMatcher::new(0x1234, "example.com", TYPE_A, CLASS_IN).matches(&response)); + } + + #[test] + fn malformed_opt_records_are_rejected_before_rcode_classification() { + for mutate in ["non-root owner", "duplicate"] { + let query = build_query(0x1234, "example.com", TYPE_A).unwrap(); + let (_, question_end) = read_name(&query, 12).unwrap(); + let mut response = query[..question_end + 4].to_vec(); + response[2..4].copy_from_slice(&0x8180u16.to_be_bytes()); + response[6..10].fill(0); + response[10..12] + .copy_from_slice(&(if mutate == "duplicate" { 2u16 } else { 1u16 }).to_be_bytes()); + let owner = if mutate == "non-root owner" { + &[1, b'x', 0][..] + } else { + &[0][..] + }; + for _ in 0..if mutate == "duplicate" { 2 } else { 1 } { + response.extend_from_slice(owner); + response.extend_from_slice(&TYPE_OPT.to_be_bytes()); + response.extend_from_slice(&EDNS_UDP_PAYLOAD_SIZE.to_be_bytes()); + response.extend_from_slice(&0u32.to_be_bytes()); + response.extend_from_slice(&0u16.to_be_bytes()); + } + + let error = + parse_response(&response, 0x1234, "example.com", TYPE_A, CLASS_IN).unwrap_err(); + assert_eq!(error.kind(), crate::dns::error::DnsErrorKind::Malformed); + } + } + fn response_with_answers(answers: &[(Vec, u16, Vec)]) -> Vec { let query = build_query(0x1234, "example.com", TYPE_A).unwrap(); let (_, question_end) = read_name(&query, 12).unwrap(); diff --git a/tests/network.rs b/tests/network.rs index 6de4f211..9343b6ce 100644 --- a/tests/network.rs +++ b/tests/network.rs @@ -167,11 +167,7 @@ fn ech_dns_discovery_failure_is_reported_and_auto_falls_back() { &url, ]); assert_exit(&required, 1); - assert!( - required.stderr.contains("ServerFailure"), - "{}", - required.stderr - ); + assert!(required.stderr.contains("SERVFAIL"), "{}", required.stderr); assert!(!required.stderr.contains("does not advertise ECH")); let inspected = run_fetch(&[ @@ -1636,7 +1632,7 @@ fn dns_over_https_udp_and_inspect_dns_cases() { doh.ca_cert_path.to_str().unwrap(), ]); assert_exit(&res, 1); - assert!(res.stderr.contains("no such host")); + assert!(res.stderr.contains("NXDOMAIN")); assert!(!res.stderr.contains("For more information")); // rustls-native-certs honors SSL_CERT_FILE on Linux. The macOS and diff --git a/tests/websocket.rs b/tests/websocket.rs index c0a20c56..8ebbb3a0 100644 --- a/tests/websocket.rs +++ b/tests/websocket.rs @@ -1305,11 +1305,7 @@ fn websocket_ech_discovery_uses_http_error_policy() { "off", ]); assert_exit(&required, 1); - assert!( - required.stderr.contains("ServerFailure"), - "{}", - required.stderr - ); + assert!(required.stderr.contains("SERVFAIL"), "{}", required.stderr); assert!(!required.stderr.contains("does not advertise ECH")); let automatic = run_fetch(&[