From 1ab59e763ca1be1f6c5174cf596f556fbcb9417d Mon Sep 17 00:00:00 2001 From: fatedier Date: Thu, 30 Jul 2026 23:30:16 +0800 Subject: [PATCH] server: validate control pool counts (#5459) --- Release.md | 4 +- pkg/config/v1/server.go | 2 +- pkg/config/v1/validation/server.go | 3 ++ pkg/config/v1/validation/server_test.go | 51 ++++++++++++++++++++++++ server/control.go | 24 +++++++++++- server/control_test.go | 52 +++++++++++++++++++++++++ server/service_test.go | 43 ++++++++++++++++++++ 7 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 pkg/config/v1/validation/server_test.go diff --git a/Release.md b/Release.md index 803ef5e4..26deaad0 100644 --- a/Release.md +++ b/Release.md @@ -1,3 +1,3 @@ -## Features +## Fixes -* Ordinary UDP proxy payloads now use a dedicated binary codec when frpc and frps successfully negotiate the capability under wire protocol v2, reducing frame size and encoding/decoding overhead. Wire protocol v1 and SUDP remain unchanged; wire protocol v2 falls back to JSON `UDPPacket` when the peer does not support or did not negotiate the capability. +* Fixed a server panic and remote denial of service caused by a client sending a negative `pool_count`. Negative values are now rejected before work-connection pool resources are allocated. diff --git a/pkg/config/v1/server.go b/pkg/config/v1/server.go index a92aac97..67c1d84b 100644 --- a/pkg/config/v1/server.go +++ b/pkg/config/v1/server.go @@ -167,7 +167,7 @@ type ServerTransportConfig struct { // If negative, keep-alive probes are disabled. TCPKeepAlive int64 `json:"tcpKeepalive,omitempty"` // MaxPoolCount specifies the maximum pool size for each proxy. By default, - // this value is 5. + // this value is 5. Negative values are invalid. MaxPoolCount int64 `json:"maxPoolCount,omitempty"` // HeartBeatTimeout specifies the maximum time to wait for a heartbeat // before terminating the connection. It is not recommended to change this diff --git a/pkg/config/v1/validation/server.go b/pkg/config/v1/validation/server.go index 8be740aa..54bd8348 100644 --- a/pkg/config/v1/validation/server.go +++ b/pkg/config/v1/validation/server.go @@ -51,6 +51,9 @@ func (v *ConfigValidator) ValidateServerConfig(c *v1.ServerConfig) (Warning, err errs = AppendError(errs, ValidatePort(c.VhostHTTPPort, "vhostHTTPPort")) errs = AppendError(errs, ValidatePort(c.VhostHTTPSPort, "vhostHTTPSPort")) errs = AppendError(errs, ValidatePort(c.TCPMuxHTTPConnectPort, "tcpMuxHTTPConnectPort")) + if c.Transport.MaxPoolCount < 0 { + errs = AppendError(errs, fmt.Errorf("invalid transport.maxPoolCount, must be non-negative")) + } for _, p := range c.HTTPPlugins { if !lo.Every(SupportedHTTPPluginOps, p.Ops) { diff --git a/pkg/config/v1/validation/server_test.go b/pkg/config/v1/validation/server_test.go new file mode 100644 index 00000000..7c0c57ad --- /dev/null +++ b/pkg/config/v1/validation/server_test.go @@ -0,0 +1,51 @@ +// Copyright 2026 The frp Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "math" + "testing" + + "github.com/stretchr/testify/require" + + v1 "github.com/fatedier/frp/pkg/config/v1" +) + +func TestValidateServerConfigMaxPoolCount(t *testing.T) { + for _, tc := range []struct { + name string + maxPoolCount int64 + wantErr bool + }{ + {name: "negative", maxPoolCount: -1, wantErr: true}, + {name: "zero", maxPoolCount: 0}, + {name: "positive", maxPoolCount: 5}, + {name: "maximum int64", maxPoolCount: math.MaxInt64}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := validServerConfigWithAuth(v1.AuthServerConfig{Method: v1.AuthMethodToken}) + cfg.Transport.MaxPoolCount = tc.maxPoolCount + require.NoError(t, cfg.Complete()) + + _, err := NewConfigValidator(nil).ValidateServerConfig(cfg) + if tc.wantErr { + require.ErrorContains(t, err, "invalid transport.maxPoolCount") + require.ErrorContains(t, err, "must be non-negative") + return + } + require.NoError(t, err) + }) + } +} diff --git a/server/control.go b/server/control.go index ea680d63..1690b01b 100644 --- a/server/control.go +++ b/server/control.go @@ -17,6 +17,7 @@ package server import ( "context" "fmt" + "math" "net" "runtime/debug" "sync" @@ -45,6 +46,8 @@ type ControlID uint64 var nextControlID atomic.Uint64 +const workConnPoolCapacityOffset = 10 + type controlEntry struct { ctl *Control id ControlID @@ -431,10 +434,27 @@ type Control struct { } func NewControl(ctx context.Context, sessionCtx *SessionContext) (*Control, error) { - poolCount := min(sessionCtx.LoginMsg.PoolCount, int(sessionCtx.ServerCfg.Transport.MaxPoolCount)) + if sessionCtx.LoginMsg.PoolCount < 0 { + return nil, fmt.Errorf("invalid pool count %d, must be non-negative", sessionCtx.LoginMsg.PoolCount) + } + if sessionCtx.ServerCfg.Transport.MaxPoolCount < 0 { + return nil, fmt.Errorf( + "invalid max pool count %d, must be non-negative", + sessionCtx.ServerCfg.Transport.MaxPoolCount, + ) + } + effectivePoolCount := min(int64(sessionCtx.LoginMsg.PoolCount), sessionCtx.ServerCfg.Transport.MaxPoolCount) + maxPoolCountForChannel := int64(math.MaxInt) - int64(workConnPoolCapacityOffset) + if effectivePoolCount > maxPoolCountForChannel { + return nil, fmt.Errorf( + "invalid effective pool count %d, cannot safely add %d for work connection pool capacity", + effectivePoolCount, workConnPoolCapacityOffset, + ) + } + poolCount := int(effectivePoolCount) ctl := &Control{ sessionCtx: sessionCtx, - workConnCh: make(chan *proxy.WorkConn, poolCount+10), + workConnCh: make(chan *proxy.WorkConn, poolCount+workConnPoolCapacityOffset), proxies: make(map[string]proxy.Proxy), poolCount: poolCount, portsUsedNum: 0, diff --git a/server/control_test.go b/server/control_test.go index 9dce3d24..965ea5ea 100644 --- a/server/control_test.go +++ b/server/control_test.go @@ -17,6 +17,7 @@ package server import ( "context" "errors" + "math" "net" "os" "sync" @@ -52,6 +53,57 @@ func TestControlPendingReplacementFinishesWithoutStarting(t *testing.T) { require.Equal(t, int64(0), metrics.closedClients()) } +func TestNewControlPoolCountBoundaries(t *testing.T) { + for _, tc := range []struct { + name string + poolCount int + maxPoolCount int64 + wantErr string + wantPoolCount int + wantCapacity int + }{ + {name: "negative pool count below offset", poolCount: -11, maxPoolCount: 5, wantErr: "invalid pool count"}, + {name: "negative pool count at offset", poolCount: -10, maxPoolCount: 5, wantErr: "invalid pool count"}, + {name: "negative pool count", poolCount: -1, maxPoolCount: 5, wantErr: "invalid pool count"}, + {name: "zero pool count", poolCount: 0, maxPoolCount: 5, wantPoolCount: 0, wantCapacity: 10}, + {name: "pool count capped", poolCount: 10, maxPoolCount: 5, wantPoolCount: 5, wantCapacity: 15}, + {name: "maximum int pool count capped", poolCount: math.MaxInt, maxPoolCount: 5, wantPoolCount: 5, wantCapacity: 15}, + {name: "negative maximum", poolCount: 1, maxPoolCount: -1, wantErr: "invalid max pool count"}, + {name: "maximum int64 with small client pool", poolCount: 1, maxPoolCount: math.MaxInt64, wantPoolCount: 1, wantCapacity: 11}, + {name: "maximum int client and server overflow", poolCount: math.MaxInt, maxPoolCount: math.MaxInt64, wantErr: "cannot safely add"}, + } { + t.Run(tc.name, func(t *testing.T) { + conn := newDeadlineReadConn() + msgConn := msg.NewConn(conn, msg.NewV1ReadWriter(conn)) + cfg := &v1.ServerConfig{} + cfg.Transport.MaxPoolCount = tc.maxPoolCount + + ctl, err := NewControl(context.Background(), &SessionContext{ + RC: &controller.ResourceController{}, + PxyManager: proxy.NewManager(), + PluginManager: plugin.NewManager(), + AuthVerifier: auth.AlwaysPassVerifier, + Conn: msgConn, + LoginMsg: &msg.Login{ + RunID: "pool-count-run", + PoolCount: tc.poolCount, + }, + ServerCfg: cfg, + }) + if tc.wantErr != "" { + require.Nil(t, ctl) + require.ErrorContains(t, err, tc.wantErr) + return + } + + require.NoError(t, err) + require.Equal(t, tc.wantPoolCount, ctl.poolCount) + require.Equal(t, tc.wantCapacity, cap(ctl.workConnCh)) + require.NoError(t, ctl.Close()) + }) + } +} + func TestControlRunningReplacementFinishesInWorker(t *testing.T) { clientRegistry := registry.NewClientRegistry() manager := NewControlManager(clientRegistry) diff --git a/server/service_test.go b/server/service_test.go index 20769725..cc7645f8 100644 --- a/server/service_test.go +++ b/server/service_test.go @@ -17,6 +17,7 @@ package server import ( "context" "errors" + "math" "net" "net/http" "runtime" @@ -631,6 +632,48 @@ func TestServiceRegisterControlRejectsInvalidCodecSelection(t *testing.T) { } } +func TestServiceRegisterControlPoolCountBoundaries(t *testing.T) { + for _, tc := range []struct { + name string + poolCount int + wantErr bool + wantPool int + }{ + {name: "less than channel offset", poolCount: -11, wantErr: true}, + {name: "channel offset", poolCount: -10, wantErr: true}, + {name: "negative", poolCount: -1, wantErr: true}, + {name: "zero", poolCount: 0, wantPool: 0}, + {name: "capped by server maximum", poolCount: 10, wantPool: 5}, + {name: "maximum int capped", poolCount: math.MaxInt, wantPool: 5}, + } { + t.Run(tc.name, func(t *testing.T) { + svr := newControlTestService(t) + svr.cfg.Transport.MaxPoolCount = 5 + conn := newDeadlineReadConn() + msgConn := msg.NewConn(conn, msg.NewV1ReadWriter(conn)) + const timestamp = int64(1) + + ctl, err := svr.RegisterControl(msgConn, &msg.Login{ + RunID: "pool-count-run", + ClientID: "client", + Timestamp: timestamp, + PrivilegeKey: util.GetAuthKey("", timestamp), + PoolCount: tc.poolCount, + }, false, wire.ProtocolV1, "") + if tc.wantErr { + require.Nil(t, ctl) + require.ErrorContains(t, err, "unexpected error when creating new controller") + return + } + + require.NoError(t, err) + require.Equal(t, tc.wantPool, ctl.poolCount) + require.NoError(t, ctl.Close()) + waitForControlDone(t, ctl) + }) + } +} + func TestServiceWorkConnRoutingRejectsLostGeneration(t *testing.T) { for _, action := range []string{"replace", "close"} { t.Run(action, func(t *testing.T) {