From 36297699f55364ee6cc10ec1ee0f312bb47f62b7 Mon Sep 17 00:00:00 2001 From: naiba Date: Mon, 18 May 2026 15:16:40 +0000 Subject: [PATCH] fix(rpc): bind io_stream sessions to creator to prevent terminal/fm hijack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createTerminal and createFM correctly check server ownership before issuing a stream UUID, but terminalStream and fmStream only verified that the UUID existed. Any authenticated user holding a valid stream UUID could attach to it, gaining the original creator's live shell or file-manager session — and the UUID is exposed via URL path (referer leaks, access logs, browser history, frontend error reporters). Bind the creator user ID into ioStreamContext at CreateStream time, expose StreamOwnership and IsStreamAuthorizedForUser, and check ownership in terminalStream/fmStream before the WebSocket upgrade so a rejected attempt does not tear down the legitimate stream via defer. NAT streams are also routed through CreateStream(_, 0); they are not reachable from /ws/terminal or /ws/file so a sentinel user ID is fine. Co-authored-by: naiba/CloudCode --- cmd/dashboard/controller/fm.go | 8 +- .../controller/stream_ownership_test.go | 117 ++++++++++++++++++ cmd/dashboard/controller/terminal.go | 9 +- cmd/dashboard/rpc/rpc.go | 5 +- service/rpc/io_stream.go | 34 ++++- service/rpc/io_stream_test.go | 70 ++++++++++- 6 files changed, 238 insertions(+), 5 deletions(-) create mode 100644 cmd/dashboard/controller/stream_ownership_test.go diff --git a/cmd/dashboard/controller/fm.go b/cmd/dashboard/controller/fm.go index 87699114..85ac2386 100644 --- a/cmd/dashboard/controller/fm.go +++ b/cmd/dashboard/controller/fm.go @@ -46,7 +46,7 @@ func createFM(c *gin.Context) (*model.CreateFMResponse, error) { return nil, err } - rpc.NezhaHandlerSingleton.CreateStream(streamId) + rpc.NezhaHandlerSingleton.CreateStream(streamId, getUid(c)) fmData, _ := json.Marshal(&model.TaskFM{ StreamID: streamId, @@ -72,6 +72,12 @@ func createFM(c *gin.Context) (*model.CreateFMResponse, error) { // @Router /ws/file/{id} [get] func fmStream(c *gin.Context) (any, error) { streamId := c.Param("id") + // GHSA-style fix: io_stream sessions must be reachable only by their creator + // (or an admin). Without this, any authenticated user who learns a stream + // UUID can hijack a live file-manager session on the target server. + if !rpc.NezhaHandlerSingleton.IsStreamAuthorizedForUser(streamId, getUid(c), callerIsAdmin(c)) { + return nil, singleton.Localizer.ErrorT("permission denied") + } if _, err := rpc.NezhaHandlerSingleton.GetStream(streamId); err != nil { return nil, err } diff --git a/cmd/dashboard/controller/stream_ownership_test.go b/cmd/dashboard/controller/stream_ownership_test.go new file mode 100644 index 00000000..ae88f233 --- /dev/null +++ b/cmd/dashboard/controller/stream_ownership_test.go @@ -0,0 +1,117 @@ +package controller + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + + "github.com/nezhahq/nezha/model" + "github.com/nezhahq/nezha/pkg/i18n" + "github.com/nezhahq/nezha/service/rpc" + "github.com/nezhahq/nezha/service/singleton" +) + +func ensureLocalizerForStreamTests(t *testing.T) { + t.Helper() + if singleton.Localizer == nil { + singleton.Localizer = i18n.NewLocalizer("en_US", "nezha", "translations", i18n.Translations) + } + // upgrader stays nil — these tests must reject the caller BEFORE WS upgrade. + // If a test ever reaches the upgrade path it will panic on nil upgrader, + // surfacing the regression. +} + +// decodeCommonResponseError returns Success and Error of a CommonResponse[any]. +func decodeCommonResponseError(t *testing.T, body []byte) (bool, string) { + t.Helper() + var resp struct { + Success bool `json:"success"` + Error string `json:"error"` + } + if err := json.Unmarshal(body, &resp); err != nil { + t.Fatalf("decode response: %v body=%s", err, string(body)) + } + return resp.Success, resp.Error +} + +func setAuthUser(c *gin.Context, userID uint64, role model.Role) { + c.Set(model.CtxKeyAuthorizedUser, &model.User{ + Common: model.Common{ID: userID}, + Role: role, + }) +} + +func TestTerminalStreamRejectsForeignMember(t *testing.T) { + gin.SetMode(gin.TestMode) + ensureLocalizerForStreamTests(t) + rpc.NezhaHandlerSingleton = rpc.NewNezhaHandler() + rpc.NezhaHandlerSingleton.CreateStream("alice-terminal", 100) + + r := gin.New() + r.Use(func(c *gin.Context) { + setAuthUser(c, 200, model.RoleMember) // bob + c.Next() + }) + r.GET("/ws/terminal/:id", commonHandler(terminalStream)) + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/ws/terminal/alice-terminal", nil) + r.ServeHTTP(w, req) + + success, errMsg := decodeCommonResponseError(t, w.Body.Bytes()) + assert.False(t, success, "foreign member must not be authorized to attach to alice's terminal") + assert.Contains(t, errMsg, "permission denied") + + // And the existing stream must NOT have been torn down by the failed attempt. + _, stillExists := rpc.NezhaHandlerSingleton.StreamOwnership("alice-terminal") + assert.True(t, stillExists, "rejected attempt must not destroy the legitimate session") +} + +func TestFMStreamRejectsForeignMember(t *testing.T) { + gin.SetMode(gin.TestMode) + ensureLocalizerForStreamTests(t) + rpc.NezhaHandlerSingleton = rpc.NewNezhaHandler() + rpc.NezhaHandlerSingleton.CreateStream("alice-fm", 100) + + r := gin.New() + r.Use(func(c *gin.Context) { + setAuthUser(c, 200, model.RoleMember) + c.Next() + }) + r.GET("/ws/file/:id", commonHandler(fmStream)) + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/ws/file/alice-fm", nil) + r.ServeHTTP(w, req) + + success, errMsg := decodeCommonResponseError(t, w.Body.Bytes()) + assert.False(t, success, "foreign member must not be authorized to attach to alice's FM session") + assert.Contains(t, errMsg, "permission denied") + + _, stillExists := rpc.NezhaHandlerSingleton.StreamOwnership("alice-fm") + assert.True(t, stillExists, "rejected attempt must not destroy the legitimate FM session") +} + +func TestTerminalStreamRejectsUnknownStreamID(t *testing.T) { + gin.SetMode(gin.TestMode) + ensureLocalizerForStreamTests(t) + rpc.NezhaHandlerSingleton = rpc.NewNezhaHandler() + + r := gin.New() + r.Use(func(c *gin.Context) { + setAuthUser(c, 100, model.RoleMember) + c.Next() + }) + r.GET("/ws/terminal/:id", commonHandler(terminalStream)) + + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/ws/terminal/nonexistent", nil) + r.ServeHTTP(w, req) + + success, _ := decodeCommonResponseError(t, w.Body.Bytes()) + assert.False(t, success, "unknown stream id must produce an error response") +} diff --git a/cmd/dashboard/controller/terminal.go b/cmd/dashboard/controller/terminal.go index 7899f1b7..e109920c 100644 --- a/cmd/dashboard/controller/terminal.go +++ b/cmd/dashboard/controller/terminal.go @@ -44,7 +44,7 @@ func createTerminal(c *gin.Context) (*model.CreateTerminalResponse, error) { return nil, err } - rpc.NezhaHandlerSingleton.CreateStream(streamId) + rpc.NezhaHandlerSingleton.CreateStream(streamId, getUid(c)) terminalData, _ := json.Marshal(&model.TerminalTask{ StreamID: streamId, @@ -72,6 +72,13 @@ func createTerminal(c *gin.Context) (*model.CreateTerminalResponse, error) { // @Router /ws/terminal/{id} [get] func terminalStream(c *gin.Context) (any, error) { streamId := c.Param("id") + // GHSA-style fix: io_stream sessions must be reachable only by their creator + // (or an admin). Without this, any authenticated user who learns a stream + // UUID — via Referer leak, access logs, browser history — can hijack a live + // terminal and gain shell access to the target server. + if !rpc.NezhaHandlerSingleton.IsStreamAuthorizedForUser(streamId, getUid(c), callerIsAdmin(c)) { + return nil, singleton.Localizer.ErrorT("permission denied") + } if _, err := rpc.NezhaHandlerSingleton.GetStream(streamId); err != nil { return nil, err } diff --git a/cmd/dashboard/rpc/rpc.go b/cmd/dashboard/rpc/rpc.go index 68c84cd6..ca148708 100644 --- a/cmd/dashboard/rpc/rpc.go +++ b/cmd/dashboard/rpc/rpc.go @@ -138,7 +138,10 @@ func ServeNAT(w http.ResponseWriter, r *http.Request, natConfig *model.NAT) { return } - rpcService.NezhaHandlerSingleton.CreateStream(streamId) + // NAT streams are anonymous HTTP-facing tunnels; they are NOT reachable + // via /ws/terminal or /ws/file (which check stream ownership), so the + // creator user ID does not need to identify a real user. + rpcService.NezhaHandlerSingleton.CreateStream(streamId, 0) defer rpcService.NezhaHandlerSingleton.CloseStream(streamId) taskData, err := json.Marshal(model.TaskNAT{ diff --git a/service/rpc/io_stream.go b/service/rpc/io_stream.go index 87ef0aa2..161be3b4 100644 --- a/service/rpc/io_stream.go +++ b/service/rpc/io_stream.go @@ -11,6 +11,7 @@ import ( ) type ioStreamContext struct { + creatorUserID uint64 userIo io.ReadWriteCloser agentIo io.ReadWriteCloser userIoConnectCh chan struct{} @@ -31,16 +32,47 @@ var bufPool = sync.Pool{ }, } -func (s *NezhaHandler) CreateStream(streamId string) { +func (s *NezhaHandler) CreateStream(streamId string, creatorUserID uint64) { s.ioStreamMutex.Lock() defer s.ioStreamMutex.Unlock() s.ioStreams[streamId] = &ioStreamContext{ + creatorUserID: creatorUserID, userIoConnectCh: make(chan struct{}), agentIoConnectCh: make(chan struct{}), } } +// IsStreamAuthorizedForUser checks whether the requesting user may attach to +// the stream. A stream is reachable only by its creator or by an admin; any +// other authenticated user must be rejected. Unknown streams are always +// rejected. +func (s *NezhaHandler) IsStreamAuthorizedForUser(streamId string, userID uint64, isAdmin bool) bool { + creator, found := s.StreamOwnership(streamId) + if !found { + return false + } + if isAdmin { + return true + } + return creator == userID +} + +// StreamOwnership returns the user ID that created the stream and whether the +// stream is still tracked. Callers must compare the returned creator against +// the requesting user before attaching to the stream — without this the +// channel becomes a session-hijack primitive (terminal/file manager RCE). +func (s *NezhaHandler) StreamOwnership(streamId string) (uint64, bool) { + s.ioStreamMutex.RLock() + defer s.ioStreamMutex.RUnlock() + + ctx, ok := s.ioStreams[streamId] + if !ok { + return 0, false + } + return ctx.creatorUserID, true +} + func (s *NezhaHandler) GetStream(streamId string) (*ioStreamContext, error) { s.ioStreamMutex.RLock() defer s.ioStreamMutex.RUnlock() diff --git a/service/rpc/io_stream_test.go b/service/rpc/io_stream_test.go index ea90387c..3fb5e08d 100644 --- a/service/rpc/io_stream_test.go +++ b/service/rpc/io_stream_test.go @@ -12,7 +12,7 @@ func TestIOStream(t *testing.T) { const testStreamID = "ffffffff-ffff-ffff-ffff-ffffffffffff" - handler.CreateStream(testStreamID) + handler.CreateStream(testStreamID, 0) userIo, agentIo := newPipeReadWriter(), newPipeReadWriter() defer func() { userIo.Close() @@ -105,3 +105,71 @@ func newPipeReadWriter() io.ReadWriteCloser { io.WriteCloser }{r, w} } + +func TestStreamOwnershipReturnsCreatorUserID(t *testing.T) { + h := NewNezhaHandler() + h.CreateStream("alice-stream", 100) + + creator, found := h.StreamOwnership("alice-stream") + if !found { + t.Fatalf("expected stream to be found after CreateStream") + } + if creator != 100 { + t.Fatalf("expected creator user ID 100, got %d", creator) + } +} + +func TestStreamOwnershipReturnsNotFoundForUnknownID(t *testing.T) { + h := NewNezhaHandler() + if _, found := h.StreamOwnership("nonexistent"); found { + t.Fatalf("expected unknown stream id to report not-found") + } +} + +func TestStreamOwnershipPreservesPerStreamCreator(t *testing.T) { + h := NewNezhaHandler() + h.CreateStream("alice-stream", 100) + h.CreateStream("bob-stream", 200) + + aliceCreator, _ := h.StreamOwnership("alice-stream") + bobCreator, _ := h.StreamOwnership("bob-stream") + if aliceCreator != 100 || bobCreator != 200 { + t.Fatalf("expected per-stream creator IDs alice=100 bob=200, got alice=%d bob=%d", + aliceCreator, bobCreator) + } +} + +func TestIsStreamAuthorizedForUserAllowsCreator(t *testing.T) { + h := NewNezhaHandler() + h.CreateStream("alice-stream", 100) + + if !h.IsStreamAuthorizedForUser("alice-stream", 100, false) { + t.Fatalf("creator must be authorized to attach to their own stream") + } +} + +func TestIsStreamAuthorizedForUserDeniesForeignMember(t *testing.T) { + h := NewNezhaHandler() + h.CreateStream("alice-stream", 100) + + if h.IsStreamAuthorizedForUser("alice-stream", 200, false) { + t.Fatalf("foreign member must not be authorized — session hijack would be possible") + } +} + +func TestIsStreamAuthorizedForUserAllowsAdmin(t *testing.T) { + h := NewNezhaHandler() + h.CreateStream("alice-stream", 100) + + if !h.IsStreamAuthorizedForUser("alice-stream", 999, true) { + t.Fatalf("admin must be authorized to attach regardless of creator") + } +} + +func TestIsStreamAuthorizedForUserDeniesUnknownStream(t *testing.T) { + h := NewNezhaHandler() + + if h.IsStreamAuthorizedForUser("nonexistent", 100, true) { + t.Fatalf("unknown stream id must not authorize even admin") + } +}