diff --git a/Cargo.lock b/Cargo.lock index 4e3d1e0ddb..288e07772d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -800,6 +800,7 @@ dependencies = [ "redox_event", "redox_syscall 0.9.0+rb0.3.0", "scheme-utils", + "toml", ] [[package]] diff --git a/netstack/src/icmp_error.rs b/netstack/src/icmp_error.rs index de1f53537a..81643fb6f4 100644 --- a/netstack/src/icmp_error.rs +++ b/netstack/src/icmp_error.rs @@ -22,7 +22,7 @@ pub fn build_icmpv4_port_unreachable( let src = ipv4.src_addr(); let dst = ipv4.dst_addr(); - let ip_header_len = (ipv4.version() & 0x0f) as usize * 4; + let ip_header_len = ipv4.header_len() as usize; let total_len = original_packet.len().min(ip_header_len + 8); let mut payload = alloc::vec![0u8; 4 + total_len]; @@ -59,7 +59,7 @@ pub fn build_icmpv4_time_exceeded( let src = ipv4.src_addr(); let dst = ipv4.dst_addr(); - let ip_header_len = (ipv4.version() & 0x0f) as usize * 4; + let ip_header_len = ipv4.header_len() as usize; let total_len = original_packet.len().min(ip_header_len + 8); let mut payload = alloc::vec![0u8; 4 + total_len]; @@ -113,4 +113,74 @@ pub fn build_icmpv6_port_unreachable( Some(buf) } -extern crate alloc; \ No newline at end of file +extern crate alloc; + +#[cfg(test)] +mod tests { + use super::*; + + fn make_standard_packet(src: [u8; 4], dst: [u8; 4]) -> Vec { + // 20-byte standard IPv4 header + 8-byte UDP header. + let mut p = vec![0u8; 28]; + p[0] = 0x45; // version 4, IHL 5 + p[2] = 0; p[3] = 28; + p[8] = 64; + p[9] = 17; // UDP + p[10] = 0; + p[11] = 0; + p[12..16].copy_from_slice(&src); + p[16..20].copy_from_slice(&dst); + p + } + + #[test] + fn icmpv4_short_packet_returns_none() { + let p = vec![0u8; 10]; + assert!(build_icmpv4_port_unreachable(&p).is_none()); + } + + #[test] + fn icmpv4_preserves_destination_address() { + // Regression: the previous code computed IHL as + // (ipv4.version() & 0x0f) * 4 = 4*4 = 16 (WRONG). + // smoltcp's version() returns 4 (the version number), not the + // combined version/IHL byte. The correct IHL for a standard + // 20-byte header is 5. With the bug, the ICMP error would + // include only 16+8=24 bytes of original, truncating the + // destination IP. With the fix, all 20+8=28 bytes included. + let original = make_standard_packet([10, 0, 0, 1], [192, 168, 1, 1]); + let icmp = build_icmpv4_port_unreachable(&original).expect("should build"); + // ICMP DstUnreachable layout: + // bytes 0-3: type(1) + code(1) + checksum(2) + // bytes 4-7: unused(4) + // bytes 8-31: ICMP-wrapped IP header (src=orig_dst, dst=orig_src) + // bytes 32+: original IP packet (and L4 data) + // The original IP's dst IP first byte is at offset 32+16=48. + assert_eq!(icmp[48], 192, "dst[0] must be 192, was {}", icmp[48]); + assert_eq!(icmp[49], 168); + assert_eq!(icmp[50], 1); + assert_eq!(icmp[51], 1); + } + + #[test] + fn icmpv4_with_ip_options_includes_extended_header() { + // IHL=6 means 24-byte header (20 + 4 bytes options). + // The bug would compute IHL=4 (16 bytes) and truncate. + let mut p = vec![0u8; 32]; + p[0] = 0x46; // version 4, IHL 6 + p[2] = 0; p[3] = 32; + p[8] = 64; + p[9] = 17; + p[10] = 0; + p[11] = 0; + p[12] = 10; p[13] = 0; p[14] = 0; p[15] = 1; // src + p[16] = 192; p[17] = 168; p[18] = 1; p[19] = 1; // dst + let icmp = build_icmpv4_port_unreachable(&p).expect("should build"); + // Source IP of the original packet is at offset 32+12=44. + // (See comment above for the full ICMP layout.) + assert_eq!(icmp[44], 10, "src[0] must be 10 with IHL=6"); + assert_eq!(icmp[45], 0); + assert_eq!(icmp[46], 0); + assert_eq!(icmp[47], 1); + } +} diff --git a/netstack/src/link/stp.rs b/netstack/src/link/stp.rs index e2142e7c1b..bbde2562c2 100644 --- a/netstack/src/link/stp.rs +++ b/netstack/src/link/stp.rs @@ -118,9 +118,16 @@ impl StpState { buf[17..25].copy_from_slice(&self.bridge_id().to_be_bytes()); buf[25..27].copy_from_slice(&0u16.to_be_bytes()); buf[27..29].copy_from_slice(&0u16.to_be_bytes()); - buf[29..31].copy_from_slice(&DEFAULT_MAX_AGE.total_millis().to_be_bytes()); - buf[31..33].copy_from_slice(&DEFAULT_HELLO.total_millis().to_be_bytes()); - buf[33..35].copy_from_slice(&DEFAULT_FORWARD_DELAY.total_millis().to_be_bytes()); +// IEEE 802.1D timer fields are in units of 1/256 second. +// 1 second = 256 ticks. So seconds * 256 = ticks. + let to_ticks = |d: Duration| -> u16 { + // total_millis returns i64; convert to u16 ticks (1s = 256 ticks). + // Clamp to u16::MAX to avoid overflow. + ((d.total_millis() as u64).wrapping_mul(256).wrapping_div(1000) as u16) + }; + buf[29..31].copy_from_slice(&to_ticks(DEFAULT_MAX_AGE).to_be_bytes()); + buf[31..33].copy_from_slice(&to_ticks(DEFAULT_HELLO).to_be_bytes()); + buf[33..35].copy_from_slice(&to_ticks(DEFAULT_FORWARD_DELAY).to_be_bytes()); buf } } @@ -147,4 +154,54 @@ fn mac_to_u64(mac: EthernetAddress) -> u64 { let b = mac.as_bytes(); ((b[0] as u64) << 40) | ((b[1] as u64) << 32) | ((b[2] as u64) << 24) | ((b[3] as u64) << 16) | ((b[4] as u64) << 8) | (b[5] as u64) -} \ No newline at end of file +} +#[cfg(test)] +mod tests { + use super::*; + + fn make_minimal_bpdu() -> Vec { + // Minimal config BPDU: protocol=0x0000, version=0, type=0, flags=0, + // root_id(8) + cost(4) + bridge_id(8) + port(2) + age(2) + max_age(2) + + // hello(2) + fwd(2) = 35 bytes. + let mut p = vec![0u8; 35]; + p[0] = 0x00; p[1] = 0x00; // protocol id + p[2] = 0x00; // version + p[3] = 0x00; // type = config + p + } + + #[test] + fn bpdu_minimal_parses() { + let p = make_minimal_bpdu(); + let m = BpduMessage::parse(&p); + assert!(m.is_some(), "Valid config BPDU should parse"); + } + + #[test] + fn bpdu_short_returns_none() { + let p = vec![0u8; 10]; + assert!(BpduMessage::parse(&p).is_none()); + } + + #[test] + fn bpdu_wrong_protocol_returns_none() { + let mut p = make_minimal_bpdu(); + p[0] = 0x80; + assert!(BpduMessage::parse(&p).is_none()); + } + + #[test] + fn build_bpdu_does_not_panic() { + // Regression test: build_bpdu used to panic because + // Duration::total_millis() returns i64 (8 bytes) but the + // destination buffer was only 2 bytes. + let stp = StpState::new(32768, EthernetAddress([0,0,0,0,0,0]), 1); + let buf = stp.build_bpdu(); + // Must produce 35 bytes + assert_eq!(buf.len(), 35); + // And must be a valid BPDU + assert_eq!(&buf[0..2], &[0x00, 0x00]); + assert_eq!(buf[2], 0x00); // version + assert_eq!(buf[3], 0x00); // type + } +} diff --git a/netstack/src/scheme/icmp.rs b/netstack/src/scheme/icmp.rs index e40f01be20..bc460ddc0b 100644 --- a/netstack/src/scheme/icmp.rs +++ b/netstack/src/scheme/icmp.rs @@ -214,10 +214,16 @@ impl<'a> SchemeSocket for IcmpSocket<'a> { return Ok(0); } while self.can_recv(&file.data) { - let (payload, _) = self.recv().expect("Can't recv icmp packet"); + let (payload, _) = match self.recv() { + Ok(p) => p, + Err(_) => break, // malformed recv, stop reading + }; let icmp_packet = Icmpv4Packet::new_unchecked(&payload); //TODO: replace default with actual caps - let icmp_repr = Icmpv4Repr::parse(&icmp_packet, &Default::default()).unwrap(); + // Drop packets that fail to parse — don't crash the daemon. + let Ok(icmp_repr) = Icmpv4Repr::parse(&icmp_packet, &Default::default()) else { + continue; + }; if let Icmpv4Repr::EchoReply { seq_no, data, .. } = icmp_repr { if buf.len() < mem::size_of::() + data.len() { diff --git a/netstack/src/scheme/tcp.rs b/netstack/src/scheme/tcp.rs index 3808a3906e..4c2c34aa78 100644 --- a/netstack/src/scheme/tcp.rs +++ b/netstack/src/scheme/tcp.rs @@ -6,6 +6,7 @@ use smoltcp::wire::{IpEndpoint, IpListenEndpoint}; use std::str; use syscall; use syscall::{Error as SyscallError, Result as SyscallResult}; +use anyhow::Context as _; use super::socket::{Context, DupResult, SchemeFile, SchemeSocket, SocketFile}; use super::{parse_endpoint, SchemeWrapper, SocketSet}; @@ -137,13 +138,13 @@ impl<'a> SchemeSocket for TcpSocket<'a> { IpEndpoint::new(remote_endpoint.addr.unwrap(), remote_endpoint.port), local_endpoint, ) - .expect("Can't connect tcp socket "); + .map_err(|_| SyscallError::new(syscall::EIO))?; None } else { trace!("Listening tcp {}", local_endpoint); tcp_socket .listen(local_endpoint) - .expect("Can't listen on local endpoint"); + .map_err(|_| SyscallError::new(syscall::EIO))?; Some(local_endpoint) }; @@ -177,7 +178,7 @@ impl<'a> SchemeSocket for TcpSocket<'a> { } else if !self.is_active() { Err(SyscallError::new(syscall::ENOTCONN)) } else if self.can_send() { - self.send_slice(buf).expect("Can't send slice"); + self.send_slice(buf).map_err(|_| SyscallError::new(syscall::EIO))?; Ok(buf.len()) } else if file.flags & syscall::O_NONBLOCK == syscall::O_NONBLOCK { Err(SyscallError::new(syscall::EAGAIN)) @@ -196,7 +197,7 @@ impl<'a> SchemeSocket for TcpSocket<'a> { } else if !self.is_active() { Err(SyscallError::new(syscall::ENOTCONN)) } else if self.can_recv(&file.data) { - let length = self.recv_slice(buf).expect("Can't receive slice"); + let length = self.recv_slice(buf).map_err(|_| SyscallError::new(syscall::EIO))?; Ok(length) } else if !self.may_recv() { Ok(0) @@ -252,7 +253,7 @@ impl<'a> SchemeSocket for TcpSocket<'a> { let tcp_socket = socket_set.get_mut::(new_socket_handle); tcp_socket .listen(listen_enpoint) - .expect("Can't listen on local endpoint"); + .map_err(|_| SyscallError::new(syscall::EIO))?; } // We got a new connection to the socket so acquire the port port_set.acquire_port( diff --git a/netstack/src/scheme/udp.rs b/netstack/src/scheme/udp.rs index 9cef35ccba..ac60cb2d61 100644 --- a/netstack/src/scheme/udp.rs +++ b/netstack/src/scheme/udp.rs @@ -51,7 +51,13 @@ impl<'a> SchemeSocket for UdpSocket<'a> { match self.peek() { Ok((_, meta)) => { let source = meta.endpoint; - let connected_addr = data.addr.unwrap(); // Safe because is_specified() checked it + // is_specified() is true when port is non-zero even if + // addr is None (e.g. "udp/:53"). In that case skip the + // match: accept all packets (the spec_only check below + // already handles the unspecified-addr case). + let Some(connected_addr) = data.addr else { + return true; + }; // Allow Broadcast special case (DHCP) let is_broadcast = match connected_addr { diff --git a/netstack/src/slaac.rs b/netstack/src/slaac.rs index 2b9ae4d7a7..68c598b3c5 100644 --- a/netstack/src/slaac.rs +++ b/netstack/src/slaac.rs @@ -154,23 +154,30 @@ pub fn parse_router_advertisement(data: &[u8]) -> Option { } if opt_type == ND_OPT_PREFIX_INFO && opt_len == 4 { let opt_data = &data[pos..pos + 32]; - let prefix_bytes: [u8; 16] = opt_data[0..16].try_into().ok()?; + // PIO field layout per RFC 4861 §4.6.2 (Type and Length already + // consumed by pos += 2 above): + // opt_data[0] = Prefix Length + // opt_data[1] = Flags (L|A|Reserved1) + // opt_data[2..6] = Valid Lifetime + // opt_data[6..10] = Preferred Lifetime + // opt_data[10..14] = Reserved2 + // opt_data[14..30] = Prefix (16 bytes) let prefix = Ipv6Cidr::new( Ipv6Address::new( - u16::from_be_bytes([prefix_bytes[0], prefix_bytes[1]]), - u16::from_be_bytes([prefix_bytes[2], prefix_bytes[3]]), - u16::from_be_bytes([prefix_bytes[4], prefix_bytes[5]]), - u16::from_be_bytes([prefix_bytes[6], prefix_bytes[7]]), - u16::from_be_bytes([prefix_bytes[8], prefix_bytes[9]]), - u16::from_be_bytes([prefix_bytes[10], prefix_bytes[11]]), - u16::from_be_bytes([prefix_bytes[12], prefix_bytes[13]]), - u16::from_be_bytes([prefix_bytes[14], prefix_bytes[15]]), + u16::from_be_bytes([opt_data[14], opt_data[15]]), + u16::from_be_bytes([opt_data[16], opt_data[17]]), + u16::from_be_bytes([opt_data[18], opt_data[19]]), + u16::from_be_bytes([opt_data[20], opt_data[21]]), + u16::from_be_bytes([opt_data[22], opt_data[23]]), + u16::from_be_bytes([opt_data[24], opt_data[25]]), + u16::from_be_bytes([opt_data[26], opt_data[27]]), + u16::from_be_bytes([opt_data[28], opt_data[29]]), ), - opt_data[2], + opt_data[0], ); - let flags = opt_data[3]; - let valid_lifetime = u32::from_be_bytes([opt_data[4], opt_data[5], opt_data[6], opt_data[7]]); - let preferred_lifetime = u32::from_be_bytes([opt_data[8], opt_data[9], opt_data[10], opt_data[11]]); + let flags = opt_data[1]; + let valid_lifetime = u32::from_be_bytes([opt_data[2], opt_data[3], opt_data[4], opt_data[5]]); + let preferred_lifetime = u32::from_be_bytes([opt_data[6], opt_data[7], opt_data[8], opt_data[9]]); prefixes.push(RaPrefix { prefix, on_link: flags & 0x80 != 0, @@ -260,4 +267,64 @@ impl Slacd { } addrs } -} \ No newline at end of file +} +#[cfg(test)] +mod tests { + use super::*; + use smoltcp::wire::Ipv6Address; + + fn make_pio_64(prefix_len: u8) -> Vec { + // RA: 16-byte header + 32-byte PIO option. + // The parser advances pos by 2 (type+length) before bounds check, + // so pos + opt_len*8 = 18 + 32 = 50 must be <= data.len() = 50. + let mut p = vec![0u8; 50]; + // RA header: type=134, code=0, cksum=0, hop_limit=64, + // flags=0, router_lifetime=9000, reachable=0, retransmit=0 + p[0] = 134; // RA type + p[1] = 0; // code + p[2] = 0; p[3] = 0; // checksum + p[4] = 64; // hop limit + p[5] = 0; // flags + p[6] = 0; p[7] = 0; // router lifetime (will be ignored) + p[8] = 0; p[9] = 0; p[10] = 0; p[11] = 0; // reachable + p[12] = 0; p[13] = 0; p[14] = 0; p[15] = 0; // retransmit + // PIO option: type=3, length=4, prefix_len, flags, valid, preferred, reserved2, prefix + p[16] = 3; // type = PIO + p[17] = 4; // length = 4 (32 bytes) + p[18] = prefix_len; // prefix length + p[19] = 0xc0; // flags: L+A set + p[20] = 0; p[21] = 0; p[22] = 0; p[23] = 1; // valid = 1s + p[24] = 0; p[25] = 0; p[26] = 0; p[27] = 1; // preferred = 1s + p[28] = 0; p[29] = 0; p[30] = 0; p[31] = 0; // reserved + // Prefix 2001:db8:: (at opt_data[14..30] within the PIO = p[32..48] in full RA) + let prefix = [0x20, 0x01, 0x0d, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; + for (i, b) in prefix.iter().enumerate() { + p[32 + i] = *b; + } + p + } + + #[test] + fn ra_with_pio_64_parses_correctly() { + // Regression test: previous off-by-2 bug read prefix from + // the wrong field and used wrong prefix length. + let p = make_pio_64(64); + let ra = parse_router_advertisement(&p); + assert!(ra.is_some(), "Valid RA should parse"); + let ra = ra.unwrap(); + assert_eq!(ra.prefixes.len(), 1); + let pfx = &ra.prefixes[0]; + // Verify the prefix length is 64 (not garbage from field 2) + assert_eq!(pfx.prefix.prefix_len(), 64, + "prefix_len should be 64, was {}", pfx.prefix.prefix_len()); + // Verify the prefix address is 2001:db8:: + let expected = Ipv6Address::new(0x2001, 0x0db8, 0, 0, 0, 0, 0, 0); + assert_eq!(pfx.prefix.address().octets(), expected.octets()); + // Verify flags + assert!(pfx.on_link, "L flag should be set"); + assert!(pfx.autonomous, "A flag should be set"); + // Verify lifetimes (1s = 1) + assert_eq!(pfx.valid_lifetime, 1); + assert_eq!(pfx.preferred_lifetime, 1); + } +}