From 7c07b0f48a2bfab527c43e083f210e471c0d316e Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 08:18:41 +0200 Subject: [PATCH] Fix memory-safety and hang bugs in the native packet engine All found in an audit of the C engine and inherited from the NetGuard import unless noted: - dhcp.c: the DHCP reply options were written ~57KB past the 500-byte response allocation ("response + sizeof(struct dhcp_packet)" advances in struct-sized units). Reachable by any co-installed app with a single UDP datagram to port 67 -- no special permission needed. This is the headline fix. - ip.c, tcp.c: validate the TCP data offset before deriving the payload pointer/length from it. A doff below 5 (or option bytes past the segment) previously wrapped datalen to ~65K; the read this enabled was out of bounds past the end of the get_mtu() tun read buffer, not an overflow of the (already length-checked) SNI reassembly buffer. Only reachable via a packet the kernel writes into the tun for a local app, so it needs CAP_NET_RAW/root -- defensive hardening, not exploitable today. - ip.c: the IPv4 header-length underflow this doff fix mirrors -- "(ihl - 5) * 4" wraps to 236 when ihl == 0, and the length checks further down are size_t so they wrap too and pass -- gets the same ihl < 5 guard next to the existing tot_len check. Same reachability caveat as above. - tcp.c: a SYN option with length 0 could spin the event-loop thread forever; option walking now always progresses and stays inside the declared options region. (optlen is signed, so an over-long option already exited via the negative-length check; only the len == 0 case actually hung.) Same reachability caveat as above. - icmp.c: ICMPv6 echo replies computed a bogus pseudo-header length (bytes - sizeof(ip6_hdr)) in host order; use htonl(bytes), matching the query path. - pcap.c: rolling over the PCAP file skipped fflush before ftruncate, and re-seeked with lseek(fileno(...)) afterwards, which never syncs the FILE*'s own buffered offset -- ftell() kept reporting the pre-rollover size and the file re-truncated on every later write. fflush before ftruncate, then fseek() (not lseek on the fd) on the FILE* itself. - Hardening: vsnprintf for log/exit formatting, bounded SOCKS5 string copies from Java, seeded rand() for ISNs. Not included: a walk of IPv6 extension headers was dropped from this change after review found it unsafe (wrong RFC 8200 unit, ESP/AH have no walkable next-header field, and non-first fragments would be misparsed as L4 data) -- tracked separately for a correct implementation. No behavior change for well-formed packets: every new check only rejects input that was previously misparsed or impossible. --- app/src/main/jni/netguard/dhcp.c | 2 +- app/src/main/jni/netguard/icmp.c | 2 +- app/src/main/jni/netguard/ip.c | 10 ++++++++++ app/src/main/jni/netguard/netguard.c | 17 ++++++++++++----- app/src/main/jni/netguard/pcap.c | 7 +++++-- app/src/main/jni/netguard/tcp.c | 28 ++++++++++++++++++++-------- app/src/main/jni/netguard/util.c | 2 +- 7 files changed, 50 insertions(+), 18 deletions(-) diff --git a/app/src/main/jni/netguard/dhcp.c b/app/src/main/jni/netguard/dhcp.c index 0852310ba..3a821ff43 100644 --- a/app/src/main/jni/netguard/dhcp.c +++ b/app/src/main/jni/netguard/dhcp.c @@ -81,7 +81,7 @@ int check_dhcp(const struct arguments *args, const struct udp_session *u, memset(&response->giaddr, 0, sizeof(response->giaddr)); // https://tools.ietf.org/html/rfc2132 - uint8_t *options = (uint8_t *) (response + sizeof(struct dhcp_packet)); + uint8_t *options = (uint8_t *) (response + 1); int idx = 0; *(options + idx++) = 53; // Message type diff --git a/app/src/main/jni/netguard/icmp.c b/app/src/main/jni/netguard/icmp.c index c4fbf0a1a..d146192c1 100644 --- a/app/src/main/jni/netguard/icmp.c +++ b/app/src/main/jni/netguard/icmp.c @@ -124,7 +124,7 @@ void check_icmp_socket(const struct arguments *args, const struct epoll_event *e memset(&pseudo, 0, sizeof(struct ip6_hdr_pseudo)); memcpy(&pseudo.ip6ph_src, &s->icmp.daddr.ip6, 16); memcpy(&pseudo.ip6ph_dst, &s->icmp.saddr.ip6, 16); - pseudo.ip6ph_len = bytes - sizeof(struct ip6_hdr); + pseudo.ip6ph_len = htonl((uint32_t) bytes); pseudo.ip6ph_nxt = IPPROTO_ICMPV6; csum = calc_checksum( 0, (uint8_t *) &pseudo, sizeof(struct ip6_hdr_pseudo)); diff --git a/app/src/main/jni/netguard/ip.c b/app/src/main/jni/netguard/ip.c index 1b7a4d7a7..ae316d806 100644 --- a/app/src/main/jni/netguard/ip.c +++ b/app/src/main/jni/netguard/ip.c @@ -338,6 +338,11 @@ void handle_ip(const struct arguments *args, uint8_t ipoptlen = (uint8_t) ((ip4hdr->ihl - 5) * 4); payload = (uint8_t *) (pkt + sizeof(struct iphdr) + ipoptlen); + if (ip4hdr->ihl < 5 || sizeof(struct iphdr) + ipoptlen > length) { + log_android(ANDROID_LOG_WARN, "IP4 invalid header length"); + return; + } + if (ntohs(ip4hdr->tot_len) != length) { log_android(ANDROID_LOG_ERROR, "Invalid length %u header length %u", length, ntohs(ip4hdr->tot_len)); @@ -563,6 +568,11 @@ void handle_ip(const struct arguments *args, const struct ip6_hdr *ip6 = (struct ip6_hdr *) pkt; const struct tcphdr *tcphdr = (struct tcphdr *) payload; const uint8_t tcpoptlen = (uint8_t) ((tcphdr->doff - 5) * 4); + if (tcphdr->doff < 5 || + sizeof(struct tcphdr) + tcpoptlen > (size_t) (length - (payload - pkt))) { + log_android(ANDROID_LOG_WARN, "TCP invalid data offset"); + return; + } const uint8_t *tcpoptions = payload + sizeof(struct tcphdr); const uint8_t *data = payload + sizeof(struct tcphdr) + tcpoptlen; const uint16_t datalen = (const uint16_t) (length - (data - pkt)); diff --git a/app/src/main/jni/netguard/netguard.c b/app/src/main/jni/netguard/netguard.c index 37a7f2e8a..4988aa212 100644 --- a/app/src/main/jni/netguard/netguard.c +++ b/app/src/main/jni/netguard/netguard.c @@ -146,6 +146,8 @@ void JNI_OnUnload(JavaVM *vm, void *reserved) { JNIEXPORT jlong JNICALL Java_eu_faircode_netguard_ServiceSinkhole_jni_1init( JNIEnv *env, jobject instance, jint sdk) { + srand((unsigned int) (time(NULL) ^ getpid())); + // Resolve the routing policy now: the packet path must never pay a dlopen, // and a failure should be logged while there is still something to read it. policy_ensure(); @@ -368,10 +370,15 @@ Java_eu_faircode_netguard_ServiceSinkhole_jni_1socks5(JNIEnv *env, jobject insta ng_add_alloc(username, "username"); ng_add_alloc(password, "password"); - strcpy(socks5_addr, addr); + if (snprintf(socks5_addr, sizeof(socks5_addr), "%s", addr) >= (int) sizeof(socks5_addr)) + log_android(ANDROID_LOG_WARN, "SOCKS5 address truncated"); socks5_port = port; - strcpy(socks5_username, username); - strcpy(socks5_password, password); + if (snprintf(socks5_username, sizeof(socks5_username), "%s", username) >= + (int) sizeof(socks5_username)) + log_android(ANDROID_LOG_WARN, "SOCKS5 username truncated"); + if (snprintf(socks5_password, sizeof(socks5_password), "%s", password) >= + (int) sizeof(socks5_password)) + log_android(ANDROID_LOG_WARN, "SOCKS5 password truncated"); log_android(ANDROID_LOG_WARN, "SOCKS5 %s:%d user=%s", socks5_addr, socks5_port, socks5_username); @@ -584,7 +591,7 @@ void report_exit(const struct arguments *args, int error, const char *fmt, ...) char line[1024]; va_list argptr; va_start(argptr, fmt); - vsprintf(line, fmt, argptr); + vsnprintf(line, sizeof(line), fmt, argptr); jreason = (*args->env)->NewStringUTF(args->env, line); ng_add_alloc(jreason, "jreason"); va_end(argptr); @@ -611,7 +618,7 @@ void report_error(const struct arguments *args, jint error, const char *fmt, ... char line[1024]; va_list argptr; va_start(argptr, fmt); - vsprintf(line, fmt, argptr); + vsnprintf(line, sizeof(line), fmt, argptr); jreason = (*args->env)->NewStringUTF(args->env, line); ng_add_alloc(jreason, "jreason"); va_end(argptr); diff --git a/app/src/main/jni/netguard/pcap.c b/app/src/main/jni/netguard/pcap.c index 134c49d99..5997929bd 100644 --- a/app/src/main/jni/netguard/pcap.c +++ b/app/src/main/jni/netguard/pcap.c @@ -65,11 +65,14 @@ void write_pcap(const void *ptr, size_t len) { if (fsize > pcap_file_size) { log_android(ANDROID_LOG_WARN, "PCAP truncate @%ld", fsize); - if (ftruncate(fileno(pcap_file), sizeof(struct pcap_hdr_s))) + if (fflush(pcap_file)) + log_android(ANDROID_LOG_ERROR, "PCAP fflush error %d: %s", + errno, strerror(errno)); + else if (ftruncate(fileno(pcap_file), sizeof(struct pcap_hdr_s))) log_android(ANDROID_LOG_ERROR, "PCAP ftruncate error %d: %s", errno, strerror(errno)); else { - if (!lseek(fileno(pcap_file), sizeof(struct pcap_hdr_s), SEEK_SET)) + if (fseek(pcap_file, sizeof(struct pcap_hdr_s), SEEK_SET)) log_android(ANDROID_LOG_ERROR, "PCAP ftruncate error %d: %s", errno, strerror(errno)); } diff --git a/app/src/main/jni/netguard/tcp.c b/app/src/main/jni/netguard/tcp.c index d0bf405f0..92a67b64b 100644 --- a/app/src/main/jni/netguard/tcp.c +++ b/app/src/main/jni/netguard/tcp.c @@ -649,6 +649,11 @@ jboolean handle_tcp(const struct arguments *args, const struct ip6_hdr *ip6 = (struct ip6_hdr *) pkt; const struct tcphdr *tcphdr = (struct tcphdr *) payload; const uint8_t tcpoptlen = (uint8_t) ((tcphdr->doff - 5) * 4); + if (tcphdr->doff < 5 || + sizeof(struct tcphdr) + tcpoptlen > (size_t) (length - (payload - pkt))) { + log_android(ANDROID_LOG_WARN, "TCP invalid data offset"); + return 0; + } const uint8_t *tcpoptions = payload + sizeof(struct tcphdr); const uint8_t *data = payload + sizeof(struct tcphdr) + tcpoptlen; const uint16_t datalen = (const uint16_t) (length - (data - pkt)); @@ -718,23 +723,30 @@ jboolean handle_tcp(const struct arguments *args, uint8_t *options = (uint8_t *) tcpoptions; while (optlen > 0) { uint8_t kind = *options; - uint8_t len = *(options + 1); if (kind == 0) // End of options list break; + if (kind == 1) { + optlen--; + options++; + continue; + } + + if (optlen < 2) + break; + + uint8_t len = *(options + 1); + if (len < 2 || len > optlen) + break; + if (kind == 2 && len == 4) mss = ntohs(*((uint16_t *) (options + 2))); else if (kind == 3 && len == 3) ws = *(options + 2); - if (kind == 1) { - optlen--; - options++; - } else { - optlen -= len; - options += len; - } + optlen -= len; + options += len; } // In tethering compatibility mode, clamp the MSS we use for diff --git a/app/src/main/jni/netguard/util.c b/app/src/main/jni/netguard/util.c index c81bdfe31..a933ddcc9 100644 --- a/app/src/main/jni/netguard/util.c +++ b/app/src/main/jni/netguard/util.c @@ -65,7 +65,7 @@ void log_android(int prio, const char *fmt, ...) { char line[1024]; va_list argptr; va_start(argptr, fmt); - vsprintf(line, fmt, argptr); + vsnprintf(line, sizeof(line), fmt, argptr); __android_log_print(prio, TAG, "%s", line); va_end(argptr); }