diff --git a/code_review_report.md b/code_review_report.md new file mode 100644 index 0000000..118115a --- /dev/null +++ b/code_review_report.md @@ -0,0 +1,121 @@ +# Punchnet (sdlan-lib-rs) Code Review Report + +This report provides a detailed, comprehensive review of the `punchnet` SD-LAN (Software-Defined LAN / VPN) client repository. It covers the overall architecture, highlights key design strengths, details security and code quality risks, and suggests practical refactoring opportunities. + +--- + +## 1. Repository Overview & Architecture + +`punchnet` is a virtual networking client written in Rust designed to establish secure, peer-to-peer Software-Defined LAN connections. + +The architecture consists of two main planes: +1. **Control Plane (QUIC)**: Communicates with a centralized supernode over QUIC (via `quinn`) to handle registration (`RegisterSuper`), policy exchange, peer discovery, and NAT type discovery. +2. **Data Plane (UDP)**: Encrypted virtual network traffic is sent directly between peer nodes using UDP. It performs NAT traversal (using STUN-like hole punching) to establish direct P2P connections, falling back to relaying via the supernode if direct routing fails. + +### Core Components + +```mermaid +graph TD + A[OS Virtual NIC: TUN/TAP] <-->|Read/Write L3/L2 Packets| B[Packet Routing Engine: packet.rs] + B <-->|Encrypt/Decrypt: AES or ChaCha20| C[UDP Socket Engine: socks.rs] + D[Control Plane: quic.rs] -->|Session Keys / ACL Rules| B + D <-->|QUIC Protocol| E[Supernode Server] + C <-->|P2P Encrypted Data| F[Remote Peers] + B -->|Local Domain Filter| G[DNS Hijacking Proxy: dns.rs] + G <-->|Proxy Queries| H[AliDNS 223.5.5.5 / Supernode DNS] +``` + +* **Platform-specific NIC Adapters** (`src/network/tun_win.rs` & `src/network/tun_linux.rs`): Handles raw packet read/write. Windows implements L3 TUN using the Wintun driver (relying on `wintun.dll`), simulating Ethernet frames (L2) locally. Linux binds directly to `/dev/net/tun` via `ioctl` (supported by helper C-code `tuntap.c`). +* **Routing & Encapsulation Engine** (`src/network/packet.rs` & `src/network/route.rs`): Identifies packet destination MACs and resolves them to peer IP/ports using a custom ARP table. Packs IP/Ethernet payloads into protobuf messages (`SdlData`), encrypts them, and forwards them. Uses a custom lock-free IP Trie (`src/utils/system_action.rs`) for prefix route resolution. +* **DNS Proxy Engine** (`src/utils/dns.rs`): Automatically intercepts DNS queries. Queries for private virtual domains (e.g. `*.network_domain`) are sent to the supernode's DNS server (`server_ip:15353`), while other public queries are proxied to AliDNS (`223.5.5.5:53`) over dynamically rotated UDP sockets. + +--- + +## 2. Key Design Highlights (Strengths) + +* **Lock-Free Concurrent Routing Lookups**: + Outbound lookups on the routing table (`RouteTable2`) happen on every single packet. The project handles this very efficiently by implementing an `IpTrie` wrapped in an `ArcSwap`. Write operations (inserting or clearing routes) clone the trie and perform an atomic pointer swap, enabling read threads to query routes concurrently without locking. +* **Dynamic DNS Socket Rotation**: + To prevent firewalls or stateful NAT routers from blocking persistent DNS proxy connections, `DynamicDNSClient` rotates its local UDP socket either every 60 seconds or after 100 queries. It spawns a new receiver thread on a fresh port and allows the old socket to time out and close gracefully, which is a highly resilient approach to DNS proxying. +* **Clean Separation of Control and Data Planes**: + Using QUIC for signaling and standard UDP for data plane routing provides a robust control connection (resilient to head-of-line blocking and network changes) while keeping the overhead of data forwarding as low as possible. + +--- + +## 3. Security Vulnerabilities & Risks + +> [!CAUTION] +> ### 1. ACL/Firewall Rules Completely Bypassed +> In [acl_session.rs](file:///d:/work/rust/sdlan-lib-rs/src/utils/acl_session.rs#L141-L146), the core firewall verification method `is_identity_ok` unconditionally returns `(true, false)`: +> ```rust +> pub fn is_identity_ok(&self, allow_routing: bool, identity: IdentityID, info: FiveTuple) -> (bool, ShouldRenew) { +> if allow_routing { +> return (true, false); +> } +> return (true, false); +> +> error!("is identity ok? {:?}", info); +> // ... remaining validation logic is dead code ... +> } +> ``` +> **Risk**: Any peer on the SD-LAN network can send traffic to any port of the client, bypassing all security group policies pushed from the supernode server. + +> [!WARNING] +> ### 2. Hardcoded Signature Key for HTTP API +> In [api/mod.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/api/mod.rs#L10), the signing key for REST APIs is hardcoded: +> ```rust +> const DIGEST_KEY: &'static str = "H6p*2RfEu4ITcL"; +> ``` +> **Risk**: This key is used to sign requests sent to the central controller via HMAC-MD5 (sent as `X-sign`). Since the key is hardcoded in the client, an attacker could reverse-engineer the binary, retrieve the key, and forge authenticated API requests. + +> [!WARNING] +> ### 3. Weak Directory Permissions for Local Credentials +> In [node.rs](file:///d:/work/rust/sdlan-lib-rs/src/network/node.rs#L64-L73) and [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L344), the client directory `.client` is created inside the base directory to hold the generated RSA key pair (`id_rsa`/`id_rsa.pub`). The code does not restrict folder permissions on Unix. +> **Risk**: If the app is run as root (needed to configure TUN/TAP) and saves files in a shared base directory (e.g. `/usr/local/punchnet`), other non-root local users might be able to read the private key file `id_rsa` if the default `umask` is weak. + +--- + +## 4. Code Quality & Potential Bugs + +### 1. DNS Settings Corruption on Improper Exit +In [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L268-L280), the code restores system DNS settings on `ctrl_c` by invoking `restore_dns`. +* **Bug**: If the process crashes, panics, is killed via `kill -9` (`SIGKILL`), or system shutdown happens without triggering the tokio runtime signal handler, the system's DNS settings (e.g., `/etc/resolv.conf` on Linux or static DNS addresses on Windows) will remain pointed to the hijacked address `100.100.100.100`. This will break the host machine's internet connectivity until manually fixed. + +### 2. PID File Race Conditions & False Positives +In [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L633-L645), `is_process_running()` checks for the PID file `/tmp/punchnet.pid` and verifies whether that process ID exists in the system: +```rust +fn is_pid_running(pid: u32) -> bool { + let mut sys = sysinfo::System::new_all(); + sys.refresh_all(); + sys.process(sysinfo::Pid::from_u32(pid)).is_some() +} +``` +* **Bug**: If `punchnet` crashes, the PID file is left over. If another completely unrelated application starts later and gets assigned the exact same process ID, `punchnet` will refuse to launch, erroneously reporting `"process is running, aborting..."`. It is safer to verify if the process name matches `"punchnet"` or check the binary executable path. + +### 3. Blocking Operations on Async Thread Pool +In `async_main.rs` and `node.rs`, there are multiple instances where filesystem operations (`fs::read_to_string`, `fs::write`) or platform calls (`Command::new("netsh")`, `Command::new("route")`) are executed synchronously within async tokio contexts instead of using `tokio::fs` or `spawn_blocking`. +* **Performance Impact**: These blocking system operations can stall the Tokio worker threads, reducing throughput and leading to increased latency in packet forwarding. + +--- + +## 5. Suggestions for Improvement & Refactoring + +1. **Fix the ACL Shortcut**: + Remove the early `return (true, false)` from `is_identity_ok` in `src/utils/acl_session.rs` and fully integrate the rules received from the server to secure the client against unauthorized peer access. +2. **Move Secret Keys to Environment/Config**: + Remove `DIGEST_KEY` from source files. Instead, use a key derivation mechanism or load it from a secure local environment variable or configuration file. +3. **Ensure Atomic & Safe File Permissions**: + When creating folders or writing key pairs on Unix, restrict the directory permissions explicitly to `0700` and files to `0600` using standard library extensions: + ```rust + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + let mut builder = std::fs::DirBuilder::new(); + builder.mode(0o700); + builder.create(rsa_path)?; + } + ``` +4. **Use Subprocess Wrappers and Async Routines**: + Migrate blocking file I/O and process spawns to async counterparts (e.g. `tokio::fs` and `tokio::process::Command`) to prevent worker thread starvation. +5. **Graceful Recovery of System DNS Settings**: + Consider writing a secondary daemon / sentinel wrapper, or register a shell-level script trap to ensure that system DNS settings are automatically restored even under fatal panics/crashes. diff --git a/src/bin/punchnet/main.rs b/src/bin/punchnet/main.rs index c40a995..3e898e4 100755 --- a/src/bin/punchnet/main.rs +++ b/src/bin/punchnet/main.rs @@ -338,6 +338,11 @@ async fn login_with_token_or_user_pass( } fn main() { + #[cfg(not(target_os = "windows"))] + unsafe { + libc::umask(0o077); + } + set_base_dir(DEFAULT_BASE_DIR); // let _guard = log::init_log(&format!("{}/.output", get_base_dir()));