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

Revert "Allow staticfiles to follow symlinks outside directory" #1681

Merged
merged 1 commit into from Jun 10, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Jun 10, 2022

@Kludex Kludex requested a review from tomchristie Jun 10, 2022
@Kludex Kludex merged commit edeba9c into master Jun 10, 2022
4 checks passed
@Kludex Kludex deleted the revert-1377-fix-staticfiles-follow-symlinks branch Jun 10, 2022
@Kludex
Copy link
Sponsor Member Author

Kludex commented Jun 10, 2022

Ok. The incident has been resolved. It's time to explain what happened.

Description

Last night, we received a message on Gitter asking for a secure communication channel to report a security issue (much appreciated btw). I sent a message to that person, with @tomchristie in cc. Promptly, they replied with the following message:

image

There was also an application to reproduce the issue attached:

import uvicorn
from starlette.applications import Starlette
from starlette.routing import Mount
from starlette.staticfiles import StaticFiles

app = Starlette(
    routes=[
        Mount('/static', app=StaticFiles(directory='static')),
    ]
)

if __name__ == '__main__':
    uvicorn.run('main:app')

As mentioned, the problem was that we were allowing any symlink outside the specified directory from StaticFiles to be exposed. You can see the snippet that introduced it below.

try:
stat_result = os.lstat(original_path)
full_path.relative_to(directory)
return full_path, stat_result
except ValueError:
# Allow clients to break out of the static files directory
# if following symlinks.
if stat.S_ISLNK(stat_result.st_mode):
stat_result = os.lstat(full_path)
return full_path, stat_result

How this happened? How to avoid it in the future?

There were three reviewers on the PR that introduced the security issue, and two approvals (mine included). Even with that, we failed to see the issue.

As a self evaluation, and retrospective, I think this incident could have been avoided if we either had paid more attention, or maybe don't being overconfident about our knowledge on the pathlib module, or the same for the syscalls involved. In any case, we interpreted the logic flow wrong.

What we did to solve the issue?

As soon as the report was received, we reverted the PR, created a new release, and "yanked" the previous one on PyPI.

The version 0.20.2 was live for less than three days. It's also good to mention that none of the FastAPI users were affected.

Lessons learned

  • We need a security report policy, to allow a secure channel to report issues.
  • Being too cautious and push back PRs is not a problem.

Notes:

  • I didn't expose the person who reported the issue, because I didn't ask, but I'd be more than happy to give merits if they allow me to do so! :)

EDIT: As @m1ckey said some words below, I guess I can now give the merits to him. Thanks so much @m1ckey! ❤️ 🏆

@tiangolo
Copy link
Sponsor Member

tiangolo commented Jun 10, 2022

Thanks a lot for doing this, the quick response, and the full report!

@m1ckey
Copy link

m1ckey commented Jun 10, 2022

Thank you for the professional handling.
I am happy that I could help to improve Starlette. 🌟

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.

None yet

4 participants