From daf5b16eda21c398eaf7dbccba1f0a234e10bfc1 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 1 Nov 2026 00:06:09 +0100 Subject: [PATCH] fix(wireguard): hold /0 allowedIPs to the overlap guard The tunnel overlap guard treated every /0 allowedIPs entry as claiming no address, while the entry was stored with its host bits and the consumers read them back: the AmneziaWG port forwarder resolves a peer's target from them and the device routes the /0 by longest prefix. An entry such as 10.8.1.2/0 therefore passed next to another client's 10.8.1.2/32, and forwarded ports reached that client's device. A written entry now counts with its whole range, so a /0 collides with every held address of its family. On the stored side only a literal 0.0.0.0/0 or ::/0 default route keeps nobody out, and a /0 with host bits claims that host. Editing a legacy peer that echoes its stored default route still works. --- .../content/docs/en/reference/api/clients.mdx | 8 +- docs/public/openapi.json | 2 +- frontend/public/openapi.json | 2 +- frontend/src/pages/api-docs/endpoints.ts | 2 +- internal/web/service/client_attach_test.go | 62 +++++++++++++ internal/web/service/client_inbound_apply.go | 2 +- internal/web/service/client_wireguard.go | 45 ++++++++-- internal/web/service/client_wireguard_test.go | 87 +++++++++++++++++++ 8 files changed, 196 insertions(+), 14 deletions(-) diff --git a/docs/content/docs/en/reference/api/clients.mdx b/docs/content/docs/en/reference/api/clients.mdx index f745c5249..5255194c7 100644 --- a/docs/content/docs/en/reference/api/clients.mdx +++ b/docs/content/docs/en/reference/api/clients.mdx @@ -609,9 +609,11 @@ _openapi: different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides - with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no - address. Allocation likewise skips every address inside a prefix - another client holds. The same validation runs on POST + with `10.0.0.5/32`; a `/0` entry collides with every address of its + family another client holds, and only a `0.0.0.0/0` or `::/0` default + route that is already stored (legacy peers) keeps nobody out. + Allocation likewise skips every address inside a prefix another client + holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along. diff --git a/docs/public/openapi.json b/docs/public/openapi.json index 86a85aca4..270316e56 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -8846,7 +8846,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `/0` entry collides with every address of its family another client holds, and only a `0.0.0.0/0` or `::/0` default route that is already stored (legacy peers) keeps nobody out. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index 86a85aca4..270316e56 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -8846,7 +8846,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `/0` entry collides with every address of its family another client holds, and only a `0.0.0.0/0` or `::/0` default route that is already stored (legacy peers) keeps nobody out. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { diff --git a/frontend/src/pages/api-docs/endpoints.ts b/frontend/src/pages/api-docs/endpoints.ts index b8cee9666..1d5cf3bc5 100644 --- a/frontend/src/pages/api-docs/endpoints.ts +++ b/frontend/src/pages/api-docs/endpoints.ts @@ -1195,7 +1195,7 @@ export const sections: readonly Section[] = [ summary: 'Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.', description: - 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.', + 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `/0` entry collides with every address of its family another client holds, and only a `0.0.0.0/0` or `::/0` default route that is already stored (legacy peers) keeps nobody out. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.', params: [ { name: 'client', diff --git a/internal/web/service/client_attach_test.go b/internal/web/service/client_attach_test.go index cf53ada50..d1ef9b12e 100644 --- a/internal/web/service/client_attach_test.go +++ b/internal/web/service/client_attach_test.go @@ -1,6 +1,8 @@ package service import ( + "errors" + "strings" "testing" "github.com/mhsanaei/3x-ui/v3/internal/database" @@ -95,3 +97,63 @@ func TestAddressesFitAmneziaWGInbound(t *testing.T) { t.Error("an address genuinely inside the awg inbound's own configured subnet must fit") } } + +// A /0 reported by a node contains every address of the target inbound, and its +// host bits are what the AmneziaWG forwarder reads as the peer's address, so +// Attach refuses it next to another client exactly as it refuses the /32. +func TestAttachRefusesDefaultRouteNextToHeldAddress(t *testing.T) { + attaches := map[string]func(*ClientService, *InboundService, string, int) error{ + "Attach": func(s *ClientService, inboundSvc *InboundService, email string, ibId int) error { + _, err := s.AttachByEmail(inboundSvc, email, []int{ibId}) + return err + }, + "BulkAttach": func(s *ClientService, inboundSvc *InboundService, email string, ibId int) error { + result, _, err := s.BulkAttach(inboundSvc, []string{email}, []int{ibId}) + if err == nil && len(result.Errors) > 0 { + err = errors.New(strings.Join(result.Errors, "; ")) + } + return err + }, + } + for name, attach := range attaches { + for _, protocol := range []model.Protocol{model.WireGuard, model.AmneziaWG} { + t.Run(name+"/"+string(protocol), func(t *testing.T) { + setupConflictDB(t) + node := 1 + seedInboundConflict(t, "own", "0.0.0.0", 51830, protocol, ``, `{"subnetIp":"10.8.1.0","subnetCidr":24,"server":{"subnetIp":"10.8.1.0","subnetCidr":24},"clients":[`+ + `{"email":"local","subId":"s-local","enable":true,"publicKey":"QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=","allowedIPs":["10.8.1.2/32"]}]}`) + seedInboundConflictNode(t, "node", "0.0.0.0", 44301, protocol, ``, `{"clients":[`+ + `{"email":"host-bits","subId":"s-host-bits","enable":true,"publicKey":"QkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkI=","allowedIPs":["10.8.1.2/0"]}]}`, &node) + + s := &ClientService{} + inboundSvc := &InboundService{} + var own model.Inbound + for _, tag := range []string{"node", "own"} { + own = model.Inbound{} + if err := database.GetDB().Where("tag = ?", tag).First(&own).Error; err != nil { + t.Fatalf("read seeded %s row: %v", tag, err) + } + clients, err := inboundSvc.GetClients(&own) + if err != nil { + t.Fatalf("GetClients(%s): %v", tag, err) + } + if err := s.SyncInbound(nil, own.Id, clients); err != nil { + t.Fatalf("SyncInbound(%s): %v", tag, err) + } + } + + err := attach(s, inboundSvc, "host-bits", own.Id) + if err == nil || !strings.Contains(err.Error(), "10.8.1.2/0 overlaps 10.8.1.2/32") { + t.Errorf("attach of host-bits (10.8.1.2/0): err = %v, want an overlap with 10.8.1.2/32", err) + } + got, gErr := s.TunnelAllowedIPsByInbound(inboundSvc, "host-bits", []int{own.Id}) + if gErr != nil { + t.Fatalf("TunnelAllowedIPsByInbound: %v", gErr) + } + if _, held := got[own.Id]; held { + t.Errorf("host-bits landed on the master's inbound with %q", got[own.Id]) + } + }) + } + } +} diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 08b2abdc0..9da44d104 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -833,7 +833,7 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo } peers = append(peers, oldClients[i].AllowedIPs...) } - if entry, taken := wireguardAllowedIPsOverlap(normalized, peers); taken != "" { + if entry, taken := wireguardAllowedIPsOverlap(withoutKeptDefaultRoutes(normalized, old.AllowedIPs), peers); taken != "" { return false, common.NewError("wireguard: allowedIPs entry", entry, "overlaps", taken, "used by another client") } clients[0].AllowedIPs = normalized diff --git a/internal/web/service/client_wireguard.go b/internal/web/service/client_wireguard.go index 0285f30d3..b7188fae2 100644 --- a/internal/web/service/client_wireguard.go +++ b/internal/web/service/client_wireguard.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/netip" + "slices" "strconv" "strings" @@ -188,9 +189,9 @@ func normalizeWireguardAllowedIPs(values []string) ([]string, error) { return out, nil } -// wireguardClaimedPrefix is the masked range an allowedIPs entry claims, as xray -// reads it. A /0 default route claims no tunnel address, as legacy peers carry it. -func wireguardClaimedPrefix(s string) (netip.Prefix, bool) { +// parseWireguardAllowedIP reads an allowedIPs entry unmasked, a bare address as +// a single host. +func parseWireguardAllowedIP(s string) (netip.Prefix, bool) { s = strings.TrimSpace(s) p, err := netip.ParsePrefix(s) if err != nil { @@ -200,14 +201,35 @@ func wireguardClaimedPrefix(s string) (netip.Prefix, bool) { } p = netip.PrefixFrom(a, a.BitLen()) } - if p.Bits() == 0 { + return p, true +} + +// isWireguardDefaultRoute reports whether an allowedIPs entry carries a /0 mask. +func isWireguardDefaultRoute(s string) bool { + p, ok := parseWireguardAllowedIP(s) + return ok && p.Bits() == 0 +} + +// wireguardClaimedPrefix is the range a stored allowedIPs entry keeps other clients +// out of. Only a 0.0.0.0/0 or ::/0 default route claims nothing (legacy peers carry it). +func wireguardClaimedPrefix(s string) (netip.Prefix, bool) { + p, ok := parseWireguardAllowedIP(s) + if !ok { return netip.Prefix{}, false } + if p.Bits() == 0 { + if p.Addr().IsUnspecified() { + return netip.Prefix{}, false + } + // The host bits are stored with it and read back as the peer's tunnel + // address (amneziawg.FirstIPv4 / FirstIPv6), so they are what it claims. + return netip.PrefixFrom(p.Addr(), p.Addr().BitLen()), true + } return p.Masked(), true } -// wireguardAllowedIPsOverlap returns the first entry whose range overlaps a used -// one, and that used entry; xray routes and attributes by containment, not equality. +// wireguardAllowedIPsOverlap returns the first entry whose whole range (a /0 holds its entire +// family) overlaps a used one, and that entry; xray routes by containment, not equality. func wireguardAllowedIPsOverlap(entries, used []string) (entry, taken string) { usedPrefixes := make([]netip.Prefix, len(used)) usedOK := make([]bool, len(used)) @@ -215,10 +237,11 @@ func wireguardAllowedIPsOverlap(entries, used []string) (entry, taken string) { usedPrefixes[i], usedOK[i] = wireguardClaimedPrefix(u) } for _, e := range entries { - ep, ok := wireguardClaimedPrefix(e) + ep, ok := parseWireguardAllowedIP(e) if !ok { continue } + ep = ep.Masked() for i, up := range usedPrefixes { if usedOK[i] && ep.Overlaps(up) { return e, used[i] @@ -228,6 +251,14 @@ func wireguardAllowedIPsOverlap(entries, used []string) (entry, taken string) { return "", "" } +// withoutKeptDefaultRoutes drops the /0 entries stored already holds: an edit that +// echoes a legacy peer's default route claims nothing new and must stay possible. +func withoutKeptDefaultRoutes(entries, stored []string) []string { + return slices.DeleteFunc(slices.Clone(entries), func(e string) bool { + return isWireguardDefaultRoute(e) && slices.Contains(stored, e) + }) +} + // defaultWireguardClients fills in blank WireGuard credentials for newly added // clients: a generated keypair when none was provided, a derived public key when // only a private key was given, and a unique tunnel address allocated from the diff --git a/internal/web/service/client_wireguard_test.go b/internal/web/service/client_wireguard_test.go index 6c723c44d..a437da9ac 100644 --- a/internal/web/service/client_wireguard_test.go +++ b/internal/web/service/client_wireguard_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" wgutil "github.com/mhsanaei/3x-ui/v3/internal/util/wireguard" ) @@ -393,3 +394,89 @@ func TestAllocateWireguardAddressSkipsAddressesInsideUsedPrefixes(t *testing.T) t.Fatalf("got %s, want 10.0.0.4/32: .2 and .3 are both inside the used 10.0.0.2/31", got) } } + +// A /0 that is being written contains every other peer's address, and its host bits +// are read back as the peer's own tunnel address (amneziawg.FirstIPv4). Only a stored +// 0.0.0.0/0 or ::/0 keeps nobody out, as legacy peers carry it. +func TestWireguardSlashZeroIsSubjectToTheOverlapGuard(t *testing.T) { + for _, tt := range []struct { + entry, used string + taken bool + }{ + {"10.8.1.2/0", "10.8.1.2/32", true}, + {"10.8.1.3/0", "10.8.1.2/32", true}, + {"0.0.0.0/0", "10.8.1.2/32", true}, + {"::/0", "fd00::2/128", true}, + {"fd00::2/0", "fd00::2/128", true}, + {"::/0", "10.8.1.2/32", false}, + {"10.8.1.2/32", "10.8.1.2/0", true}, + {"fd00::2/128", "fd00::2/0", true}, + {"10.8.1.2/32", "10.8.1.3/0", false}, + {"10.8.1.2/32", "0.0.0.0/0", false}, + {"0.0.0.0/0", "0.0.0.0/0", false}, + } { + t.Run(tt.entry+" next to "+tt.used, func(t *testing.T) { + if _, taken := wireguardAllowedIPsOverlap([]string{tt.entry}, []string{tt.used}); (taken != "") != tt.taken { + t.Errorf("taken = %q, want a collision: %v", taken, tt.taken) + } + }) + } + + existing := []model.Client{{Email: "victim@wg", AllowedIPs: []string{"10.0.0.2/32"}}} + awgSettings := `{"server":{"subnetIp":"10.0.0.0","subnetCidr":24}}` + for _, entry := range []string{"10.0.0.2/0", "0.0.0.0/0"} { + clients := []model.Client{{Email: "other@wg", AllowedIPs: []string{entry}}} + err := defaultWireguardClients("", existing, clients, []any{map[string]any{}}, nil) + if err == nil || !strings.Contains(err.Error(), entry+" overlaps 10.0.0.2/32") { + t.Errorf("wireguard: %s next to 10.0.0.2/32: err = %v, want an overlap", entry, err) + } + err = defaultAmneziaWGClients(awgSettings, existing, clients, []any{map[string]any{}}, nil) + if err == nil || !strings.Contains(err.Error(), entry+" overlaps 10.0.0.2/32") { + t.Errorf("amneziawg: %s next to 10.0.0.2/32: err = %v, want an overlap", entry, err) + } + } + + got, err := allocateWireguardAddress([]string{"10.0.0.2/0"}, "10.0.0.0/24", true) + if err != nil || got != "10.0.0.3/32" { + t.Errorf("allocated %q (%v) next to a stored 10.0.0.2/0, want 10.0.0.3/32", got, err) + } +} + +// The edit form echoes what the client holds, so a legacy peer's default route has +// to survive a save next to other clients; no edit may bring in a new one. +func TestUpdateInboundClientKeepsButNeverAddsDefaultRoute(t *testing.T) { + setupConflictDB(t) + seedInboundConflict(t, "own-wg", "0.0.0.0", 51830, model.WireGuard, ``, `{"clients":[`+ + `{"email":"legacy","subId":"s-legacy","enable":true,"publicKey":"QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=","allowedIPs":["0.0.0.0/0","::/0"]},`+ + `{"email":"victim","subId":"s-victim","enable":true,"publicKey":"QkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkI=","allowedIPs":["10.0.0.2/32"]},`+ + `{"email":"other","subId":"s-other","enable":true,"publicKey":"Q0NDQ0NDQ0NDQ0NDQ0NDQ0NDQ0NDQ0NDQ0NDQ0NDQ0M=","allowedIPs":["10.0.0.3/32"]}]}`) + s, inboundSvc := &ClientService{}, &InboundService{} + var own model.Inbound + if err := database.GetDB().Where("tag = ?", "own-wg").First(&own).Error; err != nil { + t.Fatalf("read seeded row: %v", err) + } + clients, err := inboundSvc.GetClients(&own) + if err != nil { + t.Fatalf("GetClients: %v", err) + } + if err := s.SyncInbound(nil, own.Id, clients); err != nil { + t.Fatalf("SyncInbound: %v", err) + } + save := func(email, allowedIPs string) error { + data := &model.Inbound{Id: own.Id, Settings: `{"clients":[{"email":"` + email + `","subId":"s-` + email + `","enable":true,"allowedIPs":` + allowedIPs + `}]}`} + _, err := s.UpdateInboundClient(inboundSvc, data, email) + return err + } + + if err := save("legacy", `["0.0.0.0/0","::/0"]`); err != nil { + t.Errorf("saving the legacy peer as it is: %v", err) + } + for _, allowedIPs := range []string{`["0.0.0.0/0"]`, `["10.0.0.2/0"]`, `["10.0.0.3/32","0.0.0.0/0"]`} { + if err := save("other", allowedIPs); err == nil || !strings.Contains(err.Error(), "overlaps 10.0.0.2/32") { + t.Errorf("other saved with %s: err = %v, want an overlap with 10.0.0.2/32", allowedIPs, err) + } + } + if err := save("legacy", `["10.0.0.2/0","::/0"]`); err == nil || !strings.Contains(err.Error(), "10.0.0.2/0 overlaps 10.0.0.2/32") { + t.Errorf("the legacy peer took 10.0.0.2/0 next to 10.0.0.2/32: err = %v", err) + } +}