diff --git a/cmd/dashboard/controller/csrf.go b/cmd/dashboard/controller/csrf.go index 0d3a498c..53c22d25 100644 --- a/cmd/dashboard/controller/csrf.go +++ b/cmd/dashboard/controller/csrf.go @@ -1,28 +1,79 @@ package controller import ( + "crypto/hmac" "crypto/rand" + "crypto/sha256" "encoding/hex" "net/http" + "strings" "github.com/gin-gonic/gin" "github.com/nezhahq/nezha/model" + "github.com/nezhahq/nezha/service/singleton" ) -// setCSRFCookie issues a fresh CSRF token cookie. Called by login + refresh -// handlers so the frontend always has a paired value to mirror back into -// the X-CSRF-Token header. The cookie is intentionally HttpOnly=false — +// issueCSRFToken mints a signed double-submit token (nonce.HMAC-SHA256 keyed +// by the JWT secret). Signing defeats sibling-subdomain cookie tossing: a +// naive double-submit trusts any header==cookie pair, but an injected cookie +// carries no valid HMAC and fails validateCSRFToken. Returns "" pre-init +// (no secret); callers treat that as "no cookie minted". +func issueCSRFToken() string { + secret := csrfSigningSecret() + if secret == "" { + return "" + } + var b [32]byte + if _, err := rand.Read(b[:]); err != nil { + return "" + } + nonce := hex.EncodeToString(b[:]) + return nonce + "." + csrfSign(nonce, secret) +} + +func csrfSigningSecret() string { + if singleton.Conf == nil { + return "" + } + return singleton.Conf.JWTSecretKey +} + +func csrfSign(nonce, secret string) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write([]byte(nonce)) + return hex.EncodeToString(mac.Sum(nil)) +} + +// validateCSRFToken reports whether value is a well-formed nonce.signature +// pair whose signature verifies under the current server secret. Constant +// -time comparison guards against signature-probing side channels. +func validateCSRFToken(value string) bool { + secret := csrfSigningSecret() + if secret == "" || value == "" { + return false + } + idx := strings.LastIndex(value, ".") + if idx <= 0 || idx == len(value)-1 { + return false + } + nonce, sig := value[:idx], value[idx+1:] + return hmac.Equal([]byte(sig), []byte(csrfSign(nonce, secret))) +} + +// setCSRFCookie issues a fresh signed CSRF token cookie. Called by login + +// refresh handlers so the frontend always has a paired value to mirror back +// into the X-CSRF-Token header. The cookie is intentionally HttpOnly=false — // SPA JS must be able to read it. SameSite=Strict here (not Lax) because // the cookie's sole purpose is the same-origin double-submit check and we // don't want it leaking on cross-site GET navigation either. func setCSRFCookie(c *gin.Context) { - var b [32]byte - if _, err := rand.Read(b[:]); err != nil { + token := issueCSRFToken() + if token == "" { return } c.SetSameSite(http.SameSiteStrictMode) - c.SetCookie(csrfCookieName, hex.EncodeToString(b[:]), 0, "/", "", false, false) + c.SetCookie(csrfCookieName, token, 0, "/", "", false, false) } const ( @@ -49,6 +100,7 @@ const ( // - Missing or empty X-CSRF-Token header. // - Missing or empty nz-csrf cookie. // - Header value != cookie value. +// - Cookie value not signed by the server (validateCSRFToken fails). // // The middleware DOES NOT set the csrf cookie on its own — that is the // JWT login / refresh handler's job, since those are the only places that @@ -77,7 +129,10 @@ func csrfMiddleware() gin.HandlerFunc { } header := c.GetHeader(csrfHeaderName) cookie, err := c.Cookie(csrfCookieName) - if err != nil || cookie == "" || header == "" || header != cookie { + // Both halves must be present, mirror each other, AND carry a valid + // server signature. The signature check is what stops a cookie-tossed + // pair from a sibling subdomain. + if err != nil || cookie == "" || header == "" || header != cookie || !validateCSRFToken(cookie) { c.AbortWithStatusJSON(http.StatusForbidden, model.CommonResponse[any]{ Success: false, Error: "ApiErrorForbidden: missing or invalid CSRF token", diff --git a/cmd/dashboard/controller/csrf_signed_test.go b/cmd/dashboard/controller/csrf_signed_test.go new file mode 100644 index 00000000..df740590 --- /dev/null +++ b/cmd/dashboard/controller/csrf_signed_test.go @@ -0,0 +1,84 @@ +package controller + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/nezhahq/nezha/model" + "github.com/nezhahq/nezha/service/singleton" +) + +func withCSRFSecret(t *testing.T, secret string) { + t.Helper() + prev := singleton.Conf + singleton.Conf = &singleton.ConfigClass{Config: &model.Config{}} + singleton.Conf.JWTSecretKey = secret + t.Cleanup(func() { singleton.Conf = prev }) +} + +// Sibling-subdomain cookie tossing: an attacker who can set nz-csrf for the +// parent domain injects an attacker-chosen value and mirrors it into the +// header. A naive double-submit accepts header==cookie. The signed +// double-submit must reject it because the injected value carries no valid +// server HMAC. +func TestCSRFMiddleware_RejectsUnsignedInjectedPair(t *testing.T) { + withCSRFSecret(t, "test-jwt-secret") + mw := csrfMiddleware() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/api/v1/profile", nil) + c.Request.AddCookie(&http.Cookie{Name: "nz-jwt", Value: "session"}) + c.Request.AddCookie(&http.Cookie{Name: csrfCookieName, Value: "attacker-chosen"}) + c.Request.Header.Set(csrfHeaderName, "attacker-chosen") + mw(c) + if !c.IsAborted() || w.Code != http.StatusForbidden { + t.Fatalf("an unsigned (cookie-tossed) csrf pair must be rejected, got aborted=%v code=%d", c.IsAborted(), w.Code) + } +} + +// A token minted by the server (issueCSRFToken) must pass when mirrored +// correctly into the header. +func TestCSRFMiddleware_AcceptsServerSignedPair(t *testing.T) { + withCSRFSecret(t, "test-jwt-secret") + token := issueCSRFToken() + if token == "" { + t.Fatal("issueCSRFToken must produce a value") + } + mw := csrfMiddleware() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/api/v1/profile", nil) + c.Request.AddCookie(&http.Cookie{Name: "nz-jwt", Value: "session"}) + c.Request.AddCookie(&http.Cookie{Name: csrfCookieName, Value: token}) + c.Request.Header.Set(csrfHeaderName, token) + mw(c) + if c.IsAborted() { + t.Fatal("a correctly mirrored server-signed csrf pair must pass") + } +} + +// Even with header==cookie, a value whose signature does not verify under the +// server secret must be rejected (forged signature segment). +func TestCSRFMiddleware_RejectsForgedSignature(t *testing.T) { + withCSRFSecret(t, "test-jwt-secret") + token := issueCSRFToken() + forged := token[:len(token)-1] + if token[len(token)-1] == 'a' { + forged += "b" + } else { + forged += "a" + } + mw := csrfMiddleware() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/api/v1/profile", nil) + c.Request.AddCookie(&http.Cookie{Name: csrfCookieName, Value: forged}) + c.Request.Header.Set(csrfHeaderName, forged) + mw(c) + if !c.IsAborted() || w.Code != http.StatusForbidden { + t.Fatalf("a forged-signature csrf pair must be rejected, got aborted=%v code=%d", c.IsAborted(), w.Code) + } +} diff --git a/cmd/dashboard/controller/csrf_test.go b/cmd/dashboard/controller/csrf_test.go index fec7275f..e4e94529 100644 --- a/cmd/dashboard/controller/csrf_test.go +++ b/cmd/dashboard/controller/csrf_test.go @@ -39,6 +39,7 @@ func TestCSRFMiddleware_AllowsSafeMethodsWithoutToken(t *testing.T) { // POST — would 403 forever and force a manual re-login. GET carries no CSRF // risk, so the middleware mints the cookie when it is absent. func TestCSRFMiddleware_SeedsCookieOnSafeMethodWhenMissing(t *testing.T) { + withCSRFSecret(t, "test-jwt-secret") mw := csrfMiddleware() w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -90,6 +91,8 @@ func TestCSRFMiddleware_BlocksUnsafeMethodWithoutToken(t *testing.T) { } func TestCSRFMiddleware_AcceptsMatchingHeaderAndCookie(t *testing.T) { + withCSRFSecret(t, "test-jwt-secret") + token := issueCSRFToken() mw := csrfMiddleware() for _, m := range []string{"POST", "PATCH", "PUT", "DELETE"} { t.Run(m, func(t *testing.T) { @@ -97,11 +100,11 @@ func TestCSRFMiddleware_AcceptsMatchingHeaderAndCookie(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest(m, "/api/v1/profile", nil) c.Request.AddCookie(&http.Cookie{Name: "nz-jwt", Value: "anything"}) - c.Request.AddCookie(&http.Cookie{Name: "nz-csrf", Value: "matching-token"}) - c.Request.Header.Set("X-CSRF-Token", "matching-token") + c.Request.AddCookie(&http.Cookie{Name: "nz-csrf", Value: token}) + c.Request.Header.Set("X-CSRF-Token", token) mw(c) if c.IsAborted() { - t.Fatalf("%s with matching csrf header+cookie must pass", m) + t.Fatalf("%s with matching signed csrf header+cookie must pass", m) } }) } diff --git a/cmd/dashboard/controller/mcp_transfer.go b/cmd/dashboard/controller/mcp_transfer.go index cc2dad9d..e144fd0a 100644 --- a/cmd/dashboard/controller/mcp_transfer.go +++ b/cmd/dashboard/controller/mcp_transfer.go @@ -40,7 +40,7 @@ import ( // 安全机制： // - 一次性 token，存内存 sync.Map，TTL 默认 300s，最多 600s // - token 绑定 user_id + token_id + server_id + path + direction -// - 走 HMAC-SHA256 防篡改 +// - consume 时重算并以常数时间比对 entry 的 HMAC-SHA256，防篡改 // - 命中后立即从内存删除，禁止重放 // - revalidateTransferEntry 在 consume 时重新校验 PAT/scope/owner，应对 // mint→consume 之间的权限变化 @@ -54,6 +54,11 @@ const ( transferTokenTTLDefault = 300 * time.Second transferTokenTTLMax = 600 * time.Second + // maxTransferDuration bounds a single upload/download once the agent has + // attached. 100MiB over a slow link still completes well within this; + // anything longer is treated as a stalled/abusive transfer and cancelled. + maxTransferDuration = 10 * time.Minute + maxTransferPathLen = 4096 ) @@ -105,16 +110,20 @@ func transferHMACSecret() string { return transferSecretVal } +// transferTokenSig 计算 entry 的 HMAC-SHA256 签名（hex）；mint 与 consume 共用。 +func transferTokenSig(e transferEntry) string { + mac := hmac.New(sha256.New, []byte(transferHMACSecret())) + fmt.Fprintf(mac, "%s|%d|%d|%d|%s|%d", + e.Direction, e.UserID, e.TokenID, e.ServerID, e.Path, e.ExpiresAt.UnixNano()) + return hex.EncodeToString(mac.Sum(nil)) +} + func mintTransferToken(e transferEntry) (string, error) { id, err := utils.GenerateRandomString(24) if err != nil { return "", err } - mac := hmac.New(sha256.New, []byte(transferHMACSecret())) - fmt.Fprintf(mac, "%s|%d|%d|%d|%s|%d", - e.Direction, e.UserID, e.TokenID, e.ServerID, e.Path, e.ExpiresAt.UnixNano()) - sig := hex.EncodeToString(mac.Sum(nil)) - tok := id + "." + sig + tok := id + "." + transferTokenSig(e) transferEntries.Store(tok, e) return tok, nil } @@ -125,6 +134,16 @@ func consumeTransferToken(tok string, dir transferDirection) (*transferEntry, er return nil, errors.New("invalid or already-used transfer token") } e, _ := raw.(transferEntry) + // 校验 HMAC：token 形如 id.sig，sig 必须等于 entry 字段在进程 secret 下的 + // HMAC-SHA256。仅靠 sync.Map key 随机性不构成完整性保护——一旦 entry 被 + // 持久化/跨副本共享/从 token 解码，缺这一步即认证绕过。常数时间比较防侧信道。 + idx := strings.LastIndex(tok, ".") + if idx < 0 { + return nil, errors.New("malformed transfer token") + } + if !hmac.Equal([]byte(tok[idx+1:]), []byte(transferTokenSig(e))) { + return nil, errors.New("transfer token signature mismatch") + } if e.Direction != dir { return nil, errors.New("transfer token direction mismatch") } @@ -263,8 +282,10 @@ func handleFsUploadURL(c *gin.Context, raw json.RawMessage) (any, error) { if err := decodeToolArgs(raw, &args); err != nil { return nil, err } - if args.IfMatchSHA256 != "" && len(args.IfMatchSHA256) != 64 { - return nil, errMCPInvalidArgs("if_match_sha256 must be 64 hex chars") + if args.IfMatchSHA256 != "" { + if _, decErr := hex.DecodeString(args.IfMatchSHA256); decErr != nil || len(args.IfMatchSHA256) != 64 { + return nil, errMCPInvalidArgs("if_match_sha256 must be 64 hex chars") + } } return mintTransferTool(c, args.ServerID, args.Path, args.TTLSeconds, transferDirUpload, transferEntry{ UploadMode: args.Mode, @@ -339,7 +360,14 @@ func mintTransferTool(c *gin.Context, serverID uint64, path string, ttlSeconds i // 该 PAT 被 deleteAPIToken 撤销时取消，从而切断已开始的 upload/download； // 否则只在传输自然结束时由 stop() 注销。stop() 必须 defer 调用。 func transferRevokableContext(c *gin.Context, e *transferEntry) (context.Context, func()) { - ctx, cancel := context.WithCancel(c.Request.Context()) + // Cap the whole transfer with a hard deadline. After the agent attaches, + // the relay blocks in IOStreamWrapper.Read, which only honours this ctx + // (openFsTransferStream closes the stream on ctx.Done). Without the + // deadline a stalled or malicious agent that attaches but never sends a + // complete header/chunk/final frame pins this goroutine, the IOStream and + // the spool tmpfile until the client disconnects, allowing concurrent + // hung transfers to exhaust resources within the rate limit. + ctx, cancel := context.WithTimeout(c.Request.Context(), maxTransferDuration) dereg := patConnectionRegistryShared.register(e.TokenID, cancel) return ctx, func() { dereg() @@ -676,7 +704,14 @@ func relayDownloadFrames(c *gin.Context, stream io.ReadWriteCloser, size int64) } chunkLen := binary.BigEndian.Uint64(header[4:12]) if chunkLen == 0 { - continue + // A zero-length data frame makes no progress toward `remaining`. + // Treating it as a no-op `continue` lets a malicious or buggy + // agent stream an unbounded run of zero-length NZTC frames, + // pinning this goroutine, the gRPC stream and the spool tmpfile + // forever (the final NZTO is never reached). Reject it: a real + // transfer that still owes bytes never needs an empty data frame. + c.String(http.StatusBadGateway, "stream relay failed: zero-length data frame while payload incomplete") + return errMCPMidstreamAbort } if int64(chunkLen) > remaining { c.String(http.StatusBadGateway, "agent oversent: more data bytes than declared size") @@ -982,6 +1017,13 @@ func revalidateTransferEntry(e *transferEntry) error { if err := singleton.DB.First(&tok, e.TokenID).Error; err != nil { return errors.New("originating api token no longer exists") } + // Bind the reloaded token back to the minting user. If the original PAT + // was deleted and its numeric primary key reused by a different user's + // token, the row would still load here; without this check the stale + // one-time URL would be revalidated against an unrelated token. + if tok.UserID != e.UserID { + return errors.New("originating api token no longer exists") + } if tok.IsExpired(time.Now()) { return errors.New("originating api token expired") } diff --git a/cmd/dashboard/controller/mcp_transfer_token_hmac_test.go b/cmd/dashboard/controller/mcp_transfer_token_hmac_test.go new file mode 100644 index 00000000..77c0d789 --- /dev/null +++ b/cmd/dashboard/controller/mcp_transfer_token_hmac_test.go @@ -0,0 +1,75 @@ +package controller + +import ( + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// A transfer token whose HMAC signature does not match the stored entry +// must be rejected at consume time. The signature is documented as +// "HMAC-SHA256 防篡改" (tamper-proof); if consume never verifies it, the +// guarantee is hollow. This test pins that the signature is actually +// checked: a stored entry under a token id with a forged/altered sig must +// not be consumable. +func TestConsumeTransferToken_RejectsTamperedSignature(t *testing.T) { + e := transferEntry{ + UserID: 1, + TokenID: 2, + ServerID: 3, + Path: "/srv/file", + Direction: transferDirDownload, + ExpiresAt: time.Now().Add(time.Minute), + } + tok, err := mintTransferToken(e) + require.NoError(t, err) + require.Contains(t, tok, ".", "token must carry an id.sig shape") + + // Flip the last hex nibble of the signature to forge a mismatching MAC + // while keeping the same random id portion. + idx := strings.LastIndex(tok, ".") + require.Greater(t, idx, 0) + id, sig := tok[:idx], tok[idx+1:] + last := sig[len(sig)-1] + var flipped byte + if last == '0' { + flipped = '1' + } else { + flipped = '0' + } + forged := id + "." + sig[:len(sig)-1] + string(flipped) + + // Re-store the entry under the forged token so the map lookup itself + // would succeed — only the HMAC check should reject it. + transferEntries.Store(forged, e) + t.Cleanup(func() { transferEntries.Delete(forged) }) + + got, err := consumeTransferToken(forged, transferDirDownload) + require.Error(t, err, "tampered-signature token must be rejected") + require.Nil(t, got) +} + +// A correctly minted token must still consume successfully and be single-use. +func TestConsumeTransferToken_ValidSignatureRoundTrips(t *testing.T) { + e := transferEntry{ + UserID: 10, + TokenID: 20, + ServerID: 30, + Path: "/srv/other", + Direction: transferDirUpload, + ExpiresAt: time.Now().Add(time.Minute), + } + tok, err := mintTransferToken(e) + require.NoError(t, err) + + got, err := consumeTransferToken(tok, transferDirUpload) + require.NoError(t, err) + require.NotNil(t, got) + require.Equal(t, e.Path, got.Path) + + // single-use: second consume must fail. + _, err = consumeTransferToken(tok, transferDirUpload) + require.Error(t, err) +} diff --git a/cmd/dashboard/controller/oauth2_csrf_test.go b/cmd/dashboard/controller/oauth2_csrf_test.go index 1b9f5f10..2c034c98 100644 --- a/cmd/dashboard/controller/oauth2_csrf_test.go +++ b/cmd/dashboard/controller/oauth2_csrf_test.go @@ -12,6 +12,7 @@ import ( // it so OAuth-only sessions can satisfy the double-submit CSRF gate. func TestSetCSRFCookieIssuesReadableToken(t *testing.T) { gin.SetMode(gin.TestMode) + withCSRFSecret(t, "test-jwt-secret") w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/", nil)