Don't use file separator for splitting path info. #2302
Conversation
|
@akostadinov this is a first step towards resolving the issues you raised in #2257. Do you mind reviewing this change? |
|
I think this is good. I'm not sure why we used alt separator. Does this mean URI cleaning was OS dependent before this PR? |
In principle, yes. In practice, it's unlikely people were deploying servers on Mac System 9 or Windows - not sure if there are other major situations where this could apply. |
|
I don't think this is a good idea. I believe it may it introduce a security vulnerability on Windows (where ALT_SEPARATOR is Consider: Rack::Utils.clean_path_info("/foo/..\\../bar")
# Before: "/bar"
# After: "/foo/..\\../bar"If Rack::Static/Rack::Files allows paths starting with File.read(".gem/specs/..\\../.irb_history").length
# => 1736 # for me |
|
Should I think we should be clear that the input "path_info" is a URI path, not a file path. Perhaps if there are path segments that contain semantic characters, we can raise an error (e.g. |
98930bb to
2861484
Compare
|
For reference, here is how Rails uses By the time Specifically: |
It's more work to have it raise an error. More work generally results in more complexity, and more complexity generally makes it more likely that there will be bugs/security issues. As long as it is handled securely, I don't see the reason to go out of the way to complain about it.
There are a lot of things that may or may not work depending on the OS. Even for the same OS, some file systems may be case sensitive and some not.
Considering how Rack itself is using it (only for Files/Static), even if the path given is a URI path, it will be treated as a file path. While we could raise an error, I'm not sure we want to, for reasons described above.
From the sections of the RFC you pointed out to me, my interpretation the RFC states where you must use unencoded characters, which are only a few places (e.g. separating scheme/domain and domain/path). It places no restrictions on where encoded characters can be. As I mentioned earlier, Nginx will treat Now, if you were returning the path split into segments, I think based on the BNF you would be correct to split it only on slashes. But that's not exactly what |
|
I was surprised to see this: |
|
Interesting, PR to change the Nginx behaviour: nginx/nginx#569
|
|
@ioquatix , these changes make a lot of sense to me, thank you for looking into this! The changes map what is present in the RFC and although I can't claim full understanding of intended usage, I see this as a big net improvement of making this method more predictable. There are still a couple of important questions in #2257 wrt nginx/nginx#569, I'm in favor of making invalid a percent encoded It may cause issues though if somebody is using path parameters with such characters not to represent file system paths - for example having some name in the path or Base64 representation of it. I've seen this in a project I've worked on. So it's fine to leave them be. @jeremyevans , I while technically true, that more work results in more opportunities for errors, the goal is to make Rack more helpful in voiding security issues on the app level. Especially in case Rack supports non-standard processing that can cause discrepancies between what it serves and what a reverse proxy expects or what an app expects. Presently allowing all these different path separators is ripe for cache poisoning and path traversal attacks. I think the point of a middleware software like Rack is to help the developers do the right thing and not fall into known traps. So it is reasonable for me to spend some more effort here. |
It is only by coincidence that the
::File::SEPARATORis the same as the URI path separator, but semantically, they are unrelated. Ensure that only"/"is used to separate path segments, as per the RFC: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3Note: This does not change the interpretation of
path_infowhich may be unescaped prior to being an argument of this method - as such, it does not change the interpretation of%2Fcharacters which will be a follow up discussion/PR.