From de0c190dbb3d722debed8ebbc3a755560f098140 Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Tue, 1 Sep 2026 23:14:00 +0200 Subject: [PATCH 1/8] place the NOSONAR comment in the right place --- cpp/test/test_shared.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/test/test_shared.cpp b/cpp/test/test_shared.cpp index 3ab8ad29cd..a8b84b5a98 100644 --- a/cpp/test/test_shared.cpp +++ b/cpp/test/test_shared.cpp @@ -22,9 +22,8 @@ using namespace filesystem; // Inlude the process id in the temp file path so that multiple instances of the test procedures // could run on the same host. -const path test_data_temp_path(temp_directory_path() / - path(fmt::format("piscsi-test-{}", - getpid()))); // NOSONAR Publicly writable directory is fine here +const path test_data_temp_path(temp_directory_path() / //NOSONAR Publicly writable directory is fine for tests + path(fmt::format("piscsi-test-{}", getpid()))); pair, shared_ptr> CreateDevice(PbDeviceType type, const string& extension) { From 8084d042dd5b817d6d5b4337392cabec7b39073f Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Tue, 1 Sep 2026 23:09:36 +0200 Subject: [PATCH 2/8] piscsi-web: configuration download hardening --- go/piscsi-web/internal/server/handlers.go | 40 ++++++++++++------- go/piscsi-web/internal/server/pathutil.go | 39 ++++++++++++++++++ .../internal/server/pathutil_test.go | 22 ++++++++++ 3 files changed, 86 insertions(+), 15 deletions(-) diff --git a/go/piscsi-web/internal/server/handlers.go b/go/piscsi-web/internal/server/handlers.go index e9d61529aa..61fbe17ffb 100644 --- a/go/piscsi-web/internal/server/handlers.go +++ b/go/piscsi-web/internal/server/handlers.go @@ -1395,6 +1395,22 @@ func (s *Server) handleFilesDownload(c *gin.Context) { c.String(http.StatusBadRequest, "Invalid filename") return } + if source == "config" { + file, info, err := openRegularFileWithin(sourcePath, filename) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + c.String(http.StatusNotFound, "File not found") + } else { + c.String(http.StatusBadRequest, "Invalid file") + } + return + } + defer file.Close() + + c.Header("Content-Disposition", fmt.Sprintf("attachment; filename=%s", filepath.Base(filename))) + http.ServeContent(c.Writer, c.Request, filepath.Base(filename), info.ModTime(), file) + return + } // Check if file exists if _, err := os.Stat(realPath); os.IsNotExist(err) { @@ -1998,27 +2014,21 @@ func (s *Server) handleFilesDownloadConfig(c *gin.Context) { return } - // Construct full path - fullPath := filepath.Join(s.config.ConfigDir, fileName) - - // Verify path is within config directory - cleanPath := filepath.Clean(fullPath) - configDir := filepath.Clean(s.config.ConfigDir) - if !strings.HasPrefix(cleanPath, configDir) { - c.String(http.StatusBadRequest, "Invalid file path") - return - } - - // Check if file exists - if _, err := os.Stat(fullPath); os.IsNotExist(err) { - c.String(http.StatusNotFound, "File not found: %s", fileName) + file, info, err := openRegularFileWithin(s.config.ConfigDir, fileName) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + c.String(http.StatusNotFound, "File not found: %s", fileName) + } else { + c.String(http.StatusBadRequest, "Invalid file") + } return } + defer file.Close() // Serve the file for download c.Header("Content-Description", "File Transfer") c.Header("Content-Disposition", fmt.Sprintf("attachment; filename=%s", fileName)) - c.File(fullPath) + http.ServeContent(c.Writer, c.Request, fileName, info.ModTime(), file) } // performs an action on a configuration file (load, delete, or send) diff --git a/go/piscsi-web/internal/server/pathutil.go b/go/piscsi-web/internal/server/pathutil.go index 82dab6e3d3..228abb2749 100644 --- a/go/piscsi-web/internal/server/pathutil.go +++ b/go/piscsi-web/internal/server/pathutil.go @@ -6,11 +6,15 @@ package server import ( + "errors" "fmt" + "os" "path/filepath" "strings" ) +var errNotRegularFile = errors.New("not a regular file") + // resolvePathWithin converts a browser-facing relative path into an absolute // path below root. It rejects absolute paths and lexical traversal. func resolvePathWithin(root, name string) (string, error) { @@ -100,3 +104,38 @@ func uploadDestinationPath(root, subdirectory string) (string, error) { } return target, nil } + +// openRegularFileWithin opens name beneath root without following a symlink +// outside root. Symlink leaves are rejected so callers do not serve a file +// selected by a locally planted link. +func openRegularFileWithin(root, name string) (*os.File, os.FileInfo, error) { + rootHandle, err := os.OpenRoot(root) + if err != nil { + return nil, nil, fmt.Errorf("open root directory: %w", err) + } + defer rootHandle.Close() + + entryInfo, err := rootHandle.Lstat(name) + if err != nil { + return nil, nil, err + } + if !entryInfo.Mode().IsRegular() { + return nil, nil, errNotRegularFile + } + + file, err := rootHandle.Open(name) + if err != nil { + return nil, nil, err + } + info, err := file.Stat() + if err != nil { + file.Close() + return nil, nil, err + } + if !info.Mode().IsRegular() { + file.Close() + return nil, nil, errNotRegularFile + } + + return file, info, nil +} diff --git a/go/piscsi-web/internal/server/pathutil_test.go b/go/piscsi-web/internal/server/pathutil_test.go index 8d522f718f..3745819012 100644 --- a/go/piscsi-web/internal/server/pathutil_test.go +++ b/go/piscsi-web/internal/server/pathutil_test.go @@ -1,6 +1,7 @@ package server import ( + "errors" "os" "path/filepath" "testing" @@ -71,3 +72,24 @@ func TestUploadDestinationPathRejectsEscapingSymlink(t *testing.T) { t.Fatal("uploadDestinationPath() accepted a symlink outside the configured directory") } } + +func TestOpenRegularFileWithinRejectsSymlink(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "secret.json") + if err := os.WriteFile(outside, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "download.json") + if err := os.Symlink(outside, link); err != nil { + t.Skipf("create symbolic link: %v", err) + } + + file, _, err := openRegularFileWithin(root, "download.json") + if file != nil { + file.Close() + t.Fatal("openRegularFileWithin() opened a symbolic link") + } + if !errors.Is(err, errNotRegularFile) { + t.Fatalf("openRegularFileWithin() error = %v, want errNotRegularFile", err) + } +} From d06e64b5bb8385a4ea2b3b13304853bacc057e88 Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Tue, 1 Sep 2026 23:25:51 +0200 Subject: [PATCH 3/8] piscsi-web: harden journal log capturing --- go/piscsi-web/internal/server/handlers.go | 61 +++++++++++++-- .../server/handlers_config_download_test.go | 62 +++++++++++++++ .../internal/server/system_environment.go | 7 +- .../server/system_environment_test.go | 78 +++++++++++++++++++ go/piscsi-web/web/templates/admin.html | 2 +- go/piscsi-web/web/templates/logs.html | 2 +- os_integration/systemd/piscsi-web.service | 1 + 7 files changed, 202 insertions(+), 11 deletions(-) create mode 100644 go/piscsi-web/internal/server/handlers_config_download_test.go diff --git a/go/piscsi-web/internal/server/handlers.go b/go/piscsi-web/internal/server/handlers.go index 61fbe17ffb..51deed8b50 100644 --- a/go/piscsi-web/internal/server/handlers.go +++ b/go/piscsi-web/internal/server/handlers.go @@ -2154,22 +2154,67 @@ func (s *Server) handleLogsLevel(c *gin.Context) { }) } +const ( + defaultSystemLogLines = 100 + maxSystemLogLines = 1000 + systemLogTimeout = 10 * time.Second +) + +var systemLogScopes = map[string]struct{}{ + "piscsi": {}, + "piscsi-web": {}, + "piscsi-oled": {}, + "piscsi-ctrlboard": {}, +} + +func parseSystemLogRequest(linesValue, scope string) (int, string, error) { + lines := defaultSystemLogLines + if linesValue != "" { + parsed, err := strconv.Atoi(linesValue) + if err != nil || parsed < 1 || parsed > maxSystemLogLines { + return 0, "", fmt.Errorf("log lines must be between 1 and %d", maxSystemLogLines) + } + lines = parsed + } + + if scope != "" { + if _, ok := systemLogScopes[scope]; !ok { + return 0, "", fmt.Errorf("invalid log scope") + } + } + + return lines, scope, nil +} + // displays system logs func (s *Server) handleLogsShow(c *gin.Context) { - lines := c.DefaultPostForm("lines", "100") + linesValue := c.PostForm("lines") scope := c.PostForm("scope") + lines, scope, err := parseSystemLogRequest(linesValue, scope) + if err != nil { + s.respond(c, ResponseOptions{ + Error: true, + Message: err.Error(), + Template: "logs.html", + TemplateData: gin.H{ + "Title": "PiSCSI System Logs", + "Scope": "All logs", + "Lines": defaultSystemLogLines, + }, + }) + return + } // Build journalctl command - args := []string{} - if lines != "" { - args = append(args, "-n", lines) - } + args := []string{"--no-pager", "--lines=" + strconv.Itoa(lines)} if scope != "" { - args = append(args, "-u", scope) + args = append(args, "--unit="+scope) } // Execute journalctl command - output, err := s.runSystemCommand("journalctl", args...) + ctx, cancel := context.WithTimeout(c.Request.Context(), systemLogTimeout) + defer cancel() + output, err := s.runSystemCommandContext(ctx, "journalctl", args...) logs := string(output) if err != nil { @@ -2193,7 +2238,7 @@ func (s *Server) handleLogsShow(c *gin.Context) { // Render the logs template data := s.getBaseTemplateData(c) data["Scope"] = scopeDisplay - data["Lines"] = lines + data["Lines"] = strconv.Itoa(lines) data["Logs"] = logs data["Title"] = "PiSCSI System Logs" diff --git a/go/piscsi-web/internal/server/handlers_config_download_test.go b/go/piscsi-web/internal/server/handlers_config_download_test.go new file mode 100644 index 0000000000..83201b12b2 --- /dev/null +++ b/go/piscsi-web/internal/server/handlers_config_download_test.go @@ -0,0 +1,62 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/piscsi/piscsi/go/piscsi-web/internal/config" +) + +func TestConfigurationDownloadsRejectSymlinks(t *testing.T) { + gin.SetMode(gin.TestMode) + configDir := t.TempDir() + secretPath := filepath.Join(t.TempDir(), "secret.json") + if err := os.WriteFile(secretPath, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(secretPath, filepath.Join(configDir, "download.json")); err != nil { + t.Skipf("create symbolic link: %v", err) + } + + server := &Server{config: &config.Config{ConfigDir: configDir}} + configDownloadRequest := httptest.NewRequest(http.MethodPost, "/files/download_config", + strings.NewReader(url.Values{"file": {"download.json"}}.Encode())) + configDownloadRequest.Header.Set("Content-Type", "application/x-www-form-urlencoded") + tests := []struct { + name string + request *http.Request + handler func(*gin.Context) + }{ + { + name: "configuration download endpoint", + request: configDownloadRequest, + handler: server.handleFilesDownloadConfig, + }, + { + name: "generic download endpoint with config source", + request: httptest.NewRequest(http.MethodGet, "/files/download_image?source=config&file=download.json", nil), + handler: server.handleFilesDownload, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + response := httptest.NewRecorder() + context, _ := gin.CreateTestContext(response) + context.Request = test.request + test.handler(context) + if response.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body = %s", response.Code, http.StatusBadRequest, response.Body.String()) + } + if strings.Contains(response.Body.String(), "secret") { + t.Fatal("download response exposed symlink target content") + } + }) + } +} diff --git a/go/piscsi-web/internal/server/system_environment.go b/go/piscsi-web/internal/server/system_environment.go index 3f6896c911..2324e6f560 100644 --- a/go/piscsi-web/internal/server/system_environment.go +++ b/go/piscsi-web/internal/server/system_environment.go @@ -6,6 +6,7 @@ package server import ( + "context" "fmt" "net" "os" @@ -32,10 +33,14 @@ type serviceStatus struct { } func (s *Server) runSystemCommand(name string, args ...string) ([]byte, error) { + return s.runSystemCommandContext(context.Background(), name, args...) +} + +func (s *Server) runSystemCommandContext(ctx context.Context, name string, args ...string) ([]byte, error) { if s.systemCommand != nil { return s.systemCommand(name, args...) } - return exec.Command(name, args...).CombinedOutput() + return exec.CommandContext(ctx, name, args...).CombinedOutput() } func (s *Server) systemHostname() string { diff --git a/go/piscsi-web/internal/server/system_environment_test.go b/go/piscsi-web/internal/server/system_environment_test.go index 8f7fe54f80..cde089d24f 100644 --- a/go/piscsi-web/internal/server/system_environment_test.go +++ b/go/piscsi-web/internal/server/system_environment_test.go @@ -95,3 +95,81 @@ func TestHandleLogsShowReturnsErrorWhenJournalctlFails(t *testing.T) { t.Errorf("body = %q, want journal error details", response.Body.String()) } } + +func TestHandleLogsShowValidatesAndBoundsRequestParameters(t *testing.T) { + gin.SetMode(gin.TestMode) + for _, requestBody := range []string{ + "lines=0", + "lines=1001", + "lines=all", + "scope=sshd", + } { + t.Run(requestBody, func(t *testing.T) { + journalctlCalled := false + server := &Server{ + systemCommand: func(name string, _ ...string) ([]byte, error) { + if name == "journalctl" { + journalctlCalled = true + } + return nil, nil + }, + sessionStore: sessions.NewCookieStore([]byte("test-secret-key")), + } + templates, err := web.GetTemplates() + if err != nil { + t.Fatal(err) + } + router := gin.New() + router.SetHTMLTemplate(templates) + router.POST("/logs/show", server.handleLogsShow) + request := httptest.NewRequest(http.MethodPost, "/logs/show", strings.NewReader(requestBody)) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + + if response.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", response.Code, http.StatusBadRequest) + } + if journalctlCalled { + t.Error("journalctl was called for an invalid request") + } + }) + } +} + +func TestHandleLogsShowPassesBoundedArgumentsToJournalctl(t *testing.T) { + gin.SetMode(gin.TestMode) + var gotName string + var gotArgs []string + server := &Server{ + systemCommand: func(name string, args ...string) ([]byte, error) { + if name == "journalctl" { + gotName = name + gotArgs = append([]string(nil), args...) + } + return []byte("test log"), nil + }, + sessionStore: sessions.NewCookieStore([]byte("test-secret-key")), + } + templates, err := web.GetTemplates() + if err != nil { + t.Fatal(err) + } + router := gin.New() + router.SetHTMLTemplate(templates) + router.POST("/logs/show", server.handleLogsShow) + request := httptest.NewRequest(http.MethodPost, "/logs/show", strings.NewReader("lines=200&scope=piscsi-web")) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + + if response.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", response.Code, http.StatusOK) + } + if gotName != "journalctl" { + t.Fatalf("command = %q, want journalctl", gotName) + } + if got, want := strings.Join(gotArgs, ","), "--no-pager,--lines=200,--unit=piscsi-web"; got != want { + t.Errorf("arguments = %q, want %q", got, want) + } +} diff --git a/go/piscsi-web/web/templates/admin.html b/go/piscsi-web/web/templates/admin.html index 747b993245..084fbde77e 100644 --- a/go/piscsi-web/web/templates/admin.html +++ b/go/piscsi-web/web/templates/admin.html @@ -25,7 +25,7 @@
- + +