Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a path traversal sanitizer #915

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tballison
Copy link

This adds a FilePathTraversal sanitizer.

Harnesses set directories under which the application may read and write files via an environment variable.

This does not currently handle symlinks.

@oetr
Copy link
Contributor

oetr commented Jan 2, 2025

Awesome work, thanks!

Initially I liked the idea of an allow list for directories. But on a second thought, if the fuzzer is able to access a file that's a few directories up from the current directory, then most likely path traversal is possible. This saves us the complexity of matching the files on the allow list, and the user of providing those, and not forgetting to enable this sanitizer.

The second aspect that is missing in this PR is to guide the fuzzer towards path traversal, which is difficult to achieve with an allow list.

Since you have not opened a branch in this repo, I have added my suggestions in the comments, instead of pushing my changes directly, so apologies for that!

Copy link
Contributor

@oetr oetr left a comment

Choose a reason for hiding this comment

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

Unfortunately, I ran the format script format.sh before committing my changes locally, so now every line is different!

@tballison
Copy link
Author

tballison commented Jan 7, 2025

@oetr thank you so much for your review of this and improvements. I like them quite a bit. I agree that there needs to be a customizable TARGET for those edge cases where two directories up is not a traversal.

More generally, I'm concerned about using a relative path as the TARGET.

Note that the TARGET String is ../../jazzer-traversal -- the call to normalize() isn't removing the relative components (as you may have intended???).

As one issue, let's say the working directory is /a/b/c/d and the fuzzer is able to create an input that has the application writing to ../../jazzer-traversal. If the application calls toAbsolutePath() on that target before, e.g., the call to write(), then there will never be equality here but there will definitely be traversal.

if (path.equals(TARGET)) {

Similar is true if the fuzzer is able to generate an input that has the application writing to an absolute path: /a/b/e/jazzer-traversal.

These two examples might disappear if we change the initialization of TARGET to:

PATH= Paths.get("..", "..", "jazzer-traversal").toAbsolutePath().normalize().toString();

The (less important) problem with that solution then, is that a reproducing blob would only successfully reproduce if Jazzer were running in the same location on the file system in some cases (right?). If I ran Jazzer in Docker in a specific location, and then tried to run it locally in a different working directory, the blob wouldn't reproduce...I think?

@oetr
Copy link
Contributor

oetr commented Jan 13, 2025

Getting the paths right is always more difficult than it looks :)
@tballison Thanks for these great points--now we can make this sanitizer even better!

  • I suggest, we change the default target to ../jazzer-traversal. If someone is fuzzing from the root we might want to disable this sanitizer, unless the user provides a valid target.
  • Then we derive two paths from the default/user-provided target once during initialization as follows:
    • if the target is relative:
      • we normalize the relative path and save it as relativeTarget;
      • and convert it into an absolute path starting from CWD, call it absoluteTarget
    • if the target is absolute:
      • we clean it and try to derive a relative path from CWD to it (it is not always possible to derive such path, in which case this sanitizer should only guite and compare with the absolute target) and save it as relativeTarget;
      • clean it and call it absoluteTarget
  • During fuzzing, we dispatch the comparison based on whether the query path is absolute or relative. And here we should also clean/normalize the query path
  • Guiding is trickier, but I have an idea
    • if the query path we get is relative, we guide it towards relativeTarget
    • otherwise guide towards absoluteTarget

This should work from any location. This will error out if the location cannot be resolved relative to CWD, but then it means that the target is user-set, so the user's responsible for fixing it.

WDYT?

@tballison
Copy link
Author

tballison commented Jan 14, 2025

@oetr K. I think I implemented the above. Let me know what you think.

@oetr
Copy link
Contributor

oetr commented Jan 17, 2025

@tballison Awesome job, thank you!

From my point of view, following points should be addressed before we can release this:

If you like, we can merge this PR "as-is" (after squash+rebase), and I can take over and do the rest. You will certainly get the credit for this bug detector in the release notes 🚀
That would be my preferred option, but If you like to be bossed around, we can spend a few more iteration cycles!
Let me know what you think.

@tballison
Copy link
Author

@oetr go forth! Let me not hinder you any further! 🤣

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