1.0 RFC compliance review bugs

[totoCZ]

The 0.2 branch has major structural issues, so I’m moving to 1.0.

Here are Claude identified bugs

ndppd 1.0-devel: Three bugs in NS/NA receive path

Branch: 1.0-devel
Commit: e1746bb59706adba96506ba06e30da0762638265
File: src/iface.c, src/session.c

---

Bug 1 — Missing hop limit check on received NS/NA (RFC 4861 §6.1.1)

Severity: Low (correctness / compliance)

Description

RFC 4861 §6.1.1 states that a node MUST silently discard NS/NA messages where
the IP hop limit is not 255. The check exists to reject messages that have
been IPv6-forwarded (which would have decremented the hop limit), since NDP is
strictly link-scoped and routers MUST NOT forward NS or NA (§7).

ndL_handle_msg() in src/iface.c verifies the ICMPv6 checksum correctly but
never checks ip6_hops before dispatching to ndL_handle_ns() or
ndL_handle_na(). The 0.2.x branch has had this check since commit fecd1ab.

Affected code

// src/iface.c — ndL_handle_msg(), after checksum verification:
if (ndL_calculate_icmp6_checksum(&msg->ip6h, ih, ilen) != ih->icmp6_cksum)
    return;

// Missing check here

if (ih->icmp6_type == ND_NEIGHBOR_SOLICIT)
    ndL_handle_ns(iface, &msg->ip6h, ih, ilen);

Fix

if (ndL_calculate_icmp6_checksum(&msg->ip6h, ih, ilen) != ih->icmp6_cksum)
    return;

if (msg->ip6h.ip6_hops != 255)
    return;

if (ih->icmp6_type == ND_NEIGHBOR_SOLICIT)
    ndL_handle_ns(iface, &msg->ip6h, ih, ilen);

---

Bug 2 — Unicast NS without SLLAO silently dropped (RFC 4861 §4.3)

Severity: High (functional — causes proxy to not respond to valid solicitations)

Description

RFC 4861 §4.3 says the Source Link-Layer Address Option (SLLAO) MUST be
included in multicast NS, and SHOULD be included in unicast NS. The SHOULD
means a conforming implementation MAY send a unicast NS without SLLAO and the
receiver must still process it.

ndL_handle_ns() currently handles the DAD case (unspecified source, no SLLAO)
correctly by leaving src_ll = NULL. For all other NS (non-DAD), if the SLLAO
is absent or the first option is not ND_OPT_SOURCE_LINKADDR, the function
returns early and the NS is silently dropped. A FIXME comment in the code
acknowledges this is wrong.

This is a real functional regression for environments with strict or unusual
implementations that send unicast NS without SLLAO — including certain router
dead-peer-detection implementations that send unicast NS to verify neighbor
reachability. Such a router would never get an NA back, causing it to expire
the neighbor entry and drop traffic.

Affected code

// src/iface.c — ndL_handle_ns():
if (!nd_addr_is_unspecified((nd_addr_t *)&ip6h->ip6_src)) {
    // FIXME: Source link-layer address MUST be included in multicast
    // solicitations and SHOULD be included in unicast solicitations.
    // [https://tools.ietf.org/html/rfc4861#section-4.3].

    if (len - sizeof(struct nd_neighbor_solicit) < 8)
        return;                                         // <-- silent drop

    struct nd_opt_hdr *opt = (struct nd_opt_hdr *)((void *)ns + sizeof(struct nd_neighbor_solicit));

    if (opt->nd_opt_len != 1 || opt->nd_opt_type != ND_OPT_SOURCE_LINKADDR)
        return;                                         // <-- silent drop

    src_ll = (nd_lladdr_t *)((void *)opt + 2);
}

The function then unconditionally passes src_ll to nd_proxy_handle_ns(),
which passes it to nd_session_handle_ns(). When src_ll is NULL and the
session is VALID or STALE, the send-NA path uses src_ll directly as the
destination link-layer address without a NULL check (see Bug 3).

Fix

For non-DAD unicast NS, src_ll being absent is valid. The proxy can still
respond — it just cannot include a Target Link-Layer Address Option in the NA
that is useful to the soliciting node for direct delivery. Since ndppd is
sending the NA on behalf of a proxied address anyway, the proxy’s own MAC is
used as the link-layer address, which is correct regardless.

