Skip to content

In ServeDir, change the behaviour for handling symlinks#658

Open
alexanderkjall wants to merge 2 commits intotower-rs:mainfrom
alexanderkjall:change-symlink-behaviour
Open

In ServeDir, change the behaviour for handling symlinks#658
alexanderkjall wants to merge 2 commits intotower-rs:mainfrom
alexanderkjall:change-symlink-behaviour

Conversation

@alexanderkjall
Copy link
Copy Markdown
Contributor

that point outside the base_dir to return errors, and introduce a new setting that will allow it (on linux)

Motivation

Let me preface this with that I consider this a potential security problem, but I emailed security@tokio.rs and they didn't agree and encouraged me to open a public issue.

Now on to the issue:

ServeDir in tower-http doesn't have any configuration to control symlink behaviour. ( https://docs.rs/tower-http/latest/tower_http/services/struct.ServeDir.html ).

This means that it always follows symlinks.

Attack scenario:

A local user have write permissions to the directory that files are served from, and the webserver process have access to secrets that the user shouldn't be able to read, for example the https certificate with the private key.

Then the user can create a symlink in the directory that is served and exfiltrate those secrets.

Solution

This solution is not really ready to be merged, as it's implemented as a linux only solution for now, but maybe this can act as a starting point for a discussion on how it should be solved?

I have implemented this using the openat2 syscall, as that is as far as I know the only/best way to avoid a TOCTOU (https://cwe.mitre.org/data/definitions/367.html) race condition. This makes the solution linux only, and would need to be expanded to other platforms, but I don't own to any computers running macos/windows/freebsd.

The new open function I introduced canonicalize_and_openat2 is also not async, maybe a good way forward would be for tokio::fs::File to gain an async open_at() function?

I'm also a bit unsure on what to do if ServeFile points to a symlink, happy to change that behavior.

…tside the base_dir to return errors, and introduce a new setting that will allow it (on linux)
},
fallback: None,
call_fallback_on_method_not_allowed: false,
follow_symlinks_outside_base: false,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should default to true. And relevant docs should mention the difference only if enabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for the quick review, I guess I was a bit too paranoid. I have flipped this and tried to improve the docs.

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