Repository navigation
crowdsec: enforce strict path containment in diagnostics handlers - #1543
AdamMagued wants to merge 1 commit into
Conversation
Wikid82
left a comment
There was a problem hiding this comment.
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.
|
Thanks for picking this up, @AdamMagued. So that it lands with the other CrowdSec fixes, I've carried it forward in #1563 (rebased onto |
Resolves #1541
Uses strict relative path inspection to prevent sibling directories from matching CrowdSec data directory containment checks.