fix(amneziawg): resolve 4 Low findings from the automated PR review

- manager.go: serverAddress assumed subnetIp always ends in ".0"; a
  base like "10.8.1.5" was used verbatim as the server's own address,
  eventually colliding with peer allocation (which starts at .2
  upward). Now derives the first host of the actual subnetIp/subnetCidr
  network via netip, matching serverAddressV6's own approach. A /32
  base (no host bits at all) is still used as-is. (Finding 12, partial
  -- the /16 pool-widening half of this finding only exists on the
  upstream-pr/amneziawg branch's merged client_wireguard.go, not here;
  handled separately on that branch.)

- manager.go: ensureLocked carried the previous per-peer traffic
  counters (`last`) forward even through a full restart, but
  awg-quick down+up resets the kernel's own counters to zero -- the
  next CollectTraffic computed a large negative delta (clamped to 0),
  silently discarding real traffic. Extracted the decision into
  nextTrafficBaseline: only a reload (syncconf) preserves the
  baseline. (Finding 13)

- portfwd.go: exported ForwardedPortsInclude; inbound_amneziawg.go's
  new checkForwardedPortsConflict uses it to reject, at save time, a
  client's forwardedPorts that would DNAT the panel's own port or
  another enabled inbound's port to the tunnel client --
  portForwardLines has no destination restriction, so this collision
  was previously silent. Wired into both the single-client update path
  and the add-client path (client_inbound_apply.go), plus
  normalizeAmneziaWGSettings for the whole-inbound save path. (Finding 14)

- inbound.go: InboundOption.AwgServer sent the whole ServerSettings
  struct including PrivateKey to GetInboundOptions callers -- a
  shared, admin-wide dropdown-filling endpoint the frontend's own
  AwgServerOptionSchema never reads that field from. Redacted it
  before assigning. (Finding 11)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Kuzz007
2026-07-26 11:16:39 +03:00
parent 71dc453970
commit df6d2f7652
8 changed files with 250 additions and 11 deletions
@@ -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)
+9 -3
View File
@@ -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
+39
View File
@@ -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
}
@@ -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)
}
}