Skip to content

crowdsec: enforce strict path containment in diagnostics handlers - #1543

Closed
AdamMagued wants to merge 1 commit into
Wikid82:mainfrom
AdamMagued:chore/crowdsec-diagnostics-path-containment
Closed

AdamMagued wants to merge 1 commit into
Wikid82:mainfrom
AdamMagued:chore/crowdsec-diagnostics-path-containment

Conversation

@AdamMagued

Copy link
Copy Markdown
Contributor

Resolves #1541

Uses strict relative path inspection to prevent sibling directories from matching CrowdSec data directory containment checks.

@Wikid82 Wikid82 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, and for the thorough helper tests. The filepath.Rel approach is the right idea for #1541. A few things need adjusting before this can merge, mostly because development has moved a lot since this was branched (everything below refers to development after #1542 merged):

1. Please retarget to development. PRs go into development (this one is based on main, which is well behind), so CI also hasn't run in full. You can change the base in the PR header and rebase onto development.

2. The ReadFile and WriteFile edits are no longer needed. Those endpoints were rewritten and moved to crowdsec_files.go, and they already do containment correctly (including symlink handling). Please drop those two hunks and their sibling-directory tests.

3. Please reuse the existing pathWithin(root, p) helper (backend/internal/api/handlers/crowdsec_files.go) instead of adding isPathWithinDir. It does the same job, so we avoid keeping two copies of the same logic. The two remaining strings.HasPrefix checks in crowdsec_handler.go are in DiagnosticsConfig, around cleanConfigPath (~L2194) and cleanAcquisPath (~L2240).

4. If you do keep a helper of your own, one edge case: strings.HasPrefix(rel, "..") also rejects legitimate names that merely start with two dots, such as ..foo. The check should be rel == ".." or a ".." + string(filepath.Separator) prefix (this is what pathWithin does).

5. Tests: TestCrowdsecHandler_DiagnosticsConfig_SubdirectoryLayout asserts config_path and acquis_path in the response. Those fields were removed from the diagnostics response in #1542, so that assertion needs to go. A test that a sibling directory such as <dataDir>_sibling/acquis.yaml is rejected by DiagnosticsConfig would be a good one to keep.

Happy to help with the rebase if anything is unclear.

@Wikid82

Wikid82 commented Oct 11, 2026

Copy link
Copy Markdown
Owner

Thanks for picking this up, @AdamMagued. So that it lands with the other CrowdSec fixes, I've carried it forward in #1563 (rebased onto development, reusing the existing pathWithin helper, with your original commit credited as co-author). The ReadFile/WriteFile hunks were already outdated on development. I'll close this PR once #1563 merges; no further work needed here.

@Wikid82 Wikid82 closed this Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: use filepath.Rel for the CrowdSec diagnostics path containment checks

2 participants