if (!nd_addr_is_unspecified((nd_addr_t *)&ip6h->ip6_src)) {
    if (len - sizeof(struct nd_neighbor_solicit) >= 8) {
        struct nd_opt_hdr *opt = (struct nd_opt_hdr *)((void *)ns + sizeof(struct nd_neighbor_solicit));

        if (opt->nd_opt_len == 1 && opt->nd_opt_type == ND_OPT_SOURCE_LINKADDR)
            src_ll = (nd_lladdr_t *)((void *)opt + 2);
        // No SLLAO or unrecognised option: src_ll stays NULL, continue processing.
        // RFC 4861 §4.3: SLLAO SHOULD (not MUST) be present in unicast NS.
    }
    // Packet too short to contain any option: src_ll stays NULL, continue.
}

nd_session_handle_ns() and nd_iface_send_na() must then handle src_ll = NULL gracefully (see Bug 3).

---

Bug 3 — NULL dereference in subscriber path when src_ll is NULL

Severity: High (crash — SIGSEGV)

Description

This bug is latent today (masked by Bug 2’s early return) but would become a
crash if Bug 2 is fixed without also fixing this. It is also independently
reachable via the DAD path under specific timing conditions (see below).

nd_session_handle_ns() in src/session.c is called with src_ll = NULL
when the NS source is unspecified (DAD). When the session state is not VALID or
STALE (i.e., INCOMPLETE or INVALID), the function searches the subscriber list
and then attempts to create a new subscriber:

// src/session.c — nd_session_handle_ns():
nd_sub_t *sub;
ND_LL_SEARCH(session->subs, sub, next,
    nd_addr_eq(&sub->addr, src) && nd_lladdr_eq(&sub->lladdr, src_ll));
                                                   // ^^ memcmp(ptr, NULL, 6) if src_ll is NULL

if (!sub) {
    sub = ND_NEW(nd_sub_t);
    sub->addr = *src;
    sub->lladdr = *src_ll;   // ^^ NULL dereference if src_ll is NULL
    ND_LL_PREPEND(session->subs, sub, next);
}

nd_lladdr_eq() calls memcmp(first, second, sizeof(nd_lladdr_t)) with no
NULL guard, and the assignment sub->lladdr = *src_ll dereferences NULL
directly.

Crash scenario (DAD + INCOMPLETE session): A container or host performs
DAD for an IPv6 address that ndppd is actively proxying. ndppd has a session
for that target in INCOMPLETE state (it sent an NS upstream and is waiting for
an NA). The DAD NS arrives with source :: and no SLLAO. ndL_handle_ns
passes src_ll = NULL to nd_proxy_handle_ns, which calls
nd_session_handle_ns. Session is INCOMPLETE → subscriber path → SIGSEGV.

Note on current exposure: With Bug 2 present, non-DAD NS without SLLAO
never reach nd_session_handle_ns. Once Bug 2 is fixed, any NS without SLLAO
hitting a non-VALID/STALE session will also crash here. The two fixes must be
applied together.

Fix

Guard the subscriber path against NULL src_ll. For NA dispatch, src_ll = NULL means we cannot send a unicast NA at the Ethernet level; ndppd should
skip queuing that subscriber or send to the all-nodes multicast address as it
already does for the DAD/unspecified-source case.

