Add support for DNS rebinding protections #861
Conversation
|
Why not passing those values to uvicorn instead of creating a new class, and a whole flow for it? Uvicorn does support the proposed settings. |
Can you point me to where this is supported? I looked around and can't find anything where uvicorn supports checks like this. It is also worth noting that the Origin checking is a bit specialized and is different from CORS checks since we need to actually reject requests with non-matching origin headers (whereas your average CORS middleware doesn't reject requests, it just doesn't set CORS response headers [example]). |
|
Isn't what you want the You can check the implementation here: https://github.com/encode/uvicorn/blob/master/uvicorn/middleware/proxy_headers.py The |
|
No, sadly that doesn't do what we want. DNS rebinding risks don't have anything to do with proxies or the |
|
Ah, this problem is for: "local MCP servers using the SSE transport." Since the "problem" is also present when using uvicorn standalone, I'd say this adds unnecessary complexity to the project and that the official recommendation should be to not run it locally, or to run behind a well tested reverse proxy. There are too many features already implemented in nginx that it will just a lot of complexity if this project implements it. Are the other SDKs implementing this? |
AFAIU, "I run locally" is the main use case of MCP according to the architecture diagram, though.
If the application is ran locally, spawned by some local MCP host it is typically not going to be run behind a reverse proxy.
(On this topic, I think it might be helpful for application web servers like uvicorn or for web framework like starlette to provide some built-in or blessed option for providing DNS-rebinding protection.) Note that even if DNS rebinding protection is used, the service can still be attacked by other local users. This might be important on multi-user systems for example. A better solution is to use the stdio transport or to serve the HTTP transport over Unix Domain Socket. If the service is hosted on a server machine, a localhost-bound MCP server without proper authentication could be attacked by a compromised local process. In both cases, it might be safer to use UDS-bound services if possible. |
|
I agree with @randomstuff.
(Given the nature of this attack, I don't think there's a compelling case to add it on Uvicorn - also, in 8 years of its existence no one ever requested it) [I maintain both Uvicorn and Starlette]. |
|
(Slightly out topic)
I suspect DNS-rebinding vulnerability is still highly prevalent for embedded and localhost-bound services (including development environment). No developer ever adds DNS-rebinding protection logic in their application in order to make sure their local machine cannot be attacked during development for example. This is by large mitigated by the fact that Chromium nowadays includes some DNS-rebinding protection and because virtually anyone uses Chromium-based browsers anyway. For the rest of us still not using Chromium-based browsers, this is still a potential attack vector AFAIU (apparently largely under-exploited in practice), often yielding to high values assets/high impact (cryptocurrency, RCE, etc.). Having some blessed recommendation on how to avoid this attack in major frameworks and servers might help mitigate this issue. A |
| return Response("Invalid Content-Type header", status_code=400) | ||
|
|
||
| # Skip remaining validation if DNS rebinding protection is disabled | ||
| if not self.settings.enable_dns_rebinding_protection: |
There was a problem hiding this comment.
It might make sense to reject the request for DNS rebinding protection before other checks.
There was a problem hiding this comment.
Looking at this code, I think it is in a reasonable order. I don't see any weaknesses exposed by doing it in this order. Do you?
There was a problem hiding this comment.
I don't believe there is any significant difference in term of security.
It is just that logically speaking it would make sense to always return 421 when the Host is incorrect, for any request, even if the request path does no match any route for example (http::/unxpectedhost/unspectedpath should return 421 instead of 404). (Somewhat nitpicky)
(What could be event better in term of DNS-rebinding protection would be to drop the connection altogether when the host is invalid. I guess that doing so DNS-rebinding could not even be used for fingerprinting (think something like the Facebook localhost scanning thing). I think some reverse proxies may support that when returning some special status code but I don't think it is possible with portable ASGI.)
I believe so. The spec has been updated to recommend this so I believe other SDKs should also offer this as an opt-in protection.
+1, I do see DNS rebinding as something that is often missed since it mainly impacts local development machines, but as something that is still quite impactful. So to me, it does seem like a reasonable thing for frameworks to implement since I always see it as better for a framework to provide security, rather than developers having to implement it for individual services.
While it is true that Chromium provides some mitigations (as do some DNS providers), it is worth emphasizing these are pretty incomplete and have known bypasses. So personally I wouldn't recommend ever relying solely on those mitigations for anything critical. |
There was a problem hiding this comment.
I believe this is all needed based on the conversations in this PR and my understanding of the vector.
It's only relevant to running sse/shttp on local host, which probably (never say never) only happens during development. iiuc this isn't the default behaviour so it's depending on the dev opting in, which feels like it wont happen very often. I wonder if we're really concerned about this vuln we should make it the default when running on localhost or 127.0.0.1?
Either way this is a step toward mitigation
|
I think making it the default is probably the right answer in the long term, but that is technically a breaking change so I would rather do that in a separate PR. I also suspect that if we want to do that, we should probably try to coordinate that across all the different SDKs at the same-ish time to unify communications. |
86bb54c
into
modelcontextprotocol:main
Motivation and Context
This implements the mitigations described here. To avoid breaking existing applications this doesn't enable any changes by-default, but enabling this feature is heavily encouraged for any local MCP servers using the SSE transport.
How Has This Been Tested?
Tested via unit tests.
Breaking Changes
To avoid introducing any breaking changes, the DNS rebinding protections are disabled by default. Ideally we should find a way to enable them by default. But for now, adding them as disabled is a good first step.
Types of changes
Checklist
Additional context