diff --git a/src/dns/doh.rs b/src/dns/doh.rs index d5b3bac..23b9b08 100644 --- a/src/dns/doh.rs +++ b/src/dns/doh.rs @@ -189,7 +189,7 @@ async fn lookup_doh_wire_records_with_client( return Err(WireDohError::Fallback); } - match doh_records_from_wire_response(response.body(), id) { + match doh_records_from_wire_response(response.body(), id, host, dns_type) { Ok(records) => Ok(records), Err(err) if is_dns_message_response(&response) || is_dns_wire_error(&err) => { Err(WireDohError::Fatal(err)) @@ -345,9 +345,12 @@ fn ip_records(records: Vec, answer_type: u16) -> Result Result, DnsError> { let records = - wire::parse_response(raw, expected_id).map_err(|err| DnsError(err.to_string()))?; + wire::parse_response(raw, expected_id, expected_name, expected_type, DNS_CLASS_IN) + .map_err(|err| DnsError(err.to_string()))?; let mut out = Vec::new(); for record in records { if record.class != DNS_CLASS_IN { @@ -355,14 +358,14 @@ fn doh_records_from_wire_response( } out.push(DohRecord { answer_type: record.typ, - data: wire_record_data(raw, record)?, + data: wire_record_data(raw, &record)?, ttl: Some(record.ttl), }); } Ok(out) } -fn wire_record_data(packet: &[u8], record: wire::ResourceRecord<'_>) -> Result { +fn wire_record_data(packet: &[u8], record: &wire::ResourceRecord<'_>) -> Result { let offset = record.data_offset; let len = record.data.len(); let rdata = record.data; @@ -915,6 +918,25 @@ mod tests { response } + #[test] + fn wire_response_rejects_unrelated_answer_owner() { + let query = wire::build_query(0x1234, "example.com", wire::TYPE_A).unwrap(); + let (_, _, question_end) = test_dns_question(&query).unwrap(); + let mut response = wire_response(&query, vec![(wire::TYPE_A, 30, vec![192, 0, 2, 1])]); + let mut owner = Vec::new(); + for label in "unrelated.example".split('.') { + owner.push(label.len() as u8); + owner.extend_from_slice(label.as_bytes()); + } + owner.push(0); + response.splice(question_end..question_end + 2, owner); + + let records = + doh_records_from_wire_response(&response, 0x1234, "example.com", wire::TYPE_A).unwrap(); + + assert!(records.is_empty()); + } + #[tokio::test] async fn lookup_doh_uses_rfc8484_wire_format() { let seen = Arc::new(Mutex::new(Vec::new())); diff --git a/src/dns/inspect.rs b/src/dns/inspect.rs index a50f4a7..aa6aa43 100644 --- a/src/dns/inspect.rs +++ b/src/dns/inspect.rs @@ -480,16 +480,18 @@ async fn lookup_udp_records( let mut response = crate::dns::transport::query_udp(server_addr, &raw, udp_timeout) .await .map_err(QueryError::other)?; - let raw_records = match wire::parse_response(&response, id) { - Ok(records) => records, - Err(err) if err.is_truncated() => { - response = crate::dns::transport::query_tcp(server_addr, &raw, timeout) - .await - .map_err(QueryError::truncated)?; - wire::parse_response(&response, id).map_err(QueryError::truncated)? - } - Err(err) => return Err(QueryError::other(err)), - }; + let raw_records = + match wire::parse_response(&response, id, host, query_type.dns_type, DNS_CLASS_IN) { + Ok(records) => records, + Err(err) if err.is_truncated() => { + response = crate::dns::transport::query_tcp(server_addr, &raw, timeout) + .await + .map_err(QueryError::truncated)?; + wire::parse_response(&response, id, host, query_type.dns_type, DNS_CLASS_IN) + .map_err(QueryError::truncated)? + } + Err(err) => return Err(QueryError::other(err)), + }; records_from_wire_response(&response, raw_records) } @@ -522,7 +524,7 @@ async fn lookup_tcp_records( }) .await .map_err(|_| QueryError::other("DNS lookup timed out"))??; - inspect_records_from_response(&response, id) + inspect_records_from_response(&response, id, host, query_type.dns_type) } /// Query a single record type over a fresh TCP+TLS connection. @@ -556,7 +558,7 @@ async fn lookup_tls_records( }) .await .map_err(|_| QueryError::other("DNS lookup timed out"))??; - inspect_records_from_response(&response, id) + inspect_records_from_response(&response, id, host, query_type.dns_type) } /// Query a single record type over a fresh QUIC connection. @@ -592,14 +594,28 @@ async fn lookup_quic_records( .await .map_err(|_| QueryError::other("DNS lookup timed out"))? .map_err(QueryError::other)?; - inspect_records_from_response(&response, crate::dns::resolver::DOQ_MESSAGE_ID) + inspect_records_from_response( + &response, + crate::dns::resolver::DOQ_MESSAGE_ID, + host, + query_type.dns_type, + ) } fn inspect_records_from_response( response: &[u8], expected_id: u16, + expected_name: &str, + expected_type: u16, ) -> Result, QueryError> { - let raw_records = wire::parse_response(response, expected_id).map_err(QueryError::other)?; + let raw_records = wire::parse_response( + response, + expected_id, + expected_name, + expected_type, + DNS_CLASS_IN, + ) + .map_err(QueryError::other)?; records_from_wire_response(response, raw_records) } diff --git a/src/dns/resolver.rs b/src/dns/resolver.rs index 3de3a7b..78e9e88 100644 --- a/src/dns/resolver.rs +++ b/src/dns/resolver.rs @@ -80,13 +80,13 @@ async fn lookup_udp_type_with_budget( let response = crate::dns::transport::query_udp(*server_addr, &raw, timeout) .await .map_err(resolver_error)?; - match dns_records_from_response(&response, id) { + match dns_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)?; - dns_records_from_response(&response, id).map_err(resolver_error) + dns_records_from_response(&response, id, host, dns_type).map_err(resolver_error) } Err(err) => Err(resolver_error(err)), } @@ -216,7 +216,7 @@ pub(crate) async fn lookup_quic_type( .map_err(resolver_error) }) .await?; - dns_records_from_response(&response, DOQ_MESSAGE_ID).map_err(resolver_error) + dns_records_from_response(&response, DOQ_MESSAGE_ID, host, dns_type).map_err(resolver_error) } async fn lookup_stream_type( @@ -240,7 +240,7 @@ async fn lookup_stream_type( if response.len() < 2 { return Err(ResolverError("short DNS response".to_string())); } - dns_records_from_response(&response, id).map_err(resolver_error) + dns_records_from_response(&response, id, host, dns_type).map_err(resolver_error) } async fn run_stream_lookup( @@ -286,8 +286,10 @@ async fn run_stream_lookup( let a_response = a_response.ok_or_else(|| ResolverError("missing DNS response".to_string()))?; let aaaa_response = aaaa_response.ok_or_else(|| ResolverError("missing DNS response".to_string()))?; - let a_records = dns_records_from_response(&a_response, id_a).map_err(resolver_error); - let aaaa_records = dns_records_from_response(&aaaa_response, id_aaaa).map_err(resolver_error); + let a_records = + dns_records_from_response(&a_response, id_a, host, DNS_TYPE_A).map_err(resolver_error); + let aaaa_records = dns_records_from_response(&aaaa_response, id_aaaa, host, DNS_TYPE_AAAA) + .map_err(resolver_error); combine_results(a_records, aaaa_records) } @@ -304,8 +306,8 @@ async fn run_quic_lookup( let timeout = budget.remaining().map_err(resolver_error)?; let (a_records, aaaa_records) = with_optional_timeout(timeout, async { let (a_result, aaaa_result) = tokio::join!( - quic_single_query(connection, query_a, DOQ_MESSAGE_ID), - quic_single_query(connection, query_aaaa, DOQ_MESSAGE_ID), + 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), ); Ok::<_, ResolverError>((a_result, aaaa_result)) }) @@ -330,11 +332,13 @@ async fn quic_single_query( connection: &quinn::Connection, query: Vec, expected_id: u16, + host: &str, + dns_type: u16, ) -> Result, ResolverError> { let response = crate::dns::transport::quic_query(connection, &query) .await .map_err(resolver_error)?; - dns_records_from_response(&response, expected_id).map_err(resolver_error) + dns_records_from_response(&response, expected_id, host, dns_type).map_err(resolver_error) } fn combine_results( @@ -388,9 +392,17 @@ fn dns_server_value_error(server: &str) -> ResolverError { fn dns_records_from_response( raw: &[u8], expected_id: u16, + expected_name: &str, + expected_type: u16, ) -> Result, wire::WireError> { - let records = wire::parse_response(raw, expected_id)?; - Ok(records.into_iter().filter_map(ip_record).collect()) + let records = + wire::parse_response(raw, expected_id, expected_name, expected_type, DNS_CLASS_IN)?; + + Ok(records + .into_iter() + .filter(|record| record.typ == expected_type) + .filter_map(ip_record) + .collect()) } fn ip_record(record: wire::ResourceRecord<'_>) -> Option { @@ -556,6 +568,30 @@ mod tests { drop(socket); } + #[test] + fn response_rejects_unrelated_address_owner() { + 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()); + } + + #[test] + fn response_accepts_address_through_cname_chain() { + let mut cname = Vec::new(); + write_test_name(&mut cname, "alias.example"); + let response = test_response_with_answers(&[ + ("example.com", wire::TYPE_CNAME, cname), + ("alias.example", DNS_TYPE_A, vec![192, 0, 2, 1]), + ]); + + let records = + dns_records_from_response(&response, 0x1234, "example.com", DNS_TYPE_A).unwrap(); + assert_eq!(records[0].ip, "192.0.2.1".parse::().unwrap()); + } + #[test] fn normalize_udp_dns_server_matches_go_parser() { assert_eq!( @@ -1167,6 +1203,35 @@ mod tests { Some(query) } + fn test_response_with_answers(answers: &[(&str, u16, Vec)]) -> Vec { + let query = wire::build_query(0x1234, "example.com", DNS_TYPE_A).unwrap(); + let question_end = question_end(&query).unwrap() + 4; + let mut response = Vec::new(); + response.extend_from_slice(&0x1234u16.to_be_bytes()); + response.extend_from_slice(&0x8180u16.to_be_bytes()); + response.extend_from_slice(&1u16.to_be_bytes()); + response.extend_from_slice(&(answers.len() as u16).to_be_bytes()); + response.extend_from_slice(&0u32.to_be_bytes()); + response.extend_from_slice(&query[12..question_end]); + for (name, typ, data) in answers { + write_test_name(&mut response, name); + response.extend_from_slice(&typ.to_be_bytes()); + response.extend_from_slice(&DNS_CLASS_IN.to_be_bytes()); + response.extend_from_slice(&60u32.to_be_bytes()); + response.extend_from_slice(&(data.len() as u16).to_be_bytes()); + response.extend_from_slice(data); + } + response + } + + fn write_test_name(raw: &mut Vec, name: &str) { + for label in name.trim_end_matches('.').split('.') { + raw.push(label.len() as u8); + raw.extend_from_slice(label.as_bytes()); + } + raw.push(0); + } + fn write_answer(response: &mut Vec, dns_type: u16, ttl: u32, data: &[u8]) { response.extend_from_slice(&[0xc0, 0x0c]); response.extend_from_slice(&dns_type.to_be_bytes()); diff --git a/src/dns/svcb.rs b/src/dns/svcb.rs index 4f4d4a7..5a4cae1 100644 --- a/src/dns/svcb.rs +++ b/src/dns/svcb.rs @@ -304,13 +304,14 @@ async fn lookup_udp_https_records( let mut response = crate::dns::transport::query_udp(server_addr, &raw, udp_timeout) .await .map_err(|err| FetchError::Runtime(err.to_string()))?; - let raw_records = match wire::parse_response(&response, id) { + let raw_records = match wire::parse_response(&response, id, host, DNS_TYPE_HTTPS, DNS_CLASS_IN) + { Ok(records) => records, Err(err) if err.is_truncated() => { response = crate::dns::transport::query_tcp(server_addr, &raw, timeout) .await .map_err(|err| FetchError::Runtime(err.to_string()))?; - wire::parse_response(&response, id) + wire::parse_response(&response, id, host, DNS_TYPE_HTTPS, DNS_CLASS_IN) .map_err(|err| FetchError::Runtime(err.to_string()))? } Err(err) => return Err(FetchError::Runtime(err.to_string())), diff --git a/src/dns/svcb/system.rs b/src/dns/svcb/system.rs index cbb702d..d26ac1c 100644 --- a/src/dns/svcb/system.rs +++ b/src/dns/svcb/system.rs @@ -411,7 +411,8 @@ fn lookup_https_records_blocking( #[cfg(test)] fn records_from_wire_response(raw: &[u8]) -> Result, FetchError> { let records = - wire::parse_response_without_id(raw).map_err(|err| FetchError::Runtime(err.to_string()))?; + wire::parse_response_without_id(raw, "example.com", wire::TYPE_HTTPS, wire::CLASS_IN) + .map_err(|err| FetchError::Runtime(err.to_string()))?; Ok(records .into_iter() .filter(|record| record.class == wire::CLASS_IN && record.typ == wire::TYPE_HTTPS) @@ -446,7 +447,10 @@ fn write_dns_name(out: &mut Vec, name: &str) -> Option<()> { mod tests { use super::*; - fn dns_response_without_matching_query_id(answer_rdata: Vec) -> Vec { + fn dns_response_without_matching_query_id( + answer_owner: &str, + answer_rdata: Vec, + ) -> Vec { let mut out = Vec::new(); out.extend_from_slice(&0x1234_u16.to_be_bytes()); out.extend_from_slice(&0x8180_u16.to_be_bytes()); @@ -457,7 +461,7 @@ mod tests { write_dns_name(&mut out, "example.com.").unwrap(); out.extend_from_slice(&wire::TYPE_HTTPS.to_be_bytes()); out.extend_from_slice(&wire::CLASS_IN.to_be_bytes()); - out.extend_from_slice(&[0xc0, 0x0c]); + write_dns_name(&mut out, answer_owner).unwrap(); out.extend_from_slice(&wire::TYPE_HTTPS.to_be_bytes()); out.extend_from_slice(&wire::CLASS_IN.to_be_bytes()); out.extend_from_slice(&30_u32.to_be_bytes()); @@ -478,7 +482,7 @@ mod tests { #[test] fn parses_system_wire_response_without_generated_query_id() { - let raw = dns_response_without_matching_query_id(https_rdata()); + let raw = dns_response_without_matching_query_id("example.com.", https_rdata()); let records = records_from_wire_response(&raw).unwrap(); @@ -487,4 +491,13 @@ mod tests { assert_eq!(records[0].target, "."); assert_eq!(records[0].alpn, ["h3"]); } + + #[test] + fn rejects_unrelated_https_answer_owner() { + let raw = dns_response_without_matching_query_id("unrelated.example.", https_rdata()); + + let records = records_from_wire_response(&raw).unwrap(); + + assert!(records.is_empty()); + } } diff --git a/src/dns/wire.rs b/src/dns/wire.rs index f0c483b..a594b58 100644 --- a/src/dns/wire.rs +++ b/src/dns/wire.rs @@ -16,6 +16,8 @@ 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; #[derive(Debug, Clone, PartialEq, Eq)] @@ -35,8 +37,9 @@ impl WireError { } } -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone)] pub(crate) struct ResourceRecord<'a> { + pub(crate) name: String, pub(crate) typ: u16, pub(crate) class: u16, pub(crate) ttl: u32, @@ -62,18 +65,35 @@ pub(crate) fn build_query(id: u16, host: &str, dns_type: u16) -> Result, pub(crate) fn parse_response<'a>( raw: &'a [u8], expected_id: u16, + expected_name: &str, + expected_type: u16, + expected_class: u16, ) -> Result>, WireError> { - parse_response_inner(raw, Some(expected_id)) + parse_response_inner( + raw, + Some(expected_id), + expected_name, + expected_type, + expected_class, + ) } #[cfg(test)] -pub(crate) fn parse_response_without_id(raw: &[u8]) -> Result>, WireError> { - parse_response_inner(raw, None) +pub(crate) fn parse_response_without_id<'a>( + raw: &'a [u8], + expected_name: &str, + expected_type: u16, + expected_class: u16, +) -> Result>, WireError> { + parse_response_inner(raw, None, expected_name, expected_type, expected_class) } fn parse_response_inner<'a>( raw: &'a [u8], expected_id: Option, + expected_name: &str, + expected_type: u16, + expected_class: u16, ) -> Result>, WireError> { if raw.len() < 12 { return Err(WireError("short DNS response".to_string())); @@ -82,6 +102,12 @@ fn parse_response_inner<'a>( return Err(WireError("mismatched DNS response ID".to_string())); } let flags = read_u16(raw, 2)?; + if flags & FLAG_RESPONSE == 0 { + return Err(WireError("DNS message is not a response".to_string())); + } + if flags & FLAG_OPCODE != 0 { + return Err(WireError("unexpected DNS response opcode".to_string())); + } if flags & FLAG_TRUNCATED != 0 { return Err(WireError(TRUNCATED_RESPONSE.to_string())); } @@ -96,18 +122,25 @@ fn parse_response_inner<'a>( 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())); + } let mut offset = 12; - for _ in 0..question_count { - let (_, next) = read_name(raw, offset)?; - offset = next + 4; - if offset > raw.len() { - return Err(WireError("short DNS question".to_string())); - } + let (question_name, next) = read_name(raw, offset)?; + offset = next; + let question_type = read_u16(raw, offset)?; + let question_class = read_u16(raw, offset + 2)?; + offset += 4; + if !names_equal(&question_name, expected_name) + || question_type != expected_type + || question_class != expected_class + { + return Err(WireError("mismatched DNS response question".to_string())); } let mut records = Vec::new(); for _ in 0..answer_count { - let (_, next) = read_name(raw, offset)?; + let (name, next) = read_name(raw, offset)?; offset = next; let typ = read_u16(raw, offset)?; let class = read_u16(raw, offset + 2)?; @@ -120,6 +153,7 @@ fn parse_response_inner<'a>( let data_offset = offset; offset += rdlen; records.push(ResourceRecord { + name, typ, class, ttl, @@ -127,9 +161,42 @@ fn parse_response_inner<'a>( data: &raw[data_offset..data_offset + rdlen], }); } + + // Answer records are relevant only when their owner is the queried name or + // is reachable from it through an IN-class CNAME chain. Records can appear + // in any order, so expand the reachable set to a fixed point first. + let mut reachable = vec![expected_name.to_string()]; + loop { + let mut changed = false; + for record in &records { + if record.class != expected_class + || record.typ != TYPE_CNAME + || !reachable.iter().any(|name| names_equal(name, &record.name)) + { + continue; + } + let (target, next) = read_name(raw, record.data_offset)?; + if next != record.data_offset + record.data.len() { + return Err(WireError("invalid DNS CNAME resource".to_string())); + } + if !reachable.iter().any(|name| names_equal(name, &target)) { + reachable.push(target); + changed = true; + } + } + if !changed { + break; + } + } + records.retain(|record| reachable.iter().any(|name| names_equal(name, &record.name))); Ok(records) } +pub(crate) fn names_equal(left: &str, right: &str) -> bool { + left.trim_end_matches('.') + .eq_ignore_ascii_case(right.trim_end_matches('.')) +} + pub(crate) fn read_name(packet: &[u8], offset: usize) -> Result<(String, usize), WireError> { let mut labels = Vec::new(); let mut pos = offset; @@ -241,6 +308,21 @@ fn rcode_name(status: i32) -> &'static str { mod tests { use super::*; + #[test] + 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"); + } + + #[test] + fn rejects_mismatched_response_question() { + 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"); + } + #[test] fn build_query_advertises_edns0_udp_payload_size() { let query = build_query(0x1234, "example.com", TYPE_A).unwrap();