void nd_session_handle_ns(nd_session_t *session, const nd_addr_t *src, const nd_lladdr_t *src_ll)
{
    session->ins_time = nd_current_time;

    if (session->state != ND_STATE_VALID && session->state != ND_STATE_STALE) {
        // Cannot queue a subscriber without a link-layer address to reply to.
        if (!src_ll)
            return;

        nd_sub_t *sub;
        ND_LL_SEARCH(session->subs, sub, next,
            nd_addr_eq(&sub->addr, src) && nd_lladdr_eq(&sub->lladdr, src_ll));

        if (!sub) {
            sub = ND_NEW(nd_sub_t);
            sub->addr = *src;
            sub->lladdr = *src_ll;
            ND_LL_PREPEND(session->subs, sub, next);
        }

        return;
    }

    // VALID/STALE: send NA immediately.
    nd_lladdr_t *tgt_ll = !nd_lladdr_is_unspecified(&session->rule->target)
                              ? &session->rule->target : NULL;

    if (nd_addr_is_unspecified(src) || !src_ll) {
        static const nd_lladdr_t allnodes_ll = { .u8 = { 0x33, 0x33, [5] = 1 } };
        static const nd_addr_t allnodes = { .u8 = { 0xff, 0x02, [15] = 1 } };
        nd_iface_send_na(session->rule->proxy->iface, &allnodes, &allnodes_ll,
                         &session->tgt, tgt_ll, session->rule->proxy->router);
    } else {
        nd_iface_send_na(session->rule->proxy->iface, src, src_ll,
                         &session->tgt, tgt_ll, session->rule->proxy->router);
    }
}

---

Summary

#	Location	RFC	Severity	Practical impact
1	iface.c: ndL_handle_msg()	§6.1.1 MUST	Low	Compliance only; forwarded NS cannot appear on a correct L2 segment
2	iface.c: ndL_handle_ns()	§4.3 SHOULD	High	Valid unicast NS without SLLAO silently dropped; proxy never responds
3	session.c: nd_session_handle_ns()	—	High	SIGSEGV on DAD NS hitting INCOMPLETE session; also triggered by Bug 2 fix

Bugs 2 and 3 should be fixed together. Bug 1 can be applied independently.

[totoCZ]

Issue	0.2.7-devel	1.0-devel
Hop limit 255 check on receive (RFC §6.1.1)	✅	❌ missing
ICMPv6 checksum verification	❌	✅
Unicast NS → immediate NA response	⚠️ once per INVALID window	✅ every time
NA SOLICITED flag on unicast	✅	✅
NA OVERRIDE flag	❌ omitted	✅
NA source = target address	⚠️ kernel decides	✅ explicit
INVALID session cleanup on restart	✅ (deadtime)	✅ (ttl + delete)
Multiple NS during INCOMPLETE	⚠️ _touched blocks re-probe	✅ subscriber queue
Hop-by-hop header support	❌	✅
PACKET_OUTGOING loop prevention	✅ (top commit)	N/A (single socket)
Production stability	stable-ish	alpha, not production

[totoCZ]

There is also experimental “use-kernel” option in v1.

• duplicates NDP proxying to kernel (both ndppd and kernel respond)
• kernel itself doesn’t handle unicast NDP - that’s why we have ndppd instead of their crappy implementation
• does not do anything except duplicate packets

Probably should be deleted.

[totoCZ]

Applied patch

I will add more if I discover bugs or instability.

Off to production!

