diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index 1950f1008..fb8450d46 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -1238,8 +1238,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo return err } // The relay port is derived from the id, only known after Save, and only a - // local row owns one: checkPortConflictTx ran neither check with ignoreId==0. + // local row owns one: checkPortConflictTx ran no relay check with ignoreId==0. if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG { + if self := amneziawgnetSocksSelfConflict(inbound, inbound.Id); self != "" { + return common.NewError(self) + } conflict, cErr := checkAmneziawgnetSocksRelayCollision(tx, inbound.Id) if cErr != nil { return cErr @@ -1254,6 +1257,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo if conflict != nil { return common.NewError(conflict.String()) } + // The clients' forward specs were validated while this row had no id, + // so the ports it now derives were never in the guard's context. + if aErr := s.checkAmneziaWGForwardedPorts(tx, inbound.Settings); aErr != nil { + return aErr + } } // Emails seeded here (import's ClientStats, e.g. the controller's forced // Enable=true on every imported stat row) are authoritative for this call diff --git a/internal/web/service/inbound_amneziawg.go b/internal/web/service/inbound_amneziawg.go index d5844c461..7e00d414d 100644 --- a/internal/web/service/inbound_amneziawg.go +++ b/internal/web/service/inbound_amneziawg.go @@ -278,8 +278,8 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound, oldS } for i := range parsed.Clients { c := &parsed.Clients[i] - if hit := s.checkForwardedPortsConflict(portCtx, c.ForwardedPorts); hit != "" { - return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit) + if err := s.amneziaWGForwardedPortsConflict(portCtx, c); err != nil { + return err } if err := amneziawg.ValidateConfigValue("email", c.Email); err != nil { return fmt.Errorf("amneziawg: %w", err) @@ -313,21 +313,15 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound, oldS return nil } -// portConflictContext caches the state checkForwardedPortsConflict needs — -// the panel's own port and this host's enabled inbound ports — so validating -// N clients in one save (normalizeAmneziaWGSettings, or a bulk client add) -// costs one query total instead of N. Load it once with -// loadPortConflictContext and pass it to every checkForwardedPortsConflict -// call in that batch. +// portConflictContext caches what checkForwardedPortsConflict needs — the panel's +// own port and this host's enabled rows — so one save costs one query, not N. type portConflictContext struct { webPort int inbounds []*model.Inbound } -// loadPortConflictContext loads the panel's own port and every enabled -// inbound hosted on THIS panel (node_id IS NULL) — an inbound hosted on a -// different node listens on that node's own host, never this one, so it can -// never collide with a DNAT rule this process installs. +// loadPortConflictContext loads the panel's own port and every enabled inbound +// hosted on THIS panel: a node-hosted one listens on that node's host, not here. func (s *InboundService) loadPortConflictContext(db *gorm.DB) (portConflictContext, error) { var ctx portConflictContext if webPort, err := (&SettingService{}).GetPort(); err == nil { @@ -339,15 +333,37 @@ func (s *InboundService) loadPortConflictContext(db *gorm.DB) (portConflictConte return ctx, err } -// checkForwardedPortsConflict reports whether a client's ForwardedPorts spec -// exceeds the cap, covers the panel's own web port, one of this host's own -// enabled inbound listen ports, or an AmneziaWG inbound's own phantom SOCKS5 -// relay port (SOCKSPortForInbound -- never a real inbounds row, so the loop -// below can't see it any other way). A collision on the SOCKS5 port would -// let a port-forward listener race Xray's own relay for the bind and, if it -// wins, take down that inbound's entire relay rather than just one forward. -// Returns a human-readable description of the first collision found, or "" -// when there is none. +// amneziaWGForwardedPortsConflict renders one client's ForwardedPorts collision, +// or nil: the single copy both the pre-Save pass and the post-Save re-run use. +func (s *InboundService) amneziaWGForwardedPortsConflict(ctx portConflictContext, c *model.Client) error { + hit := s.checkForwardedPortsConflict(ctx, c.ForwardedPorts) + if hit == "" { + return nil + } + return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit) +} + +// checkAmneziaWGForwardedPorts re-runs the guard over one row's stored clients: +// on create it ran before Save, when the row's own ports were not in the context. +func (s *InboundService) checkAmneziaWGForwardedPorts(db *gorm.DB, settings string) error { + var parsed amneziawg.InboundSettings + if err := json.Unmarshal([]byte(settings), &parsed); err != nil { + return nil + } + ctx, err := s.loadPortConflictContext(db) + if err != nil { + return err + } + for i := range parsed.Clients { + if err := s.amneziaWGForwardedPortsConflict(ctx, &parsed.Clients[i]); err != nil { + return err + } + } + return nil +} + +// checkForwardedPortsConflict names the panel, inbound or AmneziaWG relay port a +// client's ForwardedPorts spec would collide with: a lost bind race kills the relay. func (s *InboundService) checkForwardedPortsConflict(ctx portConflictContext, forwardedPorts string) string { if forwardedPorts == "" { return "" diff --git a/internal/web/service/inbound_amneziawg_relay_window_test.go b/internal/web/service/inbound_amneziawg_relay_window_test.go index 08f4de9c6..d8b66ebe2 100644 --- a/internal/web/service/inbound_amneziawg_relay_window_test.go +++ b/internal/web/service/inbound_amneziawg_relay_window_test.go @@ -24,6 +24,14 @@ func awgRelayWindowSettings(t *testing.T, tag string) string { clientPub + `","allowedIPs":["10.8.1.2/32"]}]}` } +// awgRelayWindowSettingsWithForward is awgRelayWindowSettings with one client's +// forwardedPorts set, the field the create-time guard validates. +func awgRelayWindowSettingsWithForward(t *testing.T, tag, forwardedPorts string) string { + t.Helper() + settings := awgRelayWindowSettings(t, tag) + return strings.Replace(settings, `"enable":true`, `"enable":true,"forwardedPorts":"`+forwardedPorts+`"`, 1) +} + // pushInboundIDSequence makes the next inbounds insert land on nextID, standing // in for a long-lived database whose AUTOINCREMENT counter has climbed there. func pushInboundIDSequence(t *testing.T, nextID int) { @@ -168,6 +176,80 @@ func TestCheckPortConflict_DisabledAmneziawgStillOwnsItsRelaySlot(t *testing.T) } } +// The forwarded-ports guard runs before Save, when the row has no id yet, so a +// client's spec never saw the relay port the row itself derives. +func TestAddInbound_AmneziawgRefusesAClientForwardingItsOwnRelayPort(t *testing.T) { + setupConflictDB(t) + + placeholder := addAmneziaWGInbound(t, "awg-placeholder", 51820, true) + ownPort := amneziawgnet.SOCKSPortForInbound(placeholder.Id + 1) + + _, _, err := (&InboundService{}).AddInbound(&model.Inbound{ + Tag: "awg-forward", + Enable: true, + Listen: "0.0.0.0", + Port: 51821, + Protocol: model.AmneziaWG, + Settings: awgRelayWindowSettingsWithForward(t, "awg-forward", fmt.Sprintf("%d", ownPort)), + }) + if err == nil { + t.Fatalf("inbound #%d derives relay port %d and its own client forwards that port; the create must be refused", + placeholder.Id+1, ownPort) + } + if !strings.Contains(err.Error(), "forwardedPorts") { + t.Fatalf("the refusal must come from the forwarded-ports guard, got %v", err) + } +} + +// The row's own WireGuard port can be the relay port its own id derives, and +// every relay check excludes that id, so nothing else compares the two. +func TestAddInbound_AmneziawgRefusesItsOwnRelayPort(t *testing.T) { + setupConflictDB(t) + + // Read the sequence instead of assuming id 1: the victim's own derived port + // has to be known before it is created. + placeholder := addAmneziaWGInbound(t, "awg-placeholder", 51820, true) + selfPort := amneziawgnet.SOCKSPortForInbound(placeholder.Id + 1) + + _, _, err := (&InboundService{}).AddInbound(&model.Inbound{ + Tag: "awg-self", + Enable: true, + Listen: "0.0.0.0", + Port: selfPort, + Protocol: model.AmneziaWG, + Settings: awgRelayWindowSettings(t, "awg-self"), + }) + if err == nil { + t.Fatalf("WireGuard port %d is inbound #%d's own relay port; the create must be refused", + selfPort, placeholder.Id+1) + } + if !strings.Contains(err.Error(), "relay port") { + t.Fatalf("the refusal must say the port is an automatic relay one, got %v", err) + } +} + +// The edit path knows the id the relay port comes from, so it has to refuse the +// same self-collision -- the reverse check skips the row it computes for. +func TestUpdateInbound_AmneziawgRefusesItsOwnRelayPort(t *testing.T) { + setupConflictDB(t) + created := addAmneziaWGInbound(t, "awg-self-edit", 51820, true) + + edit := *created + edit.Port = amneziawgnet.SOCKSPortForInbound(created.Id) + if edit.Port == created.Port { + t.Fatalf("fixture: inbound #%d already listens on its derived relay port", created.Id) + } + + _, _, err := (&InboundService{}).UpdateInbound(&edit) + if err == nil { + t.Fatalf("WireGuard port %d is inbound #%d's own relay port; the save must be refused", + edit.Port, created.Id) + } + if !strings.Contains(err.Error(), "relay port") { + t.Fatalf("the refusal must say the port is an automatic relay one, got %v", err) + } +} + // A row adopted from a node keeps the protocol it arrived with and its central // id (inbound_node.go:737), but gets no relay -- so its slot can never be taken. func TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot(t *testing.T) { @@ -197,4 +279,16 @@ func TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot(t *testing.T) { t.Fatalf("id %d is node-assigned and binds no relay, so it cannot collide; got %q", collidingID, got.String()) } + + // The same rule covers the row's own port: with no relay on this host, its + // WireGuard port may legitimately BE the port its id would derive. + adopted.Port = amneziawgnet.SOCKSPortForInbound(collidingID) + got, err = (&InboundService{}).checkPortConflict(adopted, collidingID) + if err != nil { + t.Fatalf("checkPortConflict: %v", err) + } + if got != nil { + t.Fatalf("id %d is node-assigned and binds no relay, so its own port is not a conflict; got %q", + collidingID, got.String()) + } } diff --git a/internal/web/service/port_conflict.go b/internal/web/service/port_conflict.go index 8e70af55c..c1377e1f0 100644 --- a/internal/web/service/port_conflict.go +++ b/internal/web/service/port_conflict.go @@ -223,6 +223,9 @@ func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*po // The reverse direction, only meaningful once the id is known -- AddInbound // runs it after Save. Only a local row owns a relay slot (#6537 review). if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG && ignoreId > 0 { + if self := amneziawgnetSocksSelfConflict(inbound, ignoreId); self != "" { + return nil, common.NewError(self) + } conflict, err := checkAmneziawgnetSocksRelayCollision(db, ignoreId) if err != nil { return nil, err @@ -330,6 +333,20 @@ func checkAmneziawgnetSocksRelayCollision(db *gorm.DB, id int) (*portConflictDet return nil, nil } +// amneziawgnetSocksSelfConflict: a row's own WireGuard port vs the relay port its +// own id derives -- all three checks below exclude that id, so nothing else does. +func amneziawgnetSocksSelfConflict(inbound *model.Inbound, id int) string { + if id <= 0 || inbound.NodeID != nil || !listenOverlaps("127.0.0.1", inbound.Listen) { + return "" + } + relayPort := amneziawgnet.SOCKSPortForInbound(id) + if inbound.Port != relayPort { + return "" + } + return fmt.Sprintf("WireGuard port %d is inbound #%d's own SOCKS5 relay port on 127.0.0.1; choose a different WireGuard port", + relayPort, id) +} + // checkAmneziawgnetSocksReverseConflict mirrors checkAmneziawgnetSocksConflict: // does id's own derived relay port collide with some other inbound's port. func checkAmneziawgnetSocksReverseConflict(db *gorm.DB, id int) (*portConflictDetail, error) {