Fix DNS response handling, relay caching, and server reliability

- Restore client transaction IDs and casing in stale and truncated replies
- Fix classical encryption padding at 64-byte boundaries
- Accept empty DNS responses during cache TTL processing
- Validate relayed certificates using the received datagram length
- Enforce relay size limits on cached replies and avoid caching truncated certificates
- Handle bare filenames when saving state
- Enforce the metrics connection timeout

Author: jedisct1

src/anonymized_dns.rs

@@ -105,40 +105,13 @@ pub async fn handle_anonymized_dns(
         if is_encrypted_response(&response, response_len) {
             break (response_len, false);
         }
-        if is_certificate_response(&response, encrypted_packet) {
+        if is_certificate_response(&response, response_len, encrypted_packet) {
             break (response_len, true);
         }
     };
     response.truncate(response_len);
     if is_certificate_response {
-        let mut hasher = globals.hasher;
-        hasher.write(&relayed_packet[..ANONYMIZED_DNSCRYPT_OVERHEAD]);
-        hasher.write(&dns::qname(encrypted_packet)?);
-        let packet_hash = hasher.finish128().as_u128();
-        let cached_response = {
-            match globals.cert_cache.lock().get(&packet_hash) {
-                None => None,
-                Some(response) if !(*response).has_expired() => {
-                    trace!("Relayed certificate cached");
-                    let mut cached_response = (*response).clone();
-                    cached_response.set_tid(dns::tid(encrypted_packet));
-                    Some(cached_response.into_response())
-                }
-                Some(_) => {
-                    trace!("Relayed certificate expired");
-                    None
-                }
-            }
-        };
-        match cached_response {
-            None => {
-                globals.cert_cache.lock().insert(
-                    packet_hash,
-                    CachedResponse::new(&globals.cert_cache, response.clone()),
-                );
-            }
-            Some(cached_response) => response = cached_response,
-        }
+        response = cache_certificate_response(&globals, relayed_packet, response)?;
     }
 
     #[cfg(feature = "metrics")]
@@ -147,13 +120,55 @@ pub async fn handle_anonymized_dns(
     respond_to_query(client_ctx, response).await
 }
 
+fn cache_certificate_response(
+    globals: &Globals,
+    relayed_packet: &[u8],
+    mut response: Vec<u8>,
+) -> Result<Vec<u8>, Error> {
+    let encrypted_packet = &relayed_packet[ANONYMIZED_DNSCRYPT_OVERHEAD..];
+    let mut hasher = globals.hasher;
+    hasher.write(&relayed_packet[..ANONYMIZED_DNSCRYPT_OVERHEAD]);
+    hasher.write(&dns::qname(encrypted_packet)?);
+    let packet_hash = hasher.finish128().as_u128();
+    let cached_response = {
+        match globals.cert_cache.lock().get(&packet_hash) {
+            None => None,
+            Some(response) if !(*response).has_expired() => {
+                trace!("Relayed certificate cached");
+                let mut cached_response = (*response).clone();
+                cached_response.set_tid(dns::tid(encrypted_packet));
+                let cached_response = cached_response.into_response();
+                // Cached certificates must obey the same amplification limit
+                // as the response just received from the upstream.
+                (cached_response.len() <= encrypted_packet.len()).then_some(cached_response)
+            }
+            Some(_) => {
+                trace!("Relayed certificate expired");
+                None
+            }
+        }
+    };
+    match cached_response {
+        None if !dns::is_truncated(&response) => {
+            globals.cert_cache.lock().insert(
+                packet_hash,
+                CachedResponse::new(&globals.cert_cache, response.clone()),
+            );
+        }
+        None => {}
+        Some(cached_response) => response = cached_response,
+    }
+    Ok(response)
+}
+
 #[inline]
 fn is_encrypted_response(response: &[u8], response_len: usize) -> bool {
     (DNSCRYPT_UDP_RESPONSE_MIN_SIZE..=DNSCRYPT_UDP_RESPONSE_MAX_SIZE).contains(&response_len)
         && response[..DNSCRYPT_RESPONSE_MAGIC_SIZE] == DNSCRYPT_RESPONSE_MAGIC
 }
 
-fn is_certificate_response(response: &[u8], query: &[u8]) -> bool {
+fn is_certificate_response(response: &[u8], response_len: usize, query: &[u8]) -> bool {
+    let response = &response[..response_len];
     let prefix = b"2.dnscrypt-cert.";
     if !((DNS_HEADER_SIZE + prefix.len() + 4..=DNS_MAX_PACKET_SIZE).contains(&query.len())
         && (DNS_HEADER_SIZE + prefix.len() + 4..=DNS_MAX_PACKET_SIZE).contains(&response.len())
@@ -207,11 +222,117 @@ mod tests {
     fn only_txt_in_answers_are_certificate_responses() {
         let txt_query = cert_packet(dns::DNS_TYPE_TXT, false);
         let txt_response = cert_packet(dns::DNS_TYPE_TXT, true);
-        assert!(is_certificate_response(&txt_response, &txt_query));
+        assert!(is_certificate_response(
+            &txt_response,
+            txt_response.len(),
+            &txt_query
+        ));
 
         let a_query = cert_packet(dns::DNS_TYPE_A, false);
         let a_response = cert_packet(dns::DNS_TYPE_A, true);
-        assert!(!is_certificate_response(&a_response, &a_query));
-        assert!(!is_certificate_response(&a_response, &txt_query));
+        assert!(!is_certificate_response(
+            &a_response,
+            a_response.len(),
+            &a_query
+        ));
+        assert!(!is_certificate_response(
+            &a_response,
+            a_response.len(),
+            &txt_query
+        ));
+    }
+
+    #[test]
+    fn certificate_validation_ignores_bytes_beyond_the_received_datagram() {
+        let query = cert_packet(dns::DNS_TYPE_TXT, false);
+        let response = cert_packet(dns::DNS_TYPE_TXT, true);
+        let mut buffer = vec![0; DNSCRYPT_UDP_RESPONSE_MAX_SIZE];
+        buffer[..response.len()].copy_from_slice(&response);
+        assert!(is_certificate_response(&buffer, response.len(), &query));
+        // A subsequent datagram leaves the earlier question in the receive buffer.
+        for len in [DNS_HEADER_SIZE, response.len() - 1] {
+            assert!(!is_certificate_response(&buffer, len, &query));
+        }
+    }
+
+    fn relayed_cert_query(size: usize) -> Vec<u8> {
+        let mut packet = vec![0; ANONYMIZED_DNSCRYPT_OVERHEAD];
+        let mut query = cert_packet(dns::DNS_TYPE_TXT, false);
+        query.resize(size, 0);
+        packet.extend(query);
+        packet
+    }
+
+    fn cert_response(txt_size: usize) -> Vec<u8> {
+        let mut response = cert_packet(dns::DNS_TYPE_TXT, true);
+        response[7] = 1;
+        response.extend_from_slice(&[0xc0, 0x0c, 0, 16, 0, 1, 0, 0, 2, 88]);
+        response.extend_from_slice(&((txt_size + 1) as u16).to_be_bytes());
+        response.push(txt_size as u8);
+        response.resize(response.len() + txt_size, 0x42);
+        response
+    }
+
+    #[test]
+    fn cached_certificate_respects_the_current_query_size() {
+        let runtime = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        runtime.block_on(async {
+            let globals = crate::tests::globals("127.0.0.1:9".parse().unwrap());
+            let large = cert_response(255);
+            cache_certificate_response(&globals, &relayed_cert_query(512), large).unwrap();
+            let small = cert_response(124);
+            let response =
+                cache_certificate_response(&globals, &relayed_cert_query(256), small.clone())
+                    .unwrap();
+            assert!(
+                response.len() <= 256,
+                "cache bypassed the relay amplification limit"
+            );
+            assert_eq!(response, small);
+        });
+    }
+
+    #[test]
+    fn truncated_certificate_does_not_replace_a_complete_response() {
+        let runtime = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        runtime.block_on(async {
+            let globals = crate::tests::globals("127.0.0.1:9".parse().unwrap());
+            let mut truncated = cert_response(124);
+            dns::truncate(&mut truncated);
+            cache_certificate_response(&globals, &relayed_cert_query(256), truncated).unwrap();
+            let complete = cert_response(255);
+            let response =
+                cache_certificate_response(&globals, &relayed_cert_query(512), complete.clone())
+                    .unwrap();
+            assert!(!dns::is_truncated(&response));
+            assert_eq!(response, complete);
+        });
+    }
+
+    #[test]
+    fn complete_cached_certificate_is_reused_when_it_fits() {
+        let runtime = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        runtime.block_on(async {
+            let globals = crate::tests::globals("127.0.0.1:9".parse().unwrap());
+            let mut query = relayed_cert_query(512);
+            let mut cached = cert_response(124);
+            cache_certificate_response(&globals, &query, cached.clone()).unwrap();
+            dns::set_tid(&mut query[ANONYMIZED_DNSCRYPT_OVERHEAD..], 0xabcd);
+            dns::set_tid(&mut cached, 0xabcd);
+            for mut fresh in [cert_response(255), cert_response(124)] {
+                dns::set_tid(&mut fresh, 0xabcd);
+                let response = cache_certificate_response(&globals, &query, fresh).unwrap();
+                assert_eq!(response, cached);
+            }
+        });
     }
 }

src/config.rs

@@ -205,7 +205,10 @@ impl State {
         fp.sync_data().await?;
         mem::drop(fp);
         tokio::fs::rename(path_tmp, path).await?;
-        let dir = tokio::fs::File::open(path.parent().unwrap_or_else(|| Path::new("."))).await?;
+        let parent = path
+            .parent()
+            .filter(|parent| !parent.as_os_str().is_empty());
+        let dir = tokio::fs::File::open(parent.unwrap_or_else(|| Path::new("."))).await?;
         dir.sync_all().await?;
         Ok(())
     }
@@ -219,3 +222,30 @@ impl State {
         Ok(state)
     }
 }
+
+#[cfg(all(test, unix))]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn save_state_to_a_bare_filename() {
+        let runtime = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .unwrap();
+        let path = PathBuf::from(format!(".test-state-{:016x}.state", rand::random::<u64>()));
+        let state = State {
+            provider_kp: SignKeyPair::default(),
+            dnscrypt_encryption_params_set: vec![],
+        };
+        let result = runtime.block_on(state.async_save(&path));
+        let loaded = State::from_file(&path, 10);
+        let _ = fs::remove_file(&path);
+        let _ = fs::remove_file(path.with_extension("tmp"));
+        result.unwrap();
+        assert_eq!(
+            loaded.unwrap().provider_kp.pk.as_bytes(),
+            state.provider_kp.pk.as_bytes()
+        );
+    }
+}

src/crypto.rs

@@ -259,7 +259,7 @@ impl SharedKey {
         hasher.write(client_nonce);
         let pad_size: usize = 1 + (hasher.finish() as usize & 0xff);
         let mut padded_plaintext_len = (plaintext_len + pad_size) & !63;
-        if padded_plaintext_len < plaintext_len {
+        if padded_plaintext_len <= plaintext_len {
             padded_plaintext_len += 256;
         }
         if padded_plaintext_len > max_padded_plaintext_len {

src/dns.rs

@@ -361,7 +361,7 @@ pub fn min_ttl(packet: &[u8], min_ttl: u32, max_ttl: u32, failure_ttl: u32) -> R
     ensure!(qdcount(packet) == 1, "No question");
     let mut offset = skip_name(packet, DNS_OFFSET_QUESTION)?;
     assert!(offset > DNS_OFFSET_QUESTION);
-    ensure!(packet_len - offset > 4, "Short packet");
+    ensure!(packet_len - offset >= 4, "Short packet");
     offset += 4;
     let (ancount, nscount, arcount) = (ancount(packet), nscount(packet), arcount(packet));
     let rrcount = ancount as usize + nscount as usize + arcount as usize;
@@ -389,7 +389,7 @@ pub fn set_ttl(packet: &mut [u8], ttl: u32) -> Result<(), Error> {
     ensure!(qdcount(packet) == 1, "No question");
     let mut offset = skip_name(packet, DNS_OFFSET_QUESTION)?;
     assert!(offset > DNS_OFFSET_QUESTION);
-    ensure!(packet_len - offset > 4, "Short packet");
+    ensure!(packet_len - offset >= 4, "Short packet");
     offset += 4;
     let (ancount, nscount, arcount) = (ancount(packet), nscount(packet), arcount(packet));
     let rrcount = ancount as usize + nscount as usize + arcount as usize;
@@ -729,6 +729,26 @@ pub fn serve_ip_response(client_packet: Vec<u8>, ip: IpAddr, ttl: u32) -> Result
 mod tests {
     use super::*;
 
+    #[test]
+    fn ttl_processing_accepts_responses_without_resource_records() {
+        let query = vec![
+            0x12, 0x34, 0x01, 0x00, 0x00, 0x01, 0, 0, 0, 0, 0, 0, 7, b'e', b'x', b'a', b'm', b'p',
+            b'l', b'e', 3, b'o', b'r', b'g', 0, 0, 1, 0, 1,
+        ];
+        for rcode in [0, DNS_RCODE_NXDOMAIN] {
+            let mut response = query.clone();
+            authoritative_response(&mut response);
+            set_rcode(&mut response, rcode);
+            assert_eq!(min_ttl(&response, 0, 3600, 60).unwrap(), 60);
+            let original = response.clone();
+            set_ttl(&mut response, 30).unwrap();
+            assert_eq!(response, original);
+            response.pop();
+            assert!(min_ttl(&response, 0, 3600, 60).is_err());
+            assert!(set_ttl(&mut response, 30).is_err());
+        }
+    }
+
     fn appended_len(cert_bin: &[u8]) -> usize {
         let mut packet = vec![0u8; DNS_HEADER_SIZE];
         append_txt_cert(&mut packet, cert_bin).unwrap();

src/dnscrypt.rs

@@ -328,6 +328,36 @@ mod tests {
     use super::*;
     use crate::pq;
 
+    #[test]
+    fn classical_responses_fit_and_round_trip_at_padding_boundaries() {
+        crate::crypto::init().unwrap();
+        let shared_key = SharedKey::from_bytes([0x42; 32]);
+        for nonce_byte in 0..=255 {
+            let nonce = [nonce_byte; DNSCRYPT_FULL_NONCE_SIZE];
+            let params = EncryptionParams::Classical {
+                shared_key: shared_key.clone(),
+                nonce,
+            };
+            for len in [63, 64, 65, 127, 128, 129] {
+                let response = vec![0xda; len];
+                for budget in [len + DNSCRYPT_RESPONSE_MIN_OVERHEAD, 512] {
+                    let encrypted = encrypt(response.clone(), &params, budget)
+                        .expect("response within budget failed to encrypt");
+                    assert!(encrypted.len() <= budget);
+                    assert_eq!(
+                        shared_key
+                            .decrypt(&nonce, &encrypted[DNSCRYPT_RESPONSE_HEADER_SIZE..])
+                            .unwrap(),
+                        response
+                    );
+                }
+                assert!(
+                    encrypt(response, &params, len + DNSCRYPT_RESPONSE_MIN_OVERHEAD - 1).is_err()
+                );
+            }
+        }
+    }
+
     // Any response the `maybe_truncate_response` gate lets through must
     // encrypt into the query-size budget, shrinking the padding and
     // withholding the ticket as needed; anything larger must fail, keeping