From 2b75390529619a5ca57f6ec21b1e38b8dab05e1a Mon Sep 17 00:00:00 2001 From: Rogee Date: Sat, 22 Aug 2026 21:19:43 +0800 Subject: [PATCH] HH-452: cover upload security boundaries (#106) Co-authored-by: Rogee --- backend/internal/security/security_test.go | 82 +++++++++++++++++++ backend/internal/security/ssrf_protection.go | 7 +- backend/internal/service/upload_service.go | 5 ++ .../internal/service/upload_service_test.go | 45 ++++++++++ 4 files changed, 137 insertions(+), 2 deletions(-) diff --git a/backend/internal/security/security_test.go b/backend/internal/security/security_test.go index 80450989..66876016 100644 --- a/backend/internal/security/security_test.go +++ b/backend/internal/security/security_test.go @@ -1,6 +1,7 @@ package security import ( + "bufio" "context" "crypto/hmac" "crypto/sha256" @@ -12,6 +13,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync/atomic" "testing" "time" @@ -614,6 +616,86 @@ func TestSafeRedirectCheckConfig_BlocksPrivateAndNonHTTPRedirects(t *testing.T) } } +func TestSafeHTTPClient_RevalidatesEveryRealRedirect(t *testing.T) { + var firstHits, secondHits, privateHits atomic.Int32 + private := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + privateHits.Add(1) + })) + defer private.Close() + second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + secondHits.Add(1) + http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound) + })) + defer second.Close() + first := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + firstHits.Add(1) + http.Redirect(w, r, "http://second.example/next", http.StatusFound) + })) + defer first.Close() + + addresses := map[string]string{ + "first.example": first.Listener.Addr().String(), + "second.example": second.Listener.Addr().String(), + "169.254.169.254": private.Listener.Addr().String(), + } + client := NewSafeHTTPClient(DefaultSSRFConfig()) + client.client.Transport = &http.Transport{DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + host, _, err := net.SplitHostPort(addr) + if err != nil { + return nil, err + } + return (&net.Dialer{}).DialContext(ctx, network, addresses[host]) + }} + + req, err := http.NewRequest(http.MethodGet, "http://first.example/start", nil) + require.NoError(t, err) + resp, err := client.Do(req) + if resp != nil { + resp.Body.Close() + } + require.ErrorContains(t, err, "private/reserved") + assert.Equal(t, int32(1), firstHits.Load()) + assert.Equal(t, int32(1), secondHits.Load()) + assert.Zero(t, privateHits.Load()) +} + +func TestSafeHTTPClient_DialsFirstValidatedDNSResult(t *testing.T) { + serverConn, clientConn := net.Pipe() + defer serverConn.Close() + serverDone := make(chan error, 1) + go func() { + request, err := http.ReadRequest(bufio.NewReader(serverConn)) + if err == nil { + request.Body.Close() + _, err = serverConn.Write([]byte("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok")) + } + serverDone <- err + }() + + lookups := 0 + dialedAddress := "" + client := newSafeHTTPClient(DefaultSSRFConfig(), + func(context.Context, string) ([]net.IPAddr, error) { + lookups++ + if lookups == 1 { + return []net.IPAddr{{IP: net.ParseIP("93.184.216.34")}}, nil + } + return []net.IPAddr{{IP: net.ParseIP("127.0.0.1")}}, nil + }, + func(_ context.Context, _, addr string) (net.Conn, error) { + dialedAddress = addr + return clientConn, nil + }, + ) + + resp, err := client.SafeFetchURL(context.Background(), "http://rebind.example/file") + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, 1, lookups) + assert.Equal(t, "93.184.216.34:80", dialedAddress) + require.NoError(t, <-serverDone) +} + func TestValidateURL_Empty(t *testing.T) { err := ValidateURL("", DefaultSSRFConfig()) assert.Error(t, err) diff --git a/backend/internal/security/ssrf_protection.go b/backend/internal/security/ssrf_protection.go index 459aad5c..8daaab61 100644 --- a/backend/internal/security/ssrf_protection.go +++ b/backend/internal/security/ssrf_protection.go @@ -90,7 +90,10 @@ func NewSafeHTTPClient(cfg SSRFConfig) *SafeHTTPClient { Timeout: 10 * time.Second, KeepAlive: 30 * time.Second, } + return newSafeHTTPClient(cfg, net.DefaultResolver.LookupIPAddr, dialer.DialContext) +} +func newSafeHTTPClient(cfg SSRFConfig, lookupIPAddr func(context.Context, string) ([]net.IPAddr, error), dialContext func(context.Context, string, string) (net.Conn, error)) *SafeHTTPClient { transport := &http.Transport{ DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { host, port, err := net.SplitHostPort(addr) @@ -98,7 +101,7 @@ func NewSafeHTTPClient(cfg SSRFConfig) *SafeHTTPClient { return nil, fmt.Errorf("invalid address: %s", addr) } - ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) + ips, err := lookupIPAddr(ctx, host) if err != nil { return nil, fmt.Errorf("DNS resolution failed for %s: %w", host, err) } @@ -113,7 +116,7 @@ func NewSafeHTTPClient(cfg SSRFConfig) *SafeHTTPClient { // would reopen a DNS-rebinding window between validation and connect. var lastErr error for _, ip := range ips { - conn, err := dialer.DialContext(ctx, network, net.JoinHostPort(ip.IP.String(), port)) + conn, err := dialContext(ctx, network, net.JoinHostPort(ip.IP.String(), port)) if err == nil { return conn, nil } diff --git a/backend/internal/service/upload_service.go b/backend/internal/service/upload_service.go index 1ab54770..2f5bf83c 100644 --- a/backend/internal/service/upload_service.go +++ b/backend/internal/service/upload_service.go @@ -1036,6 +1036,11 @@ func readValidatedUpload(reader io.Reader, filename, declaredMIME string, expect return nil, "", fmt.Errorf("unsupported file type: extension %s", filepath.Ext(filename)) } } + sample := bytes.TrimSpace(data[:min(len(data), 512)]) + sample = bytes.TrimSpace(bytes.TrimPrefix(sample, []byte{0xef, 0xbb, 0xbf})) + if (declaredMIME == "text/plain" || declaredMIME == "text/csv") && bytes.HasPrefix(sample, []byte("<")) { + return nil, "", fmt.Errorf("active markup is not allowed for MIME type %s", declaredMIME) + } if !uploadMIMEMatches(declaredMIME, detectedMIME) { return nil, "", fmt.Errorf("file content type %s does not match declared type %s", detectedMIME, declaredMIME) } diff --git a/backend/internal/service/upload_service_test.go b/backend/internal/service/upload_service_test.go index d1a88931..a7273352 100644 --- a/backend/internal/service/upload_service_test.go +++ b/backend/internal/service/upload_service_test.go @@ -22,6 +22,7 @@ import ( "github.com/gochat/gochat/internal/config" "github.com/gochat/gochat/internal/model" "github.com/gochat/gochat/internal/repository" + "github.com/gochat/gochat/internal/security" ) var ( @@ -362,6 +363,50 @@ func TestUploadService_RejectsSpoofedMIMEAndSVG(t *testing.T) { } } +func TestUploadService_RejectsTextPlainSVGAndXMLAcrossUploadPaths(t *testing.T) { + _, svc, _ := setupUploadServiceTest(t) + payloads := map[string][]byte{ + "svg": []byte(``), + "xml": []byte(``), + } + paths := map[string]func(*testing.T, []byte) error{ + "direct": func(t *testing.T, payload []byte) error { + file := createTestFileHeader(t, "attack.txt", payload) + file.Header.Set("Content-Type", "text/plain") + _, err := svc.AccountDirectUpload(context.Background(), 1, AccountDirectUploadRequest{FileHeader: file}) + return err + }, + "widget": func(t *testing.T, payload []byte) error { + file := createTestFileHeader(t, "attack.txt", payload) + file.Header.Set("Content-Type", "text/plain") + _, err := svc.WidgetDirectUpload(context.Background(), WidgetDirectUploadRequest{ + WebsiteToken: "test-website", + AuthToken: "test-widget", + FileHeader: file, + }) + return err + }, + "remote": func(t *testing.T, payload []byte) error { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/plain") + _, _ = w.Write(payload) + })) + defer server.Close() + svc.fetchClient = security.NewSafeHTTPClient(security.SSRFConfig{MaxRedirects: 3}) + _, err := svc.AccountUploadFromURL(context.Background(), 1, server.URL+"/attack.txt") + return err + }, + } + + for payloadName, payload := range payloads { + for pathName, upload := range paths { + t.Run(payloadName+"/"+pathName, func(t *testing.T) { + require.ErrorContains(t, upload(t, payload), "active markup") + }) + } + } +} + func TestUploadService_AccountUploadFromURLRejectsPrivateTargets(t *testing.T) { _, svc, _ := setupUploadServiceTest(t) for _, target := range []string{