diff --git a/internal/amneziawg/manager.go b/internal/amneziawg/manager.go index 99b56917e..c2a55a00c 100644 --- a/internal/amneziawg/manager.go +++ b/internal/amneziawg/manager.go @@ -96,16 +96,28 @@ func interfaceNameForID(id int) string { } // serverAddress returns the server's own tunnel address for a subnet base, -// e.g. "10.8.1.1/24" for base "10.8.1.0". The server holds the first usable -// host; a base that isn't a bare network address is used as-is. +// e.g. "10.8.1.1/24" for base "10.8.1.0" or "10.8.1.5". The server always +// holds the first usable host of the network subnetIP/cidr actually +// describes -- derived via netip rather than assuming subnetIP already ends +// in ".0", so a subnetIP that isn't a bare network address (a typo, or a +// manually edited value) can never collide with peer addresses, which are +// allocated starting from the network's second host upward (see +// allocateWireguardAddress). Falls back to the previous literal behavior +// only if subnetIP/cidr doesn't parse as an IPv4 network at all -- normal +// saves never reach that path since ValidateSubnetIPv4 already rejects it. func serverAddress(subnetIP string, cidr int) string { if cidr <= 0 { cidr = 24 } - if strings.HasSuffix(subnetIP, ".0") { - return strings.TrimSuffix(subnetIP, "0") + "1/" + strconv.Itoa(cidr) + // A /32 has no host bits at all -- "first usable host" is meaningless, + // and Next() would step outside the block entirely -- so a single-host + // base is used exactly as given, same as before this fix. + prefix, err := netip.ParsePrefix(fmt.Sprintf("%s/%d", subnetIP, cidr)) + if err != nil || !prefix.Addr().Is4() || cidr >= 32 { + return fmt.Sprintf("%s/%d", subnetIP, cidr) } - return fmt.Sprintf("%s/%d", subnetIP, cidr) + host := prefix.Masked().Addr().Next() + return fmt.Sprintf("%s/%d", host, cidr) } // serverAddressV6 returns the server's own IPv6 tunnel address for a subnet @@ -307,12 +319,26 @@ func (m *Manager) ensureLocked(inst Instance) error { last := map[string]peerCounters{} if exists { - last = cur.last + last = nextTrafficBaseline(action, cur.last) } m.ifaces[inst.Id] = &managed{inst: inst, structuralFP: structFP, hostRulesFP: hostRulesFP, peersFP: peersFP, last: last} return nil } +// nextTrafficBaseline decides what per-peer traffic counters ensureLocked +// should carry into the next managed entry. Only a reload (awg syncconf) +// preserves the kernel's own per-peer transfer counters; a full down+up +// zeroes them. Carrying the old baseline forward after a restart would make +// the next CollectTraffic compute a large negative delta (clamped to 0 by +// the caller), silently discarding whatever the peers transferred since the +// previous poll instead of just resuming the count from zero. +func nextTrafficBaseline(action ensureAction, prev map[string]peerCounters) map[string]peerCounters { + if action == ensureReload { + return prev + } + return map[string]peerCounters{} +} + // Remove tears down and forgets the interface for an inbound id. func (m *Manager) Remove(id int) { m.mu.Lock() diff --git a/internal/amneziawg/manager_test.go b/internal/amneziawg/manager_test.go index f5c223cf9..816a354d2 100644 --- a/internal/amneziawg/manager_test.go +++ b/internal/amneziawg/manager_test.go @@ -106,8 +106,10 @@ func TestServerAddress(t *testing.T) { want string }{ {"10.8.1.0", 24, "10.8.1.1/24"}, - {"10.8.1.0", 0, "10.8.1.1/24"}, // cidr <= 0 defaults to /24 - {"192.168.5.10", 32, "192.168.5.10/32"}, + {"10.8.1.0", 0, "10.8.1.1/24"}, // cidr <= 0 defaults to /24 + {"10.8.1.5", 24, "10.8.1.1/24"}, // non-network base: must not collide with peer allocation starting at .2 + {"10.8.1.254", 24, "10.8.1.1/24"}, + {"192.168.5.10", 32, "192.168.5.10/32"}, // /32 has no host bits: used as-is } for _, c := range cases { if got := serverAddress(c.subnet, c.cidr); got != c.want { @@ -225,6 +227,17 @@ func TestEnsureActionFor(t *testing.T) { } } +func TestNextTrafficBaseline(t *testing.T) { + prev := map[string]peerCounters{"pubA": {rx: 100, tx: 200}} + + if got := nextTrafficBaseline(ensureReload, prev); len(got) != 1 || got["pubA"] != prev["pubA"] { + t.Errorf("a reload must preserve the previous baseline (syncconf never resets kernel counters), got %v", got) + } + if got := nextTrafficBaseline(ensureRestart, prev); len(got) != 0 { + t.Errorf("a restart must reset the baseline to empty (awg-quick down+up zeroes kernel counters), got %v", got) + } +} + func TestHostRulesFingerprintCoversForwardedPortsAndPeerIP(t *testing.T) { a := baseInstance() b := baseInstance() diff --git a/internal/amneziawg/portfwd.go b/internal/amneziawg/portfwd.go index fceeec092..be6847112 100644 --- a/internal/amneziawg/portfwd.go +++ b/internal/amneziawg/portfwd.go @@ -90,6 +90,23 @@ func parsePortNumber(s string) (int, bool) { return n, true } +// ForwardedPortsInclude reports whether port is covered by any spec in a raw +// ForwardedPorts string (a single port or an inclusive range). For callers +// outside this package that need to check a spec against something other +// than rendering it into iptables rules -- e.g. save-time validation that a +// client isn't about to hijack the panel's own port or another inbound's +// port (portForwardLines has no -d restriction, so a forwarded port that +// collides with one already in use on the host silently redirects it to the +// tunnel client instead). +func ForwardedPortsInclude(forwardedPorts string, port int) bool { + for _, spec := range parseForwardedPorts(forwardedPorts) { + if port >= spec.start && port <= spec.end { + return true + } + } + return false +} + // portForwardComment returns a short, shell-safe iptables comment tag for one // peer's forwarded-port rules, so PostDown removes exactly what PostUp added // regardless of ordering. Derived from a hash of the peer's email rather than diff --git a/internal/amneziawg/portfwd_test.go b/internal/amneziawg/portfwd_test.go new file mode 100644 index 000000000..3f94c802d --- /dev/null +++ b/internal/amneziawg/portfwd_test.go @@ -0,0 +1,25 @@ +package amneziawg + +import "testing" + +func TestForwardedPortsInclude(t *testing.T) { + cases := []struct { + spec string + port int + want bool + }{ + {"80,443", 80, true}, + {"80,443", 443, true}, + {"80,443", 8080, false}, + {"8000-8100", 8050, true}, + {"8000-8100", 7999, false}, + {"8000-8100", 8101, false}, + {"", 80, false}, + {"not-a-port", 80, false}, + } + for _, c := range cases { + if got := ForwardedPortsInclude(c.spec, c.port); got != c.want { + t.Errorf("ForwardedPortsInclude(%q, %d) = %v, want %v", c.spec, c.port, got, c.want) + } + } +} diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 1093e5ee0..9316ecb92 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -401,6 +401,13 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model return false, common.NewError("empty client ID") } } + if oldInbound.Protocol == model.AmneziaWG { + if hit, err := inboundSvc.checkForwardedPortsConflict(client.ForwardedPorts); err != nil { + return false, err + } else if hit != "" { + return false, common.NewError("amneziawg: forwardedPorts collides with", hit) + } + } } var oldSettings map[string]any @@ -652,6 +659,13 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo clients[0].ForwardedPorts = old.ForwardedPorts } } + if oldInbound.Protocol == model.AmneziaWG { + if hit, err := inboundSvc.checkForwardedPortsConflict(clients[0].ForwardedPorts); err != nil { + return false, err + } else if hit != "" { + return false, common.NewError("amneziawg: forwardedPorts collides with", hit) + } + } var oldSettings map[string]any err = json.Unmarshal([]byte(oldInbound.Settings), &oldSettings) diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index fd1862fb2..8e6d55dc9 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -409,16 +409,22 @@ func inboundWireguardHints(protocol string, settings string) (string, int, strin // inboundAmneziaWGServer returns the AmneziaWG server block for the clients // page's config-download builder, or nil when the inbound isn't AmneziaWG or -// its settings don't parse. +// its settings don't parse. PrivateKey is redacted: GetInboundOptions is a +// shared, admin-wide list used to fill dropdowns, not a place a live tunnel +// secret needs to travel — the frontend's own AwgServerOptionSchema never +// reads it, so nothing is lost by not sending it, and it shouldn't widen the +// blast radius of a log capture, proxy cache, or browser devtools screenshot. func inboundAmneziaWGServer(protocol string, settings string) *amneziawg.ServerSettings { if protocol != string(model.AmneziaWG) || strings.TrimSpace(settings) == "" { return nil } var parsed amneziawg.InboundSettings - if err := json.Unmarshal([]byte(settings), &parsed); err != nil { + if err := json.Unmarshal([]byte(settings), &parsed); err != nil || parsed.Server == nil { return nil } - return parsed.Server + redacted := *parsed.Server + redacted.PrivateKey = "" + return &redacted } // inboundMtprotoDomain returns the inbound-level FakeTLS default domain, used by diff --git a/internal/web/service/inbound_amneziawg.go b/internal/web/service/inbound_amneziawg.go index b773a5784..201a72441 100644 --- a/internal/web/service/inbound_amneziawg.go +++ b/internal/web/service/inbound_amneziawg.go @@ -201,6 +201,14 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound) erro return fmt.Errorf("amneziawg: ipv6ExternalInterface: %w", err) } + for _, c := range parsed.Clients { + if hit, err := s.checkForwardedPortsConflict(c.ForwardedPorts); err != nil { + return err + } else if hit != "" { + return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit) + } + } + bs, err := json.MarshalIndent(parsed, "", " ") if err != nil { return err @@ -208,3 +216,34 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound) erro inbound.Settings = string(bs) return nil } + +// checkForwardedPortsConflict reports whether a client's ForwardedPorts spec +// covers the panel's own web port or any enabled inbound's own listen port. +// portForwardLines has no destination restriction and nothing else checks +// this, so a collision here would silently DNAT traffic meant for the panel +// or another protocol straight to the tunnel client instead. Returns a +// human-readable description of the first collision found, or "" when there +// is none. +func (s *InboundService) checkForwardedPortsConflict(forwardedPorts string) (string, error) { + if forwardedPorts == "" { + return "", nil + } + if webPort, err := (&SettingService{}).GetPort(); err == nil && amneziawg.ForwardedPortsInclude(forwardedPorts, webPort) { + return fmt.Sprintf("the panel's own port (%d)", webPort), nil + } + var inbounds []*model.Inbound + if err := database.GetDB().Model(model.Inbound{}).Where("enable = ?", true).Find(&inbounds).Error; err != nil { + return "", err + } + for _, ib := range inbounds { + if !amneziawg.ForwardedPortsInclude(forwardedPorts, ib.Port) { + continue + } + name := ib.Remark + if name == "" { + name = ib.Tag + } + return fmt.Sprintf("inbound '%s' (#%d, port %d)", name, ib.Id, ib.Port), nil + } + return "", nil +} diff --git a/internal/web/service/inbound_amneziawg_test.go b/internal/web/service/inbound_amneziawg_test.go new file mode 100644 index 000000000..b100fccbb --- /dev/null +++ b/internal/web/service/inbound_amneziawg_test.go @@ -0,0 +1,99 @@ +package service + +import ( + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +func TestCheckForwardedPortsConflict_EmptySpecNoConflict(t *testing.T) { + setupConflictDB(t) + svc := &InboundService{} + hit, err := svc.checkForwardedPortsConflict("") + if err != nil || hit != "" { + t.Fatalf("an empty spec must never conflict; got hit=%q err=%v", hit, err) + } +} + +func TestCheckForwardedPortsConflict_CollidesWithPanelPort(t *testing.T) { + setupConflictDB(t) + svc := &InboundService{} + // getString falls back to defaultValueMap's "webPort": "2053" on a fresh + // DB with no explicit setting row. + hit, err := svc.checkForwardedPortsConflict("2053") + if err != nil { + t.Fatalf("checkForwardedPortsConflict: %v", err) + } + if !strings.Contains(hit, "panel") { + t.Fatalf("expected a collision naming the panel's own port, got %q", hit) + } +} + +func TestCheckForwardedPortsConflict_CollidesWithEnabledInboundPort(t *testing.T) { + setupConflictDB(t) + seedInboundConflict(t, "vless-8080", "0.0.0.0", 8080, model.VLESS, `{"network":"tcp"}`, `{}`) + + svc := &InboundService{} + hit, err := svc.checkForwardedPortsConflict("8000-8100") + if err != nil { + t.Fatalf("checkForwardedPortsConflict: %v", err) + } + if !strings.Contains(hit, "vless-8080") { + t.Fatalf("expected a collision naming the colliding inbound, got %q", hit) + } +} + +func TestCheckForwardedPortsConflict_IgnoresDisabledInboundPort(t *testing.T) { + setupConflictDB(t) + disabled := &model.Inbound{Tag: "vless-8080-off", Enable: false, Listen: "0.0.0.0", Port: 8080, Protocol: model.VLESS, StreamSettings: `{"network":"tcp"}`} + if err := database.GetDB().Create(disabled).Error; err != nil { + t.Fatalf("seed disabled inbound: %v", err) + } + + svc := &InboundService{} + hit, err := svc.checkForwardedPortsConflict("8080") + if err != nil || hit != "" { + t.Fatalf("a disabled inbound's port must not be reserved; got hit=%q err=%v", hit, err) + } +} + +func TestCheckForwardedPortsConflict_NoCollisionWhenPortsDontOverlap(t *testing.T) { + setupConflictDB(t) + seedInboundConflict(t, "vless-8080", "0.0.0.0", 8080, model.VLESS, `{"network":"tcp"}`, `{}`) + + svc := &InboundService{} + hit, err := svc.checkForwardedPortsConflict("9000-9100") + if err != nil || hit != "" { + t.Fatalf("unrelated ports must not conflict; got hit=%q err=%v", hit, err) + } +} + +// inboundAmneziaWGServer is pure (no DB), so it needs neither setupConflictDB +// nor CGO/sqlite -- it can run in any Go environment. +func TestInboundAmneziaWGServer_RedactsPrivateKey(t *testing.T) { + settings := `{"server":{"privateKey":"super-secret","publicKey":"pub","mtu":1420},"clients":[]}` + got := inboundAmneziaWGServer(string(model.AmneziaWG), settings) + if got == nil { + t.Fatal("expected a non-nil server block") + } + if got.PrivateKey != "" { + t.Fatalf("PrivateKey must be redacted, got %q", got.PrivateKey) + } + if got.PublicKey != "pub" || got.MTU != 1420 { + t.Fatalf("non-secret fields must still come through unchanged, got %+v", got) + } +} + +func TestInboundAmneziaWGServer_NonAmneziaWGReturnsNil(t *testing.T) { + if got := inboundAmneziaWGServer(string(model.VLESS), `{"server":{"privateKey":"x"}}`); got != nil { + t.Fatalf("a non-AmneziaWG protocol must return nil, got %+v", got) + } +} + +func TestInboundAmneziaWGServer_MissingServerBlockReturnsNil(t *testing.T) { + if got := inboundAmneziaWGServer(string(model.AmneziaWG), `{"clients":[]}`); got != nil { + t.Fatalf("settings with no server block must return nil, got %+v", got) + } +}