From c148cb8ce7618f33246cfa272fb02be6e4297de9 Mon Sep 17 00:00:00 2001 From: Deivid Soto Date: Fri, 15 May 2026 17:10:42 +0200 Subject: [PATCH] fix(security): harden HLS session IDs, /health disclosure, archive password handling Phase 1 security audit follow-up: - Reject HLS session IDs that aren't safe filesystem components (regex allowlist) to defend against path traversal via a buggy or compromised server. Applied at StartHLSSession and at the /hls URL handler; invalid IDs share the 404 of unknown sessions so the accepted format isn't enumerable. - /health no longer leaks the active filename, taskID prefix or client IP to non-loopback callers. Uses net.IP.IsLoopback so IPv4-mapped IPv6 (::ffff:127.0.0.1) is recognised and the empty-string parse failure stops bypassing the boundary. - unrar/7z passwords now travel through stdin instead of -p in argv, removing /proc//cmdline disclosure. Control characters in the password are rejected up front so a hostile NZB cannot feed extra prompt answers. Both invocations are bounded by a 30-minute context to stop indefinite hangs if the tool ever decides to prompt. --- internal/engine/hls.go | 3 + internal/engine/hls_test.go | 30 ++++++++++ internal/engine/stream_server.go | 33 +++++++++-- internal/engine/stream_server_test.go | 81 ++++++++++++++++++++++++++ internal/engine/validate.go | 12 ++++ internal/usenet/postprocess/extract.go | 70 ++++++++++++++++++---- 6 files changed, 213 insertions(+), 16 deletions(-) create mode 100644 internal/engine/validate.go diff --git a/internal/engine/hls.go b/internal/engine/hls.go index 537a79b..03a9948 100644 --- a/internal/engine/hls.go +++ b/internal/engine/hls.go @@ -241,6 +241,9 @@ func StartHLSSession(ctx context.Context, cfg HLSSessionConfig) (*HLSSession, er if cfg.SessionID == "" { return nil, errors.New("hls: empty session id") } + if !validSessionID.MatchString(cfg.SessionID) { + return nil, errors.New("hls: invalid session id") + } if cfg.SourcePath == "" { return nil, errors.New("hls: empty source path") } diff --git a/internal/engine/hls_test.go b/internal/engine/hls_test.go index 0aea35d..7c7cfa4 100644 --- a/internal/engine/hls_test.go +++ b/internal/engine/hls_test.go @@ -261,3 +261,33 @@ func TestCleanupHLSOrphanDirsMissingRoot(t *testing.T) { t.Errorf("CleanupHLSOrphanDirs on missing root = %v, want nil", err) } } + +func TestValidSessionID(t *testing.T) { + good := []string{ + "abc", + "7b8c4f12-9d3e-4a1b-9c2f-aabbccddeeff", + "ABC_123-xyz", + strings.Repeat("a", 128), + } + bad := []string{ + "", + "../etc/passwd", + "foo/bar", + "foo\\bar", + "foo.bar", + "with spaces", + "with\nnewline", + strings.Repeat("a", 129), + "héctor", // non-ascii + } + for _, id := range good { + if !validSessionID.MatchString(id) { + t.Errorf("validSessionID rejected good id %q", id) + } + } + for _, id := range bad { + if validSessionID.MatchString(id) { + t.Errorf("validSessionID accepted bad id %q", id) + } + } +} diff --git a/internal/engine/stream_server.go b/internal/engine/stream_server.go index 2e42868..7440979 100644 --- a/internal/engine/stream_server.go +++ b/internal/engine/stream_server.go @@ -303,6 +303,12 @@ func (ss *StreamServer) hlsHandler(w http.ResponseWriter, r *http.Request) { return } sessionID := parts[0] + // Reject malformed IDs with the same 404 we return for unknown sessions — + // no oracle for the accepted format. + if !validSessionID.MatchString(sessionID) { + http.Error(w, "hls session not found", http.StatusNotFound) + return + } session := ss.hls.Get(sessionID) if session == nil { http.Error(w, "hls session not found", http.StatusNotFound) @@ -392,6 +398,17 @@ func (ss *StreamServer) healthHandler(w http.ResponseWriter, r *http.Request) { ss.mu.RUnlock() clientIP, _, _ := net.SplitHostPort(r.RemoteAddr) + // Only expose filename/taskID/client to loopback callers (local diagnostics). + // Remote callers (LAN, Tailscale, UPnP public) get a minimal probe response + // so that scanners and unauthenticated peers cannot fingerprint the active + // download. The web stream-probe only checks HTTP 200 + Content-Type. + // + // Use net.IP.IsLoopback so we also accept ::ffff:127.0.0.1 (Linux dual-stack + // IPv4-mapped form) and reject the empty-string fallthrough when + // SplitHostPort fails on a malformed RemoteAddr — both would otherwise + // silently bypass the disclosure boundary. + parsedIP := net.ParseIP(clientIP) + isLocal := parsedIP != nil && parsedIP.IsLoopback() type healthResponse struct { Status string `json:"status"` @@ -399,19 +416,23 @@ func (ss *StreamServer) healthHandler(w http.ResponseWriter, r *http.Request) { File string `json:"file,omitempty"` Task string `json:"task,omitempty"` Port int `json:"port"` - Client string `json:"client"` + Client string `json:"client,omitempty"` } resp := healthResponse{ Status: "ok", Port: ss.port, - Client: clientIP, } if provider != nil { resp.Streaming = true - resp.File = provider.FileName() - resp.Task = taskID - if len(resp.Task) > 8 { - resp.Task = resp.Task[:8] + } + if isLocal { + resp.Client = clientIP + if provider != nil { + resp.File = provider.FileName() + resp.Task = taskID + if len(resp.Task) > 8 { + resp.Task = resp.Task[:8] + } } } diff --git a/internal/engine/stream_server_test.go b/internal/engine/stream_server_test.go index 623a16d..3c58b1a 100644 --- a/internal/engine/stream_server_test.go +++ b/internal/engine/stream_server_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "strings" "sync" "testing" @@ -379,6 +380,86 @@ func TestStreamServer_Health_WithFile(t *testing.T) { } } +// TestStreamServer_Health_NonLoopback_NoLeak verifica que /health no revela +// nombre de fichero, taskID ni client IP cuando el caller no es loopback. +// Protección contra reconnaissance vía LAN / UPnP / Tailscale. +func TestStreamServer_Health_NonLoopback_NoLeak(t *testing.T) { + srv := NewStreamServer(0) + srv.disableUPnP = true + ctx := context.Background() + if err := srv.Listen(ctx); err != nil { + t.Fatalf("Listen() error: %v", err) + } + defer srv.Shutdown(ctx) + + provider := newFakeProvider("secret.mkv", []byte("data")) + srv.SetFile(provider, "secret-task-id") + + cases := []struct { + name string + remoteAddr string + }{ + {"lan_ipv4", "192.168.1.50:54321"}, + {"empty_host_no_bypass", ":54321"}, + {"public_ipv4", "203.0.113.10:443"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/health", nil) + req.RemoteAddr = tc.remoteAddr + srv.healthHandler(rr, req) + + body := rr.Body.String() + if !strings.Contains(body, `"status":"ok"`) { + t.Errorf("body missing status:ok: %q", body) + } + if !strings.Contains(body, `"streaming":true`) { + t.Errorf("body should report streaming bool: %q", body) + } + if strings.Contains(body, "secret.mkv") { + t.Errorf("body leaked filename: %q", body) + } + if strings.Contains(body, "secret-t") { + t.Errorf("body leaked task id: %q", body) + } + if strings.Contains(body, "192.168.1.50") || strings.Contains(body, "203.0.113.10") { + t.Errorf("body leaked client ip: %q", body) + } + }) + } +} + +// TestStreamServer_HLS_InvalidSessionID verifica que el hlsHandler rechaza +// session IDs con caracteres ilegales devolviendo 404 (uniforme con sesión +// inexistente) para no filtrar el formato aceptado a un attacker. +func TestStreamServer_HLS_InvalidSessionID(t *testing.T) { + srv := NewStreamServer(0) + srv.disableUPnP = true + ctx := context.Background() + if err := srv.Listen(ctx); err != nil { + t.Fatalf("Listen() error: %v", err) + } + defer srv.Shutdown(ctx) + + bad := []string{ + "/hls/..%2Fetc%2Fpasswd/master.m3u8", + "/hls/foo.bar/master.m3u8", + "/hls/foo%20bar/master.m3u8", + "/hls/foo%2Fbar/master.m3u8", + } + for _, path := range bad { + t.Run(path, func(t *testing.T) { + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, path, nil) + srv.hlsHandler(rr, req) + if rr.Code != http.StatusNotFound { + t.Errorf("path %q: status = %d, want 404", path, rr.Code) + } + }) + } +} + // TestStreamServer_MKV_ContentType verifica que el Content-Type para .mkv // es el correcto. func TestStreamServer_MKV_ContentType(t *testing.T) { diff --git a/internal/engine/validate.go b/internal/engine/validate.go new file mode 100644 index 0000000..288a41b --- /dev/null +++ b/internal/engine/validate.go @@ -0,0 +1,12 @@ +// Package engine — validate.go centralises input validators used by the +// stream/HLS HTTP handlers and the daemon glue. Keep new validators in this +// file so a future reviewer can audit the trust boundary in one place. +package engine + +import "regexp" + +// validSessionID restricts session IDs to characters safe for use as a single +// filesystem path component. Server-issued UUIDs and hex strings match this; +// anything containing slashes, dots, or path separators is rejected so a +// compromised or buggy server cannot escape hlsTmpDirRoot via os.MkdirAll. +var validSessionID = regexp.MustCompile(`^[a-zA-Z0-9_-]{1,128}$`) diff --git a/internal/usenet/postprocess/extract.go b/internal/usenet/postprocess/extract.go index 0a9b582..0c6a8e3 100644 --- a/internal/usenet/postprocess/extract.go +++ b/internal/usenet/postprocess/extract.go @@ -1,6 +1,7 @@ package postprocess import ( + "context" "fmt" "log" "os" @@ -8,8 +9,25 @@ import ( "path/filepath" "regexp" "strings" + "time" ) +// extractTimeout caps how long a single extractor invocation may run. Without +// a cap, an encrypted archive that triggers a TTY-only prompt (or a corrupt +// archive that confuses the tool) hangs the post-process pipeline forever. +const extractTimeout = 30 * time.Minute + +// validatePassword rejects passwords containing control characters that could +// inject extra answers into unrar/7z prompts via stdin (e.g. a newline lets an +// attacker-controlled NZB password feed a second response to overwrite or +// rename prompts). +func validatePassword(password string) error { + if strings.ContainsAny(password, "\r\n\x00") { + return fmt.Errorf("invalid password: contains control characters") + } + return nil +} + // ExtractorType identifies which extraction tool is available. type ExtractorType string @@ -50,18 +68,35 @@ func Extract(archivePath string, outputDir string, password string) ([]string, e } // extractUnrar extracts using unrar. +// +// Security: when a password is supplied it is sent via stdin rather than via +// the `-p` switch so it does not appear in `/proc//cmdline` +// (visible to any other process on the host). unrar prompts for the password +// when no `-p` switch is given, and reads the prompt response from stdin when +// no controlling TTY is attached (the usual case for a daemon-spawned child). func extractUnrar(unrarPath, archivePath, outputDir, password string) ([]string, error) { + if err := validatePassword(password); err != nil { + return nil, err + } args := []string{"x", "-o+", "-y"} - if password != "" { - args = append(args, "-p"+password) - } else { - args = append(args, "-p-") // no password, skip asking + if password == "" { + // Tell unrar there is no password so it skips the prompt and fails + // fast on encrypted archives instead of hanging. + args = append(args, "-p-") } args = append(args, archivePath, outputDir+"/") - cmd := exec.Command(unrarPath, args...) + ctx, cancel := context.WithTimeout(context.Background(), extractTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, unrarPath, args...) cmd.Dir = outputDir + if password != "" { + cmd.Stdin = strings.NewReader(password + "\n") + } output, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { + return nil, fmt.Errorf("unrar: timed out after %s", extractTimeout) + } if err != nil { // Check for password error outStr := string(output) @@ -75,18 +110,33 @@ func extractUnrar(unrarPath, archivePath, outputDir, password string) ([]string, } // extract7z extracts using 7z. +// +// Security: same rationale as extractUnrar — passwords go through stdin to +// avoid `/proc//cmdline` exposure. 7z reads the password from stdin when +// no `-p` switch is given and the archive is encrypted. func extract7z(szPath, archivePath, outputDir, password string) ([]string, error) { + if err := validatePassword(password); err != nil { + return nil, err + } args := []string{"x", "-y", "-o" + outputDir} - if password != "" { - args = append(args, "-p"+password) - } else { - args = append(args, "-p") // empty password + if password == "" { + // `-p` with no value tells 7z the password is empty so encrypted + // archives fail fast instead of waiting for a prompt. + args = append(args, "-p") } args = append(args, archivePath) - cmd := exec.Command(szPath, args...) + ctx, cancel := context.WithTimeout(context.Background(), extractTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, szPath, args...) cmd.Dir = outputDir + if password != "" { + cmd.Stdin = strings.NewReader(password + "\n") + } output, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { + return nil, fmt.Errorf("7z: timed out after %s", extractTimeout) + } if err != nil { outStr := string(output) if strings.Contains(outStr, "Wrong password") || strings.Contains(outStr, "incorrect password") {