diff --git a/backend/pkg/docker/client.go b/backend/pkg/docker/client.go index 5eb7b268..067d2caa 100644 --- a/backend/pkg/docker/client.go +++ b/backend/pkg/docker/client.go @@ -640,7 +640,7 @@ func (dc *dockerClient) ListContainerDir( if err != nil { return ContainerDirListing{}, fmt.Errorf("failed to attach list exec for '%s': %w", dirPath, err) } - output, readErr := demuxExecStdout(resp.Reader) + output, readErr := demuxExecStdout(resp.Reader, maxListStdoutBytes) resp.Close() if readErr != nil { return ContainerDirListing{}, fmt.Errorf("failed to read list output for '%s': %w", dirPath, readErr) @@ -654,22 +654,7 @@ func (dc *dockerClient) ListContainerDir( return ContainerDirListing{}, fmt.Errorf("list command failed for '%s' with exit code %d: %s", dirPath, inspect.ExitCode, string(output)) } - // find -print0 emits `\0\0…`. - entryPaths := make([]string, 0) - for _, tok := range strings.Split(string(output), "\x00") { - if tok == "" { - continue - } - entryPaths = append(entryPaths, tok) - } - - // Over the cap, list the first maxListEntries and flag the truncation rather - // than failing the whole listing — the caller reports the partiality. - truncated := false - if len(entryPaths) > maxListEntries { - entryPaths = entryPaths[:maxListEntries] - truncated = true - } + entryPaths, truncated := parseFindEntries(output) stats, failures := statContainerEntries(ctx, entryPaths, containerListWorkers, func(ctx context.Context, entryPath string) (container.PathStat, error) { return dc.ContainerStatPath(ctx, containerID, entryPath) @@ -695,10 +680,27 @@ func (dc *dockerClient) ListContainerDir( return listing, nil } +// parseFindEntries splits `find -print0` output (NUL-delimited absolute paths), +// dropping empties, and caps the result at maxListEntries — returning truncated=true +// so the caller reports the partiality instead of failing or silently dropping. +func parseFindEntries(output []byte) (entries []string, truncated bool) { + for _, tok := range strings.Split(string(output), "\x00") { + if tok == "" { + continue + } + entries = append(entries, tok) + } + if len(entries) > maxListEntries { + return entries[:maxListEntries], true + } + return entries, false +} + // demuxExecStdout reads a non-TTY Docker exec stream — stdout and stderr // interleaved as frames with an 8-byte header (stream id + big-endian size) — -// and returns only the stdout bytes. -func demuxExecStdout(r io.Reader) ([]byte, error) { +// and returns only the stdout bytes, erroring if stdout exceeds maxStdout so a +// compromised sandbox can't stream unbounded output into memory. +func demuxExecStdout(r io.Reader, maxStdout int) ([]byte, error) { var stdout bytes.Buffer header := make([]byte, 8) for { @@ -719,8 +721,8 @@ func demuxExecStdout(r io.Reader) ([]byte, error) { if _, err := io.CopyN(sink, r, size); err != nil { return nil, err } - if stdout.Len() > maxListStdoutBytes { - return nil, fmt.Errorf("listing output exceeded %d bytes", maxListStdoutBytes) + if stdout.Len() > maxStdout { + return nil, fmt.Errorf("listing output exceeded %d bytes", maxStdout) } } return stdout.Bytes(), nil diff --git a/backend/pkg/docker/client_listing_test.go b/backend/pkg/docker/client_listing_test.go new file mode 100644 index 00000000..b3b57f69 --- /dev/null +++ b/backend/pkg/docker/client_listing_test.go @@ -0,0 +1,55 @@ +package docker + +import ( + "bytes" + "encoding/binary" + "strings" + "testing" +) + +func listingFrame(streamID byte, payload string) []byte { + h := make([]byte, 8) + h[0] = streamID + binary.BigEndian.PutUint32(h[4:8], uint32(len(payload))) + return append(h, []byte(payload)...) +} + +func TestParseFindEntries(t *testing.T) { + entries, truncated := parseFindEntries([]byte("a\x00b\x00\x00c\x00")) + if truncated { + t.Error("small input must not truncate") + } + if len(entries) != 3 || entries[0] != "a" || entries[1] != "b" || entries[2] != "c" { + t.Fatalf("split/skip-empty wrong: %q", entries) + } + + // exactly at the cap → not truncated + e, tr := parseFindEntries([]byte(strings.Repeat("x\x00", maxListEntries))) + if tr || len(e) != maxListEntries { + t.Fatalf("at cap: truncated=%v len=%d (want false, %d)", tr, len(e), maxListEntries) + } + + // cap+1 → truncated, capped to maxListEntries + e, tr = parseFindEntries([]byte(strings.Repeat("x\x00", maxListEntries+1))) + if !tr || len(e) != maxListEntries { + t.Fatalf("over cap: truncated=%v len=%d (want true, %d)", tr, len(e), maxListEntries) + } +} + +func TestDemuxExecStdout_StdoutOnly_AndByteCap(t *testing.T) { + // stdout is returned; stderr (id 2) is discarded + in := append(listingFrame(1, "hello"), listingFrame(2, "diagnostic")...) + out, err := demuxExecStdout(bytes.NewReader(in), 1<<20) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if string(out) != "hello" { + t.Fatalf("want stdout only, got %q", out) + } + + // stdout exceeding the cap → error before materializing the full buffer + _, err = demuxExecStdout(bytes.NewReader(listingFrame(1, strings.Repeat("x", 50))), 10) + if err == nil || !strings.Contains(err.Error(), "listing output exceeded") { + t.Fatalf("want cap error, got %v", err) + } +} diff --git a/backend/pkg/server/services/flow_files.go b/backend/pkg/server/services/flow_files.go index 7f1a434e..e5b8ef10 100644 --- a/backend/pkg/server/services/flow_files.go +++ b/backend/pkg/server/services/flow_files.go @@ -38,6 +38,11 @@ import ( // resources/ ← user resources copied from user_resources table; also pushed to container /work/resources/ // container/ ← files synced from the container via pull; never sent back to container +// maxContainerListPaths bounds how many directory paths one container-files +// request may list, so the per-path entry cap can't be multiplied into an +// unbounded fan-out or response. +const maxContainerListPaths = 128 + type pendingUpload struct { fileName string dstPath string @@ -940,6 +945,14 @@ func (s *FlowFileService) GetFlowContainerFiles(c *gin.Context) { response.Error(c, response.ErrFlowFilesInvalidRequest, err) return } + // Bound the number of paths per request so the per-path entry cap can't be + // multiplied by an attacker-chosen path count into a huge fan-out / response. + if len(containerPaths) > maxContainerListPaths { + err = fmt.Errorf("too many paths requested (%d, limit %d)", len(containerPaths), maxContainerListPaths) + logger.FromContext(c).WithError(err).WithField("flow_id", flowID).Error("too many container paths") + response.Error(c, response.ErrFlowFilesInvalidRequest, err) + return + } if len(containerPaths) == 0 { containerPaths = []string{docker.WorkFolderPathInContainer} } diff --git a/backend/pkg/server/services/flow_files_partial_test.go b/backend/pkg/server/services/flow_files_partial_test.go index ceba24c6..8578438f 100644 --- a/backend/pkg/server/services/flow_files_partial_test.go +++ b/backend/pkg/server/services/flow_files_partial_test.go @@ -4,6 +4,8 @@ import ( "fmt" "net/http" "os" + "strconv" + "strings" "testing" "pentagi/pkg/docker" @@ -132,3 +134,29 @@ func TestGetFlowContainerFiles_TruncatedFlagSurfaced(t *testing.T) { require.Equal(t, http.StatusOK, *code) assert.True(t, resp.Truncated) } + +// The per-request path count is bounded so the per-path entry cap can't be +// multiplied by an attacker-chosen number of paths. +func TestGetFlowContainerFiles_TooManyPathsRejected(t *testing.T) { + buildQuery := func(n int) string { + parts := make([]string, n) + for i := range parts { + parts[i] = "paths[]=/p" + strconv.Itoa(i) + } + return strings.Join(parts, "&") + } + + // One over the cap → 400 before any docker call. + db := setupFlowFileServiceTestDB(t) + seedFlow(t, db, 1, 1) + svc := NewFlowFileService(db, t.TempDir(), &fakeDockerClient{running: true}, nil) + c, w := newFlowFileTestContext(http.MethodGet, + "/flows/1/files/container?"+buildQuery(maxContainerListPaths+1), nil, + []string{"flow_files.view", "containers.view"}, 1, 1) + svc.GetFlowContainerFiles(c) + require.Equal(t, http.StatusBadRequest, w.Code) + + // Exactly at the cap is allowed. + code, _ := listContainerFiles(t, &fakeDockerClient{running: true}, buildQuery(maxContainerListPaths)) + require.Equal(t, http.StatusOK, *code) +}