From 83bcc2d8c156593fc2df2596e8e0425da60700ac Mon Sep 17 00:00:00 2001 From: Tomasz Klak Date: Tue, 26 Nov 2024 14:41:28 +0100 Subject: [PATCH 1/4] Fix test_parse_device2 by moving Processing Instruction to first line Processing instructions need to be at the beginning of the document. --- src/common/parsing.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/common/parsing.rs b/src/common/parsing.rs index ee99ff65..6a0b61d9 100644 --- a/src/common/parsing.rs +++ b/src/common/parsing.rs @@ -507,8 +507,7 @@ fn test_parse_device1() { #[test] fn test_parse_device2() { - let text = r#" - + let text = r#" 1 From 5ab6a67763c5ebfb059d7219f4166e3d010c748a Mon Sep 17 00:00:00 2001 From: Tomasz Klak Date: Tue, 26 Nov 2024 14:41:45 +0100 Subject: [PATCH 2/4] Update to rust edition 2021 --- .github/workflows/ci.yml | 8 +------ Cargo.toml | 2 +- rust-toolchain | 1 + src/aio/gateway.rs | 12 ++++------ src/aio/search.rs | 2 +- src/common/parsing.rs | 49 ++++++++++++++++------------------------ src/errors.rs | 6 ++--- src/gateway.rs | 6 ++--- src/search.rs | 4 ++-- 9 files changed, 35 insertions(+), 55 deletions(-) create mode 100644 rust-toolchain diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b71d06c..7ded3586 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,11 +23,5 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - run: rustup component add clippy - - uses: actions-rs/clippy-check@v1 - with: - token: ${{ secrets.GITHUB_TOKEN }} - args: --all-features --all-targets + - run: cargo clippy --all-features --all-targets diff --git a/Cargo.toml b/Cargo.toml index 0845bb4b..30a715eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ authors = ["Simon Bernier St-Pierre "] description = "Internet Gateway Protocol client" documentation = "https://docs.rs/igd/" -edition = "2018" +edition = "2021" homepage = "https://github.com/sbstp/rust-igd" keywords = ["igd", "upnp"] license = "MIT" diff --git a/rust-toolchain b/rust-toolchain new file mode 100644 index 00000000..369f9966 --- /dev/null +++ b/rust-toolchain @@ -0,0 +1 @@ +1.77.2 diff --git a/src/aio/gateway.rs b/src/aio/gateway.rs index c91e1ea9..c3fb29ac 100644 --- a/src/aio/gateway.rs +++ b/src/aio/gateway.rs @@ -118,7 +118,7 @@ impl Gateway { // Fall back to using AddPortMapping with a random port. let gateway = self.clone(); gateway - .retry_add_random_port_mapping(protocol, local_addr, lease_duration, &description) + .retry_add_random_port_mapping(protocol, local_addr, lease_duration, description) .await } } @@ -132,7 +132,7 @@ impl Gateway { ) -> Result { for _ in 0u8..20u8 { match self - .add_random_port_mapping(protocol, local_addr, lease_duration, &description) + .add_random_port_mapping(protocol, local_addr, lease_duration, description) .await { Ok(port) => return Ok(port), @@ -247,11 +247,9 @@ impl Gateway { .perform_request( messages::DELETE_PORT_MAPPING_HEADER, &messages::format_delete_port_message( - self.control_schema - .get("DeletePortMapping") - .ok_or_else(|| RemovePortError::RequestError(RequestError::UnsupportedAction( - "DeletePortMapping".to_string(), - )))?, + self.control_schema.get("DeletePortMapping").ok_or_else(|| { + RemovePortError::RequestError(RequestError::UnsupportedAction("DeletePortMapping".to_string())) + })?, protocol, external_port, ), diff --git a/src/aio/search.rs b/src/aio/search.rs index 1c99e6e0..286e68cd 100644 --- a/src/aio/search.rs +++ b/src/aio/search.rs @@ -75,7 +75,7 @@ fn handle_broadcast_resp(from: &SocketAddr, data: &[u8]) -> Result<(SocketAddr, debug!("handling broadcast response from: {}", from); // Convert response to text - let text = std::str::from_utf8(&data).map_err(SearchError::from)?; + let text = std::str::from_utf8(data).map_err(SearchError::from)?; // Parse socket address and path let (addr, root_url) = parsing::parse_search_result(text)?; diff --git a/src/common/parsing.rs b/src/common/parsing.rs index 6a0b61d9..8df5ec5a 100644 --- a/src/common/parsing.rs +++ b/src/common/parsing.rs @@ -53,40 +53,29 @@ where } fn parse_device(device: &Element) -> Option<(String, String)> { - let services = device - .get_child("serviceList") - .map(|service_list| { - service_list - .children - .iter() - .filter_map(|child| { - let child = child.as_element()?; - if child.name == "service" { - parse_service(child) - } else { - None - } - }) - .next() - }) - .flatten(); - let devices = device.get_child("deviceList").map(parse_device_list).flatten(); - services.or(devices) -} - -fn parse_device_list(device_list: &Element) -> Option<(String, String)> { - device_list - .children - .iter() - .filter_map(|child| { + let services = device.get_child("serviceList").and_then(|service_list| { + service_list.children.iter().find_map(|child| { let child = child.as_element()?; - if child.name == "device" { - parse_device(child) + if child.name == "service" { + parse_service(child) } else { None } }) - .next() + }); + let devices = device.get_child("deviceList").and_then(parse_device_list); + services.or(devices) +} + +fn parse_device_list(device_list: &Element) -> Option<(String, String)> { + device_list.children.iter().find_map(|child| { + let child = child.as_element()?; + if child.name == "device" { + parse_device(child) + } else { + None + } + }) } fn parse_service(service: &Element) -> Option<(String, String)> { @@ -606,7 +595,7 @@ fn test_parse_device2() { "#; let result = parse_control_urls(text.as_bytes()); - assert!(result.is_ok()); + assert!(result.is_ok(), "result: {result:?}"); let (control_schema_url, control_url) = result.unwrap(); assert_eq!(control_url, "/igdupnp/control/WANIPConn1"); assert_eq!(control_schema_url, "/igdconnSCPD.xml"); diff --git a/src/errors.rs b/src/errors.rs index 1f162eb9..2d0e4810 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -405,10 +405,8 @@ pub enum GetGenericPortMappingEntryError { impl From for GetGenericPortMappingEntryError { fn from(err: RequestError) -> GetGenericPortMappingEntryError { match err { - RequestError::ErrorCode(code, _) if code == 606 => GetGenericPortMappingEntryError::ActionNotAuthorized, - RequestError::ErrorCode(code, _) if code == 713 => { - GetGenericPortMappingEntryError::SpecifiedArrayIndexInvalid - } + RequestError::ErrorCode(606, _) => GetGenericPortMappingEntryError::ActionNotAuthorized, + RequestError::ErrorCode(713, _) => GetGenericPortMappingEntryError::SpecifiedArrayIndexInvalid, other => GetGenericPortMappingEntryError::RequestError(other), } } diff --git a/src/gateway.rs b/src/gateway.rs index de015268..8931c7c5 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -25,7 +25,7 @@ impl Gateway { fn perform_request(&self, header: &str, body: &str, ok: &str) -> RequestResult { let url = format!("http://{}{}", self.addr, self.control_url); - let response = attohttpc::post(&url) + let response = attohttpc::post(url) .header("SOAPAction", header) .header("Content-Type", "text/xml") .text(body) @@ -120,7 +120,7 @@ impl Gateway { const ATTEMPTS: usize = 20; for _ in 0..ATTEMPTS { - if let Ok(port) = self.add_random_port_mapping(protocol, local_addr, lease_duration, &description) { + if let Ok(port) = self.add_random_port_mapping(protocol, local_addr, lease_duration, description) { return Ok(port); } } @@ -137,7 +137,7 @@ impl Gateway { ) -> Result { let external_port = common::random_port(); - if let Err(err) = self.add_port_mapping(protocol, external_port, local_addr, lease_duration, &description) { + if let Err(err) = self.add_port_mapping(protocol, external_port, local_addr, lease_duration, description) { match parsing::convert_add_random_port_mapping_error(err) { Some(err) => return Err(err), None => return self.add_same_port_mapping(protocol, local_addr, lease_duration, description), diff --git a/src/search.rs b/src/search.rs index fd281b47..69fe8bf0 100644 --- a/src/search.rs +++ b/src/search.rs @@ -72,7 +72,7 @@ pub fn search_gateway(options: SearchOptions) -> Result { fn get_control_urls(addr: &SocketAddrV4, root_url: &str) -> Result<(String, String), SearchError> { let url = format!("http://{}:{}{}", addr.ip(), addr.port(), root_url); - match RequestBuilder::try_new(Method::GET, &url) { + match RequestBuilder::try_new(Method::GET, url) { Ok(request_builder) => { let response = request_builder.send()?; parsing::parse_control_urls(&response.bytes()?[..]) @@ -84,7 +84,7 @@ fn get_control_urls(addr: &SocketAddrV4, root_url: &str) -> Result<(String, Stri fn get_schemas(addr: &SocketAddrV4, control_schema_url: &str) -> Result>, SearchError> { let url = format!("http://{}:{}{}", addr.ip(), addr.port(), control_schema_url); - match RequestBuilder::try_new(Method::GET, &url) { + match RequestBuilder::try_new(Method::GET, url) { Ok(request_builder) => { let response = request_builder.send()?; parsing::parse_schemas(&response.bytes()?[..]) From ccb5247fadee43fea7bff1e8a33f0950e9bd5378 Mon Sep 17 00:00:00 2001 From: Tomasz Klak Date: Tue, 26 Nov 2024 16:43:51 +0100 Subject: [PATCH 3/4] Add http timeout option for gateway searching --- src/aio/search.rs | 25 ++++++++++++++++--------- src/common/options.rs | 3 +++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/aio/search.rs b/src/aio/search.rs index 286e68cd..dd1e40cc 100644 --- a/src/aio/search.rs +++ b/src/aio/search.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::net::SocketAddr; +use std::time::Duration; use futures::prelude::*; use hyper::Client; @@ -19,18 +20,14 @@ pub async fn search_gateway(options: SearchOptions) -> Result timeout(t, search_response).await?, - None => search_response.await, - }?; + let (response_body, from) = run_with_timeout(options.timeout, receive_search_response(&mut socket)).await??; let (addr, root_url) = handle_broadcast_resp(&from, &response_body)?; - let (control_schema_url, control_url) = get_control_urls(&addr, &root_url).await?; - let control_schema = get_control_schemas(&addr, &control_schema_url).await?; + let (control_schema_url, control_url) = + run_with_timeout(options.http_timeout, get_control_urls(&addr, &root_url)).await??; + let control_schema = + run_with_timeout(options.http_timeout, get_control_schemas(&addr, &control_schema_url)).await??; let addr = match addr { SocketAddr::V4(a) => Ok(a), @@ -49,6 +46,16 @@ pub async fn search_gateway(options: SearchOptions) -> Result(timeout_value: Option, fut: F) -> Result +where + F: Future + Send, +{ + match timeout_value { + Some(t) => Ok(timeout(t, fut).await?), + None => Ok(fut.await), + } +} + // Create a new search async fn send_search_request(socket: &mut UdpSocket, addr: SocketAddr) -> Result<(), SearchError> { debug!( diff --git a/src/common/options.rs b/src/common/options.rs index 31f11cd0..de18a21b 100644 --- a/src/common/options.rs +++ b/src/common/options.rs @@ -23,6 +23,8 @@ pub struct SearchOptions { pub broadcast_address: SocketAddr, /// Timeout for a search iteration (defaults to 10s) pub timeout: Option, + /// Http requests timeout (defaults to None) + pub http_timeout: Option, } impl Default for SearchOptions { @@ -31,6 +33,7 @@ impl Default for SearchOptions { bind_addr: SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(0, 0, 0, 0), 0)), broadcast_address: "239.255.255.250:1900".parse().unwrap(), timeout: Some(Duration::from_secs(10)), + http_timeout: None, } } } From 42caae84cb0070403b5b34b1a5e012927f73819b Mon Sep 17 00:00:00 2001 From: Tomasz Klak Date: Tue, 26 Nov 2024 16:45:29 +0100 Subject: [PATCH 4/4] Reject gateway search responses that spoof IP in message body If the response contains an url pointing to the different IP than the one from which the response was received, reject such response. --- Cargo.toml | 1 + src/aio/search.rs | 75 +++++++++++++++++++++++++++++++++++++++++++++++ src/errors.rs | 10 +++++++ 3 files changed, 86 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 30a715eb..332d4521 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ version = "0.14" [dev-dependencies] simplelog = "0.9" +test-log = "0.2" tokio = {version = "1", features = ["full"]} [features] diff --git a/src/aio/search.rs b/src/aio/search.rs index dd1e40cc..c6450169 100644 --- a/src/aio/search.rs +++ b/src/aio/search.rs @@ -24,6 +24,37 @@ pub async fn search_gateway(options: SearchOptions) -> Result { + if src_ip.ip() != url_ip.ip() { + return Err(SearchError::SpoofedIp { + src_ip: (*src_ip.ip()).into(), + url_ip: (*url_ip.ip()).into(), + }); + } + } + (SocketAddr::V6(src_ip), SocketAddr::V6(url_ip)) => { + if src_ip.ip() != url_ip.ip() { + return Err(SearchError::SpoofedIp { + src_ip: (*src_ip.ip()).into(), + url_ip: (*url_ip.ip()).into(), + }); + } + } + (SocketAddr::V6(src_ip), SocketAddr::V4(url_ip)) => { + return Err(SearchError::SpoofedIp { + src_ip: (*src_ip.ip()).into(), + url_ip: (*url_ip.ip()).into(), + }) + } + (SocketAddr::V4(src_ip), SocketAddr::V6(url_ip)) => { + return Err(SearchError::SpoofedIp { + src_ip: (*src_ip.ip()).into(), + url_ip: (*url_ip.ip()).into(), + }) + } + } + let (control_schema_url, control_url) = run_with_timeout(options.http_timeout, get_control_urls(&addr, &root_url)).await??; let control_schema = @@ -126,3 +157,47 @@ async fn get_control_schemas( let c = std::io::Cursor::new(&resp); parsing::parse_schemas(c) } + +#[cfg(test)] +mod tests { + use super::*; + use std::{ + net::{Ipv4Addr, SocketAddrV4}, + time::Duration, + }; + use test_log::test; + + #[test(tokio::test)] + async fn ip_spoofing_in_broadcast_response() { + let port = { + // Not 100% reliable way to find a free port number, but should be good enough + let sock = UdpSocket::bind((Ipv4Addr::UNSPECIFIED, 0)).await.unwrap(); + sock.local_addr().unwrap().port() + }; + + let options = SearchOptions { + bind_addr: SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, port)), + timeout: Some(Duration::from_secs(5)), + http_timeout: Some(Duration::from_secs(1)), + ..Default::default() + }; + + tokio::spawn(async move { + tokio::time::sleep(Duration::from_secs(1)).await; + + let sock = UdpSocket::bind((Ipv4Addr::LOCALHOST, 0)).await.unwrap(); + + sock.send_to(b"location: http://1.2.3.4:5/test", (Ipv4Addr::LOCALHOST, port)) + .await + .unwrap(); + }); + + let result = search_gateway(options).await; + if let Err(SearchError::SpoofedIp { src_ip, url_ip }) = result { + assert_eq!(src_ip, Ipv4Addr::LOCALHOST); + assert_eq!(url_ip, Ipv4Addr::new(1, 2, 3, 4)); + } else { + panic!("Unexpected result: {result:?}"); + } + } +} diff --git a/src/errors.rs b/src/errors.rs index 2d0e4810..e2c1b93f 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,6 +1,7 @@ use std::error; use std::fmt; use std::io; +use std::net::IpAddr; use std::str; #[cfg(feature = "aio")] use std::string::FromUtf8Error; @@ -313,6 +314,13 @@ pub enum SearchError { /// Error parsing URI #[cfg(feature = "aio")] InvalidUri(hyper::http::uri::InvalidUri), + /// Ip spoofing detected error + SpoofedIp { + /// The IP from which packet was actually received + src_ip: IpAddr, + /// The IP which the receiving packet pretended to be from + url_ip: IpAddr, + }, } impl From for SearchError { @@ -371,6 +379,7 @@ impl fmt::Display for SearchError { SearchError::HyperError(ref e) => write!(f, "Hyper Error: {}", e), #[cfg(feature = "aio")] SearchError::InvalidUri(ref e) => write!(f, "InvalidUri Error: {}", e), + SearchError::SpoofedIp { src_ip, url_ip } => write!(f, "Spoofed IP from {src_ip} as {url_ip}"), } } } @@ -387,6 +396,7 @@ impl error::Error for SearchError { SearchError::HyperError(ref e) => Some(e), #[cfg(feature = "aio")] SearchError::InvalidUri(ref e) => Some(e), + SearchError::SpoofedIp { .. } => None, } } }