fix: close prefix-confusion and symlink bypasses in path allowlist - #104
Open
zcaceres wants to merge 1 commit into
Open
fix: close prefix-confusion and symlink bypasses in path allowlist#104zcaceres wants to merge 1 commit into
zcaceres wants to merge 1 commit into
Conversation
isWithinDirectory used startsWith, so `/srv/share-evil` passed a
`startsWith('/srv/share')` check. Rewrite using path.relative and reject
results starting with `..` or that are absolute (Windows cross-drive).
assertPathAllowed resolved with path.resolve+normalize but never followed
symlinks. An attacker who can write into the allowed directory (the
intended use case for MD_SHARE_DIR) could plant a symlink to ~/.ssh/id_rsa
and exfiltrate via an LLM tool call. Realpath both the input and the
allowed dirs before the containment check; walk up parents when the leaf
doesn't exist yet so dir-level symlinks are still caught.
Adds regression tests for sibling-prefix rejection, symlink escape, and
the realpath-aware happy path (also fixes macOS /tmp -> /private/tmp).
Refs #99.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes two real bypasses in the
MD_ALLOWED_PATHS/MD_SHARE_DIRpath allowlist:isWithinDirectory.src/utils.tsusednormPath.startsWith(normDir), so'/srv/share-evil/secret.txt'.startsWith('/srv/share')returnedtrue. A sibling directory whose name shares a prefix with the allowed dir bypassed the check.assertPathAllowed.path.resolve+path.normalizedon't follow symlinks, so an attacker who can write into the allowed directory (the intended use case forMD_SHARE_DIR) could plant a symlink to~/.ssh/id_rsa,~/.aws/credentials, etc., and exfiltrate via an LLM-triggered tool call.Both bugs only affect operators who set
MD_ALLOWED_PATHS/MD_SHARE_DIR— exactly the audience trusting the allowlist for safety. Default installs (no env var = no allowlist) are unaffected.Context from the original report: #99. The PR's headline claim ("filePath is passed unchecked to markitdown") was already addressed by the existing
assertPathAllowedcall inMarkdownify.toMarkdown, but the two sub-bugs above were left open. This PR closes them with the minimum surface area.Changes
src/utils.tsisWithinDirectoryto usepath.relativeand reject results that start with..or are absolute (the absolute case handles Windows cross-drive comparisons).realpathOrAncestorhelper that resolves symlinks on the longest existing ancestor of a path and re-joins any non-existent suffix.assertPathAllowednow realpath-resolves both the input path and the allowed directories before the containment check.src/utils.test.tsisWithinDirectory.assertPathAllowedtests: sibling-prefix rejection, symlink escape, and a realpath-aware happy path (also covers macOS/tmp→/private/tmp).No new dependencies, no new env vars, no behavior change for users without the allowlist set.
Test plan
bun test src/utils.test.ts— 95 pass, 0 failbun run build— clean compileMD_ALLOWED_PATHSset:/tmp/allowed-evil/x) → rejected/etc/hosts→ rejected