diff --git a/src/iface.c b/src/iface.c
index b8bd27c..206c95d 100644
--- a/src/iface.c
+++ b/src/iface.c
@@ -78,18 +78,15 @@ static void ndL_handle_ns(nd_iface_t *iface, struct ip6_hdr *ip6h, struct icmp6_
     nd_lladdr_t *src_ll = NULL;
 
     if (!nd_addr_is_unspecified((nd_addr_t *)&ip6h->ip6_src)) {
-        // FIXME: Source link-layer address MUST be included in multicast solicitations and SHOULD be included in
-        //        unicast solicitations. [https://tools.ietf.org/html/rfc4861#section-4.3].
-
-        if (len - sizeof(struct nd_neighbor_solicit) < 8)
-            return;
-
-        struct nd_opt_hdr *opt = (struct nd_opt_hdr *)((void *)ns + sizeof(struct nd_neighbor_solicit));
-
-        if (opt->nd_opt_len != 1 || opt->nd_opt_type != ND_OPT_SOURCE_LINKADDR)
-            return;
-
-        src_ll = (nd_lladdr_t *)((void *)opt + 2);
+        /* RFC 4861 §4.3: SLLAO MUST be present in multicast NS, SHOULD be present in unicast NS.
+         * SHOULD means a conforming sender may omit it; we must still process the NS.
+         * Parse SLLAO if present; leave src_ll = NULL if absent — handled downstream. */
+        if (len - sizeof(struct nd_neighbor_solicit) >= 8) {
+            struct nd_opt_hdr *opt = (struct nd_opt_hdr *)((void *)ns + sizeof(struct nd_neighbor_solicit));
+
+            if (opt->nd_opt_len == 1 && opt->nd_opt_type == ND_OPT_SOURCE_LINKADDR)
+                src_ll = (nd_lladdr_t *)((void *)opt + 2);
+        }
     }
 
     nd_proxy_handle_ns(iface->proxy, (nd_addr_t *)&ip6h->ip6_src, (nd_addr_t *)&ip6h->ip6_dst,
@@ -199,6 +196,10 @@ static void ndL_handle_msg(nd_iface_t *iface, ndL_ip6_msg_t *msg)
     if (ndL_calculate_icmp6_checksum(&msg->ip6h, ih, ilen) != ih->icmp6_cksum)
         return;
 
+    /* RFC 4861 §6.1.1: hop limit MUST be 255 — reject anything that may have been forwarded. */
+    if (msg->ip6h.ip6_hops != 255)
+        return;
+
     if (ih->icmp6_type == ND_NEIGHBOR_SOLICIT)
         ndL_handle_ns(iface, &msg->ip6h, ih, ilen);
     else if (ih->icmp6_type == ND_NEIGHBOR_ADVERT)
diff --git a/src/session.c b/src/session.c
index bfffa58..7c1b11a 100644
--- a/src/session.c
+++ b/src/session.c
@@ -71,6 +71,12 @@ void nd_session_handle_ns(nd_session_t *session, const nd_addr_t *src, const nd_
     session->ins_time = nd_current_time;
 
     if (session->state != ND_STATE_VALID && session->state != ND_STATE_STALE) {
+        /* Bug 3 fix: src_ll may be NULL for unicast NS without SLLAO (RFC 4861 §4.3 SHOULD)
+         * or for DAD NS (unspecified source). Cannot queue a subscriber without a link-layer
+         * address to reply to — skip silently rather than crash. */
+        if (!src_ll)
+            return;
+
         nd_sub_t *sub;
         ND_LL_SEARCH(session->subs, sub, next, nd_addr_eq(&sub->addr, src) && nd_lladdr_eq(&sub->lladdr, src_ll));
 
@@ -86,7 +92,9 @@ void nd_session_handle_ns(nd_session_t *session, const nd_addr_t *src, const nd_
 
     nd_lladdr_t *tgt_ll = !nd_lladdr_is_unspecified(&session->rule->target) ? &session->rule->target : NULL;
 
-    if (nd_addr_is_unspecified(src)) {
+    /* Bug 3 fix: treat NULL src_ll (no SLLAO, or unspecified source) the same as DAD —
+     * send to all-nodes multicast since we have no unicast link-layer destination. */
+    if (nd_addr_is_unspecified(src) || !src_ll) {
         static const nd_lladdr_t allnodes_ll = { .u8 = { 0x33, 0x33, [5] = 1 } };
         static const nd_addr_t allnodes = { .u8 = { 0xff, 0x02, [15] = 1 } };
         nd_iface_send_na(session->rule->proxy->iface, &allnodes, &allnodes_ll, //

[totoCZ]

forked into https://github.com/totoCZ/ndppd

│ 1 │ Medium │ iface.c:100 │ §7.1.1 MUST │ NS target multicast not rejected │
│ 2 │ Medium │ iface.c:85 │ §4.6 MUST │ Zero-length option doesn't discard packet │
│ 3 │ Low │ iface.c:85 │ §4.6 │ Only first option iterated │
│ 4 │ High │ session.c:60 │ §7.3.3 │ STALE → immediate OVERRIDE NA before fresh NUD │
│ 5 │ Medium │ session.c:94 │ §7.2.5 │ Gratuitous NA ignored while VALID │
│ 6 │ Critical (BSD) │ iface.c:285 │ — │ BPF msg pointer always at buf base │
│ 7 │ Critical (BSD) │ iface.c:291 │ — │ BPF plen check uses total read length │
│ 8 │ High │ session.c:230 │ — │ Div-by-zero if retrans-limit=0 │
│ 9 │ Low │ session.c:231 │ — │ Exponential backoff int overflow │