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

Host Header Attack: url_for() should not trust unvalidated Host header #1855

Open
Kludex opened this issue Sep 10, 2022 Discussed in #1854 · 4 comments
Open

Host Header Attack: url_for() should not trust unvalidated Host header #1855

Kludex opened this issue Sep 10, 2022 Discussed in #1854 · 4 comments
Labels
need confirmation This issue needs confirmation.

Comments

@Kludex
Copy link
Sponsor Member

Kludex commented Sep 10, 2022

Discussed in #1854

Originally posted by lieryan September 9, 2022
This is somewhat related to ticket #843.

Currently, url_for() can be used to generate an absolute URL for a route and it uses Host header to do so. There is currently no validation in the value of the host header, which can be abused for some quite funky stuffs.

This behavior is a security issue because it opens up the application to Host Header Attack.

You can fix this issue in your application by adding a TrustedHostMiddleware and setting allowed_hosts=["yourdomain.com"], but I think most people aren't going to know that they must use TrustedHostMiddleware if they want to use url_for() securely.

To reproduce:

from starlette.applications import Starlette
from starlette.responses import RedirectResponse, JSONResponse
from starlette.routing import Route


async def main(request):
    return JSONResponse({

        "url_for": request.url_for("main"),
        "url_path_for": app.url_path_for("main"),
    })


async def redirect(request):
    return RedirectResponse(request.url_for("main"))


routes = [
    Route("/", endpoint=main),
    Route("/redirect", endpoint=redirect),
]

app = Starlette(routes=routes)


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

And on the shell do (note the funky looking Host header):

$ curl -L -v -H 'Host: www.google.com/search' localhost:8000/redirect
< HTTP/1.1 307 Temporary Redirect
< date: Fri, 09 Sep 2022 13:51:10 GMT
< server: uvicorn
< content-length: 0
< location: http://www.google.com/search/


$ $ curl -L -H 'Host: www.google.com/search' localhost:8000/ | jq
{
  "url_for": "http://www.google.com/search/",
  "url_path_for": "/"
}

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Kludex Kludex added the need confirmation This issue needs confirmation. label Sep 10, 2022
@wrzasa
Copy link

wrzasa commented Sep 12, 2022

Here's a so far unanswered discussion topic that points another issue coming (I think) from the same problem: taking directly raw headers to generate url/redirect etc, without any chance to put some validation logic in-between: #1835

@csrgxtu
Copy link

csrgxtu commented Sep 16, 2022

thus TrustedHostMiddleware should be required for each application to disable Host Header Attack.

@alex-oleshkevich
Copy link
Member

What was the idea behind making a full URL instead of only path part?

@lieryan
Copy link

lieryan commented Oct 27, 2022

@alex-oleshkevich sometimes you're rendering absolute url so it can be inserted into Emails/SMS.

For API, rendering absolute URL can make it easier for clients to cache the result, either in full or just parts of the response, without requiring them to cache the base context URL or trying to resolve the relative URLs.

This might not be that big of a problem if you only have one API domain, but clients that calls cross domain requests to many different APIs, and where the API often cross references other APIs as well, taking care of which relative URL belongs to which base URL is an extra unnecessary complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need confirmation This issue needs confirmation.
Projects
None yet
Development

No branches or pull requests

5 participants