From d1aac6b98989b79d66c0f52bd18dc8c3142e7601 Mon Sep 17 00:00:00 2001 From: "honest.tai" <43802073+honestTai@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:14:56 +0800 Subject: [PATCH] fix(payment): close EasyPay forged-callback sign reuse (fixes #7881) An order-creation signature (visible to the payer in popup submit.php URLs) could be replayed as a payment-success notification, with no merchant key required. Two compounding defects: - CanonicalizeReturnURL kept client-supplied query parameters, so a return_url ending in '&trade_status=TRADE_SUCCESS' entered the signed order string verbatim; - the sign base string concatenates values unescaped, so a callback that decodes return_url only up to the smuggled pair re-sorts to a byte-identical sign base and passes verification. Fixes: - CanonicalizeReturnURL now drops client query parameters entirely; the server builds its own signed set and the client query has no legitimate use; - VerifyNotification rejects any parameter outside the genuine async notify set (pid/trade_no/out_trade_no/type/name/money/trade_status/ param/sign/sign_type), failing closed against the whole smuggling class regardless of the injection point. Regression tests cover the PoC payload, full order-URL replay, genuine callback acceptance, unknown-parameter rejection (including empty values), and the return_url smuggling strip. Genuinely paid orders that an exotic upstream variant might reject are still recovered through the existing upstream QueryOrder reconcile path. --- backend/internal/payment/provider/easypay.go | 26 +++ .../provider/easypay_notify_security_test.go | 172 ++++++++++++++++++ .../service/payment_resume_service.go | 7 + .../service/payment_resume_service_test.go | 29 ++- 4 files changed, 230 insertions(+), 4 deletions(-) create mode 100644 backend/internal/payment/provider/easypay_notify_security_test.go diff --git a/backend/internal/payment/provider/easypay.go b/backend/internal/payment/provider/easypay.go index 53e5651327..bbb007bb3e 100644 --- a/backend/internal/payment/provider/easypay.go +++ b/backend/internal/payment/provider/easypay.go @@ -376,6 +376,9 @@ func (e *EasyPay) VerifyNotification(_ context.Context, rawBody string, _ map[st // url.ParseQuery already decodes values — no additional decode needed. params := make(map[string]string) for k := range values { + if !easyPayNotifyAllowedParams[k] { + return nil, fmt.Errorf("unexpected notify param: %s", k) + } params[k] = values.Get(k) } sign := params["sign"] @@ -601,3 +604,26 @@ func easyPaySign(params map[string]string, pkey string) string { func easyPayVerifySign(params map[string]string, pkey string, sign string) bool { return hmac.Equal([]byte(easyPaySign(params, pkey)), []byte(sign)) } + +// easyPayNotifyAllowedParams is the exact parameter set a genuine EasyPay +// (彩虹易支付-compatible) async notification carries. Order-creation-only +// fields (notify_url, return_url, cid, device, clientip, ...) must never +// appear in a callback: because the sign base string concatenates values +// unescaped, a signed order URL whose return_url embeds e.g. +// "trade_status=TRADE_SUCCESS" could otherwise be replayed as a forged +// payment-success notification (issue #7881). Rejecting unknown keys closes +// the whole smuggling class; genuinely paid orders rejected by an exotic +// upstream variant are still recovered by the upstream QueryOrder reconcile +// path. +var easyPayNotifyAllowedParams = map[string]bool{ + "pid": true, + "trade_no": true, + "out_trade_no": true, + "type": true, + "name": true, + "money": true, + "trade_status": true, + "param": true, + "sign": true, + "sign_type": true, +} diff --git a/backend/internal/payment/provider/easypay_notify_security_test.go b/backend/internal/payment/provider/easypay_notify_security_test.go new file mode 100644 index 0000000000..66a373e71b --- /dev/null +++ b/backend/internal/payment/provider/easypay_notify_security_test.go @@ -0,0 +1,172 @@ +package provider + +// Regression tests for the EasyPay forged-callback vulnerability +// (Wei-Shaw/sub2api issue #7875). +// +// Attack recap: the order-creation signature (visible to the payer in the +// submit.php popup URL) signs return_url among other fields, and the sign +// base string concatenates values unescaped. A client-supplied return_url +// ending in "&trade_status=TRADE_SUCCESS" therefore produced a signature +// that was byte-identical to a payment-success notification's signature +// once the callback decoded return_url partially and promoted the smuggled +// pair to a top-level parameter. Merchant-key rotation could not help +// because the attacker never needed the key. +// +// Fixes covered here: +// 1. service.CanonicalizeReturnURL drops client-supplied query parameters +// (see payment_resume_service_test.go). +// 2. VerifyNotification rejects any parameter outside the genuine async +// notify set, so smuggled order-creation fields (return_url et al.) +// fail closed even if a signed value smuggles a fake pair. + +import ( + "context" + "net/url" + "testing" + + "github.com/Wei-Shaw/sub2api/internal/payment" +) + +// easyPayPoCProvider returns a provider with a fixed test credential set. +func easyPayPoCProvider() *EasyPay { + return &EasyPay{config: map[string]string{ + "pid": "1000", + "pkey": "MERCHANT_SECRET_KEY", + "apiBase": "https://pay.example.com", + "notifyUrl": "https://site.example.com/api/v1/payment/webhook/easypay", + "returnUrl": "https://site.example.com/payment/result", + }} +} + +// TestEasyPayNotifyRejectsForgedSignReuseCallback is the exact PoC payload +// that was accepted before the fix: the order's own submit.php signature +// replayed with trade_status smuggled out of the return_url value. +func TestEasyPayNotifyRejectsForgedSignReuseCallback(t *testing.T) { + t.Parallel() + e := easyPayPoCProvider() + + // Order creation (popup mode): return_url carries the smuggled pair as + // its last sorted inner query key, exactly as buildPaymentReturnURL + // would have produced before CanonicalizeReturnURL stripped user query. + returnURL := "https://site.example.com/payment/result?order_id=99&out_trade_no=ORDER123&status=success&trade_status=TRADE_SUCCESS" + createParams := map[string]string{ + "pid": "1000", + "type": "alipay", + "out_trade_no": "ORDER123", + "notify_url": e.config["notifyUrl"], + "return_url": returnURL, + "name": "balance recharge", + "money": "650.00", + } + sign := easyPaySign(createParams, e.config["pkey"]) + + // Forged callback: return_url encoded only up to status=success, then a + // raw &trade_status=TRADE_SUCCESS promotes it to a top-level parameter. + prefix := "https://site.example.com/payment/result?order_id=99&out_trade_no=ORDER123&status=success" + cb := url.Values{} + cb.Set("pid", "1000") + cb.Set("type", "alipay") + cb.Set("out_trade_no", "ORDER123") + cb.Set("notify_url", e.config["notifyUrl"]) + cb.Set("name", "balance recharge") + cb.Set("money", "650.00") + cb.Set("return_url", prefix) + rawCallback := cb.Encode() + "&trade_status=TRADE_SUCCESS" + "&sign=" + sign + "&sign_type=MD5" + + if _, err := e.VerifyNotification(context.Background(), rawCallback, nil); err == nil { + t.Fatal("forged sign-reuse callback must be rejected") + } +} + +// TestEasyPayNotifyRejectsOrderURLReplay covers the milder variant where the +// attacker replays the complete signed pay URL unchanged: return_url itself +// is not a legitimate notify parameter and must also be rejected. +func TestEasyPayNotifyRejectsOrderURLReplay(t *testing.T) { + t.Parallel() + e := easyPayPoCProvider() + + createParams := map[string]string{ + "pid": "1000", + "type": "alipay", + "out_trade_no": "ORDER123", + "notify_url": e.config["notifyUrl"], + "return_url": "https://site.example.com/payment/result", + "name": "balance recharge", + "money": "650.00", + } + createParams["sign"] = easyPaySign(createParams, e.config["pkey"]) + createParams["sign_type"] = signTypeMD5 + q := url.Values{} + for k, v := range createParams { + q.Set(k, v) + } + + if _, err := e.VerifyNotification(context.Background(), q.Encode(), nil); err == nil { + t.Fatal("replayed order URL must be rejected") + } +} + +// TestEasyPayNotifyAcceptsGenuineCallback locks in the legitimate notify +// contract: the canonical parameter set with a valid signature succeeds. +func TestEasyPayNotifyAcceptsGenuineCallback(t *testing.T) { + t.Parallel() + e := easyPayPoCProvider() + + params := map[string]string{ + "pid": "1000", + "trade_no": "2026100622001400000001", + "out_trade_no": "ORDER123", + "type": "alipay", + "name": "balance recharge", + "money": "650.00", + "trade_status": tradeStatusSuccess, + } + sign := easyPaySign(params, e.config["pkey"]) + params["sign"] = sign + params["sign_type"] = signTypeMD5 + q := url.Values{} + for k, v := range params { + q.Set(k, v) + } + + n, err := e.VerifyNotification(context.Background(), q.Encode(), nil) + if err != nil { + t.Fatalf("genuine callback rejected: %v", err) + } + if n.Status != payment.ProviderStatusSuccess { + t.Fatalf("status = %v, want success", n.Status) + } + if n.OrderID != "ORDER123" || n.TradeNo != "2026100622001400000001" || n.Amount != 650.00 { + t.Fatalf("unexpected notification: %+v", n) + } +} + +// TestEasyPayNotifyRejectsUnknownParam ensures any parameter outside the +// canonical notify set fails closed, including empty-valued ones that the +// signer itself would skip. +func TestEasyPayNotifyRejectsUnknownParam(t *testing.T) { + t.Parallel() + e := easyPayPoCProvider() + + params := map[string]string{ + "pid": "1000", + "trade_no": "T1", + "out_trade_no": "ORDER123", + "type": "alipay", + "name": "balance recharge", + "money": "650.00", + "trade_status": tradeStatusSuccess, + } + sign := easyPaySign(params, e.config["pkey"]) + q := url.Values{} + for k, v := range params { + q.Set(k, v) + } + q.Set("sign", sign) + q.Set("sign_type", signTypeMD5) + q.Set("device", "") // empty value: invisible to the signer, still rejected + + if _, err := e.VerifyNotification(context.Background(), q.Encode(), nil); err == nil { + t.Fatal("callback with unknown param must be rejected") + } +} diff --git a/backend/internal/service/payment_resume_service.go b/backend/internal/service/payment_resume_service.go index 9ae62fde33..7bc76c8009 100644 --- a/backend/internal/service/payment_resume_service.go +++ b/backend/internal/service/payment_resume_service.go @@ -245,6 +245,13 @@ func CanonicalizeReturnURL(raw string, srcHost string, srcURL string) (string, e return "", infraerrors.BadRequest("INVALID_RETURN_URL", "return_url must use http or https") } parsed.Fragment = "" + // Client-supplied query parameters are dropped: buildPaymentReturnURL adds + // its own signed set, and user-controlled pairs such as + // "trade_status=TRADE_SUCCESS" inside the signed return_url value can be + // smuggled into EasyPay-style sign base strings (values are concatenated + // unescaped), letting an order-creation signature be replayed as a forged + // payment-success callback. See issue #7881. + parsed.RawQuery = "" if parsed.Path == "" { parsed.Path = "/" } diff --git a/backend/internal/service/payment_resume_service_test.go b/backend/internal/service/payment_resume_service_test.go index 17b637fa23..54b85b4a37 100644 --- a/backend/internal/service/payment_resume_service_test.go +++ b/backend/internal/service/payment_resume_service_test.go @@ -10,6 +10,7 @@ import ( "encoding/json" "net/url" "strconv" + "strings" "testing" "time" @@ -81,12 +82,30 @@ func TestNormalizePaymentSource(t *testing.T) { func TestCanonicalizeReturnURL(t *testing.T) { t.Parallel() + // Client-supplied query and fragment are both dropped: user-controlled + // pairs inside the signed return_url value could be smuggled into + // EasyPay-style sign base strings (issue #7881). got, err := CanonicalizeReturnURL("https://example.com/payment/result?b=2#a", "example.com", "") if err != nil { t.Fatalf("CanonicalizeReturnURL returned error: %v", err) } - if got != "https://example.com/payment/result?b=2" { - t.Fatalf("CanonicalizeReturnURL = %q, want %q", got, "https://example.com/payment/result?b=2") + if got != "https://example.com/payment/result" { + t.Fatalf("CanonicalizeReturnURL = %q, want %q", got, "https://example.com/payment/result") + } +} + +func TestCanonicalizeReturnURLStripsSmuggledTradeStatus(t *testing.T) { + t.Parallel() + + got, err := CanonicalizeReturnURL( + "https://example.com/payment/result?trade_status=TRADE_SUCCESS", + "example.com", "", + ) + if err != nil { + t.Fatalf("CanonicalizeReturnURL returned error: %v", err) + } + if strings.Contains(got, "trade_status") { + t.Fatalf("CanonicalizeReturnURL kept smuggled trade_status: %q", got) } } @@ -117,8 +136,10 @@ func TestCanonicalizeReturnURLAllowsConfiguredFrontendHost(t *testing.T) { if err != nil { t.Fatalf("CanonicalizeReturnURL returned error: %v", err) } - if got != "https://app.example.com/payment/result?from=checkout" { - t.Fatalf("CanonicalizeReturnURL = %q, want %q", got, "https://app.example.com/payment/result?from=checkout") + // The host allowlist still applies, but client query parameters are + // always dropped (issue #7881: signed-value parameter smuggling). + if got != "https://app.example.com/payment/result" { + t.Fatalf("CanonicalizeReturnURL = %q, want %q", got, "https://app.example.com/payment/result") } }