Skip to content

fix: prevent symlink escapes in file path validation - #20

Open
traitimtrongvag wants to merge 2 commits into
DanWBR:masterfrom
traitimtrongvag:fix/symlink-path-validation
Open

fix: prevent symlink escapes in file path validation#20
traitimtrongvag wants to merge 2 commits into
DanWBR:masterfrom
traitimtrongvag:fix/symlink-path-validation

Conversation

@traitimtrongvag

Copy link
Copy Markdown
Contributor

I fixed the FILES path validator so existing symlinks are resolved before blocked-path checks, and non-existing destinations validate their nearest existing parent.

I added Linux regression coverage for symlinked reads and write destinations, plus a normal temporary write.

@DanWBR

DanWBR commented Sep 10, 2026

Copy link
Copy Markdown
Owner

I'm the AI assistant working with Daniel. I built this branch and ran the suite against it, and wrote a throwaway probe for the one thing I wanted to be sure about, so the notes below are executed rather than read off the diff.

The bug is real and worth fixing. Path.GetFullPath normalises .. and separators but does not follow links, so a link inside the browsable tree was a straight route to anything the blocklist names. Good catch.

Status: builds clean, the FileBrowserService tests pass (26), and the full suite shows no new failures.

The escape is only half closed

FileSystemInfo.ResolveLinkTarget resolves the path only when the last component is a link. If the link is an intermediate directory, the leaf is an ordinary file, ResolveLinkTarget returns null, full is left as it was, and IsBlocked never sees the real target.

I probed it with a junction, since Windows lets an unprivileged process create one and ResolveLinkTarget treats junctions and symlinks alike:

asked  : ...\polaris-probe\winlink\win.ini      (winlink -> C:\Windows)
threw  : (nothing)
gave   : ...\polaris-probe\winlink\win.ini
blocked: False

The same probe with the link as the last component is correctly refused, so the branch does work for the case it targets. On Linux the equivalent is a ~/files/etc-link -> /etc and then a read of etc-link/shadow.

The two tests in the PR do not catch this because the intermediate-link case is only exercised on the write path, where ResolveSafeDestination's parent walk happens to canonicalise the existing parent. The read path has no such walk.

What I would ask for: canonicalise the whole path rather than the leaf. A small helper that walks from the root and resolves each segment's link target would cover both paths, and ResolveSafeDestination's hand-rolled walk could then call it instead of half-repeating it.

Smaller things

mustExist: false gets no resolution at all. The new block is inside if (mustExist), so a caller that validates a path it is about to create still receives an unresolved one. ResolveSafeDestination covers the write case it owns, but any other mustExist: false caller does not benefit.

The OS gate silently passes. if (!OperatingSystem.IsLinux()) return; makes the test report success on Windows and macOS, where it ran nothing. Assert.Ignore("reason") says so out loud, and the suite already reports ignored tests.

ResolveSafe_SymlinkToBlockedFile_Refused depends on /etc/shadow existing. Where it does not (some containers), File.Exists(link) is false and the call throws FileNotFoundException instead, so the test fails for the wrong reason. Pointing the link at /etc/ssh (a directory in the blocklist) or at /proc would be sturdier.

Worth a sentence in the code. ResolveSafe now returns the link's target rather than the path that was asked for. That is the right call with a blocklist, and it is a behaviour change for any caller that echoes the path back or compares it to a configured root, which is the kind of thing worth writing down next to the resolution.

Thanks for finding this one. The direction is right; it is the intermediate-component case I would not want to leave open, since it is the easier of the two to arrange.

@traitimtrongvag

Copy link
Copy Markdown
Contributor Author

Thanks for checking this. I’ll fix the intermediate-link case and the other points you mentioned.

Resolve each existing path component before checking the blocklist.

This prevents intermediate symlinks from bypassing path validation and keeps missing trailing components supported for non-existing paths.

Add regression tests for intermediate symlinks and mustExist:false paths.
@traitimtrongvag traitimtrongvag changed the title fix: block symlink escapes in file path validation fix: prevent symlink escapes in file path validation Sep 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.

2 participants