From dd056e006cac6adf790cc9d4371d4eaa815cb13c Mon Sep 17 00:00:00 2001 From: Alex Melan <59925151+AlexMelanFromRingo@users.noreply.github.com> Date: Tue, 12 May 2026 20:42:57 +0000 Subject: [PATCH] fix: avoid panics on edge-case input across modules (#1343) ## Summary - ipv6rwc: validate IPv6 packet length before reading the version nibble in writePC - config: guard the BOM check against configs shorter than two bytes - admin: replace unchecked net.Error type assertion with errors.As; tolerate empty unix socket paths - multicast: log and continue on ReadFrom errors instead of panicking; use checked type assertion on UDPAddr - mobile: reject negative length in SendBuffer; nil-check AddrForKey in GetPeersJSON and SummaryForConfig - admin/get{tree,paths,sessions}: skip entries when AddrForKey returns nil instead of dereferencing - core/nodeinfo: validate the requested public key length in nodeInfoAdminHandler, matching the other proto handlers - add regression tests for the panic paths ## Why A handful of error paths and platform-API edge cases reach fixed-size indexing or unchecked type assertions before any length validation. Most are reachable only locally (an empty config piped to -useconf, a 0-byte packet from the mobile bindings, an admin DialTimeout error that doesn't satisfy net.Error on some platforms), but they crash the daemon hard. Have them return errors or skip the entry instead. ## Testing - go test ./... - go vet ./... --- contrib/mobile/mobile.go | 16 +++++--- contrib/mobile/mobile_test.go | 26 +++++++++++++ src/admin/admin.go | 15 ++++++-- src/admin/getpaths.go | 3 ++ src/admin/getsessions.go | 3 ++ src/admin/gettree.go | 3 ++ src/config/config.go | 4 +- src/config/config_test.go | 24 ++++++++++++ src/core/nodeinfo.go | 4 ++ src/ipv6rwc/ipv6rwc.go | 3 ++ src/ipv6rwc/ipv6rwc_test.go | 70 +++++++++++++++++++++++++++++++++++ src/multicast/multicast.go | 8 +++- 12 files changed, 166 insertions(+), 13 deletions(-) create mode 100644 src/ipv6rwc/ipv6rwc_test.go diff --git a/contrib/mobile/mobile.go b/contrib/mobile/mobile.go index 72ea7d68..c7213707 100644 --- a/contrib/mobile/mobile.go +++ b/contrib/mobile/mobile.go @@ -138,7 +138,7 @@ func (m *Yggdrasil) SendBuffer(p []byte, length int) error { if m.iprwc == nil { return nil } - if len(p) < length { + if length < 0 || len(p) < length { return nil } _, _ = m.iprwc.Write(p[:length]) @@ -231,8 +231,7 @@ func (m *Yggdrasil) GetPeersJSON() (result string) { }{} for _, v := range m.core.GetPeers() { var ip string - if v.Key != nil { - a := address.AddrForKey(v.Key) + if a := address.AddrForKey(v.Key); a != nil { ip = net.IP(a[:]).String() } peers = append(peers, struct { @@ -286,11 +285,18 @@ func SummaryForConfig(b []byte) *ConfigSummary { if err := cfg.UnmarshalHJSON(b); err != nil { return nil } + if len(cfg.PrivateKey) != ed25519.PrivateKeySize { + return nil + } pub := ed25519.PrivateKey(cfg.PrivateKey).Public().(ed25519.PublicKey) hpub := hex.EncodeToString(pub) - addr := net.IP(address.AddrForKey(pub)[:]) + addrPtr, snetPtr := address.AddrForKey(pub), address.SubnetForKey(pub) + if addrPtr == nil || snetPtr == nil { + return nil + } + addr := net.IP(addrPtr[:]) snet := net.IPNet{ - IP: append(address.SubnetForKey(pub)[:], 0, 0, 0, 0, 0, 0, 0, 0), + IP: append(snetPtr[:], 0, 0, 0, 0, 0, 0, 0, 0), Mask: net.CIDRMask(64, 128), } return &ConfigSummary{ diff --git a/contrib/mobile/mobile_test.go b/contrib/mobile/mobile_test.go index 74689294..856f890d 100644 --- a/contrib/mobile/mobile_test.go +++ b/contrib/mobile/mobile_test.go @@ -26,3 +26,29 @@ func TestStartYggdrasil(t *testing.T) { t.Fatalf("Failed to stop Yggdrasil: %s", err) } } + +// SendBuffer previously panicked when the caller passed a negative +// length (p[:length] out of range) and Send/SendBuffer with empty +// payload reached writePC, which also panicked. +func TestSendBufferRejectsBadLength(t *testing.T) { + logger := log.New(os.Stdout, "", 0) + ygg := &Yggdrasil{logger: logger} + if err := ygg.StartAutoconfigure(); err != nil { + t.Fatalf("Failed to start Yggdrasil: %s", err) + } + defer func() { _ = ygg.Stop() }() + defer func() { + if r := recover(); r != nil { + t.Fatalf("SendBuffer must not panic on bad length, got: %v", r) + } + }() + if err := ygg.SendBuffer([]byte{1, 2, 3, 4}, -1); err != nil { + t.Fatalf("SendBuffer returned unexpected error: %s", err) + } + if err := ygg.SendBuffer(nil, 0); err != nil { + t.Fatalf("SendBuffer returned unexpected error: %s", err) + } + if err := ygg.Send(nil); err != nil { + t.Fatalf("Send returned unexpected error: %s", err) + } +} diff --git a/src/admin/admin.go b/src/admin/admin.go index 305cac30..b7ad15ef 100644 --- a/src/admin/admin.go +++ b/src/admin/admin.go @@ -91,7 +91,15 @@ func New(c *core.Core, log core.Logger, opts ...SetupOption) (*AdminSocket, erro case "unix": if _, err := os.Stat(u.Path); err == nil { a.log.Debugln("Admin socket", u.Path, "already exists, trying to clean up") - if _, err := net.DialTimeout("unix", u.Path, time.Second*2); err == nil || err.(net.Error).Timeout() { + _, dialErr := net.DialTimeout("unix", u.Path, time.Second*2) + inUse := dialErr == nil + if !inUse { + var netErr net.Error + if errors.As(dialErr, &netErr) && netErr.Timeout() { + inUse = true + } + } + if inUse { a.log.Errorln("Admin socket", u.Path, "already exists and is in use by another process") os.Exit(1) } else { @@ -105,9 +113,8 @@ func New(c *core.Core, log core.Logger, opts ...SetupOption) (*AdminSocket, erro } a.listener, err = net.Listen("unix", u.Path) if err == nil { - switch u.Path[:1] { - case "@": // maybe abstract namespace - default: + abstract := u.Path != "" && u.Path[0] == '@' + if !abstract { if err := os.Chmod(u.Path, 0660); err != nil { a.log.Warnln("WARNING:", u.Path, "may have unsafe permissions!") } diff --git a/src/admin/getpaths.go b/src/admin/getpaths.go index 250f4c6a..08b2bc5d 100644 --- a/src/admin/getpaths.go +++ b/src/admin/getpaths.go @@ -28,6 +28,9 @@ func (a *AdminSocket) getPathsHandler(_ *GetPathsRequest, res *GetPathsResponse) res.Paths = make([]PathEntry, 0, len(paths)) for _, p := range paths { addr := address.AddrForKey(p.Key) + if addr == nil { + continue + } res.Paths = append(res.Paths, PathEntry{ IPAddress: net.IP(addr[:]).String(), PublicKey: hex.EncodeToString(p.Key), diff --git a/src/admin/getsessions.go b/src/admin/getsessions.go index 2d76a35b..4a917011 100644 --- a/src/admin/getsessions.go +++ b/src/admin/getsessions.go @@ -28,6 +28,9 @@ func (a *AdminSocket) getSessionsHandler(_ *GetSessionsRequest, res *GetSessions res.Sessions = make([]SessionEntry, 0, len(sessions)) for _, s := range sessions { addr := address.AddrForKey(s.Key) + if addr == nil { + continue + } res.Sessions = append(res.Sessions, SessionEntry{ IPAddress: net.IP(addr[:]).String(), PublicKey: hex.EncodeToString(s.Key[:]), diff --git a/src/admin/gettree.go b/src/admin/gettree.go index 993827d9..00212dcd 100644 --- a/src/admin/gettree.go +++ b/src/admin/gettree.go @@ -27,6 +27,9 @@ func (a *AdminSocket) getTreeHandler(_ *GetTreeRequest, res *GetTreeResponse) er res.Tree = make([]TreeEntry, 0, len(tree)) for _, d := range tree { addr := address.AddrForKey(d.Key) + if addr == nil { + continue + } res.Tree = append(res.Tree, TreeEntry{ IPAddress: net.IP(addr[:]).String(), PublicKey: hex.EncodeToString(d.Key[:]), diff --git a/src/config/config.go b/src/config/config.go index 5dd7b3d4..1d01ed58 100644 --- a/src/config/config.go +++ b/src/config/config.go @@ -99,8 +99,8 @@ func (cfg *NodeConfig) ReadFrom(r io.Reader) (int64, error) { // throwing everywhere when it's converting things into UTF-16 for the hell // of it - remove it and decode back down into UTF-8. This is necessary // because hjson doesn't know what to do with UTF-16 and will panic - if bytes.Equal(conf[0:2], []byte{0xFF, 0xFE}) || - bytes.Equal(conf[0:2], []byte{0xFE, 0xFF}) { + if len(conf) >= 2 && (bytes.Equal(conf[0:2], []byte{0xFF, 0xFE}) || + bytes.Equal(conf[0:2], []byte{0xFE, 0xFF})) { utf := unicode.UTF16(unicode.BigEndian, unicode.UseBOM) decoder := utf.NewDecoder() conf, err = decoder.Bytes(conf) diff --git a/src/config/config_test.go b/src/config/config_test.go index 6b74b50f..c8e311f7 100644 --- a/src/config/config_test.go +++ b/src/config/config_test.go @@ -1,9 +1,33 @@ package config import ( + "bytes" "testing" ) +// ReadFrom previously sliced conf[0:2] for the BOM check without +// guarding the length, so empty or single-byte configs piped via +// -useconf panicked with index out of range. +func TestConfigReadFromEmpty(t *testing.T) { + for _, tc := range []struct { + name string + body []byte + }{ + {name: "empty", body: nil}, + {name: "single byte", body: []byte{'{'}}, + } { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("ReadFrom must not panic on short input, got: %v", r) + } + }() + var cfg NodeConfig + _, _ = cfg.ReadFrom(bytes.NewReader(tc.body)) + }) + } +} + func TestConfig_Keys(t *testing.T) { /* var nodeConfig NodeConfig diff --git a/src/core/nodeinfo.go b/src/core/nodeinfo.go index 698aac7f..3e026e1d 100644 --- a/src/core/nodeinfo.go +++ b/src/core/nodeinfo.go @@ -1,6 +1,7 @@ package core import ( + "crypto/ed25519" "encoding/hex" "encoding/json" "errors" @@ -151,6 +152,9 @@ func (m *nodeinfo) nodeInfoAdminHandler(in json.RawMessage) (interface{}, error) if kbs, err = hex.DecodeString(req.Key); err != nil { return nil, fmt.Errorf("failed to decode public key: %w", err) } + if len(kbs) != ed25519.PublicKeySize { + return nil, fmt.Errorf("invalid public key length") + } copy(key[:], kbs) ch := make(chan []byte, 1) m.sendReq(nil, key, func(info json.RawMessage) { diff --git a/src/ipv6rwc/ipv6rwc.go b/src/ipv6rwc/ipv6rwc.go index 59f4f022..c1ae2eae 100644 --- a/src/ipv6rwc/ipv6rwc.go +++ b/src/ipv6rwc/ipv6rwc.go @@ -281,6 +281,9 @@ func (k *keyStore) readPC(p []byte) (int, error) { } func (k *keyStore) writePC(bs []byte) (int, error) { + if len(bs) == 0 { + return 0, errors.New("empty packet") + } if bs[0]&0xf0 != 0x60 { return 0, errors.New("not an IPv6 packet") // not IPv6 } diff --git a/src/ipv6rwc/ipv6rwc_test.go b/src/ipv6rwc/ipv6rwc_test.go new file mode 100644 index 00000000..75d72900 --- /dev/null +++ b/src/ipv6rwc/ipv6rwc_test.go @@ -0,0 +1,70 @@ +package ipv6rwc + +import ( + "testing" +) + +// writePC indexed bs[0] before checking len(bs), so an empty buffer +// reaching it (via the mobile Send/SendBuffer API or a zero-byte TUN +// read) panicked with index out of range. Reject empty input cleanly. +func TestWritePCRejectsEmptyPacket(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("writePC must not panic on empty input, got: %v", r) + } + }() + var k keyStore + if _, err := k.writePC(nil); err == nil { + t.Fatalf("expected an error for nil input, got nil") + } + if _, err := k.writePC([]byte{}); err == nil { + t.Fatalf("expected an error for zero-length input, got nil") + } +} + +// Buffers shorter than the minimum IPv6 header (40 bytes) but non-empty +// must still be rejected without panicking. +func TestWritePCRejectsTruncatedPacket(t *testing.T) { + var k keyStore + for _, tc := range []struct { + name string + buf []byte + }{ + {name: "single byte ipv6 marker", buf: []byte{0x60}}, + {name: "ten bytes ipv6 marker", buf: append([]byte{0x60}, make([]byte, 9)...)}, + {name: "thirty-nine bytes ipv6 marker", buf: append([]byte{0x60}, make([]byte, 38)...)}, + } { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("writePC must not panic on truncated input, got: %v", r) + } + }() + if _, err := k.writePC(tc.buf); err == nil { + t.Fatalf("expected an error for truncated input, got nil") + } + }) + } +} + +// Non-IPv6 packets (e.g. IPv4) must report "not an IPv6 packet" rather +// than the length-based error, regardless of whether they meet the IPv6 +// 40-byte minimum. +func TestWritePCReportsIPv4AsNonIPv6(t *testing.T) { + var k keyStore + for _, tc := range []struct { + name string + buf []byte + }{ + {name: "ipv4 short", buf: []byte{0x45, 0, 0, 20}}, + {name: "ipv4 padded to 40", buf: append([]byte{0x45, 0, 0, 60}, make([]byte, 36)...)}, + {name: "ipv4 padded to 64", buf: append([]byte{0x45, 0, 0, 60}, make([]byte, 60)...)}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := k.writePC(tc.buf) + if err == nil || err.Error() != "not an IPv6 packet" { + t.Fatalf("expected \"not an IPv6 packet\" error, got: %v", err) + } + }) + } +} diff --git a/src/multicast/multicast.go b/src/multicast/multicast.go index 49d4af23..8c169108 100644 --- a/src/multicast/multicast.go +++ b/src/multicast/multicast.go @@ -392,7 +392,8 @@ func (m *Multicast) listen() { if !m.IsStarted() { return } - panic(err) + m.log.Warnln("Multicast listener read error:", err) + continue } if rcm != nil { // Windows can't set the flag needed to return a non-nil value here @@ -417,7 +418,10 @@ func (m *Multicast) listen() { case adv.PublicKey.Equal(m.core.PublicKey()): continue } - from := fromAddr.(*net.UDPAddr) + from, ok := fromAddr.(*net.UDPAddr) + if !ok { + continue + } from.Port = int(adv.Port) var interfaces map[string]*interfaceInfo phony.Block(m, func() {