-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
unescape path on commonlogger #655
Conversation
This needs proper security review. |
ping @spastorino @raggi |
ping |
for the common logger case, this is probably ok on the rack side. it does run the risk of opening vectors for bad log viewers, which is not a major concern for us, but it is a major change. it's unfortunate that SPEC says "may" for PATH_INFO, which leaves us in a bad state of not knowing if this is correct or not. we should consider a SPEC update and a major version change. what servers does this appear on? |
The problem happens on unicorn and puma .. I guess is because they enable the |
marking for 1.6, as it's not too much of a major change, it won't break apps, but i don't want it in a minor patch level. |
@@ -47,7 +47,7 @@ def log(env, status, header, began_at) | |||
env["REMOTE_USER"] || "-", | |||
now.strftime("%d/%b/%Y:%H:%M:%S %z"), | |||
env["REQUEST_METHOD"], | |||
env["PATH_INFO"], | |||
Utils.unescape(env["PATH_INFO"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should catch encoding exceptions here, otherwise this could be abused to prevent evil requests from reaching the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something like this :
(Utils.unescape(s = env["PATH_INFO"]) rescue s),
Kind of what we do here:
https://github.com/arthurnn/rack/blob/unescape_logger/lib/rack/request.rb#L311
I dont love the rescue
all, but I guess that what we need in here to ensure the behaviour.
cc @tenderlove |
Will this allow injecting ANSI codes? On Wed, Aug 27, 2014 at 4:42 PM, Arthur Nogueira Neves <
|
Unescaping the path probably would allow injecting ANSI codes. Let me ping some security folks about this first. I'm extremely weary of unescaping stuff before putting it in the logs as it could lead to security issues. |
What escaped the |
This is a bug in ActionDispatch::Static. It's wrongly escaping PATH_INFO. Reopened rails/rails#11816 Think this should be closed. |
@jeremy ( ͡° ͜ʖ ͡°) |
A dark & winding path... this traces its roots back to #265. The Rails bug was introduced in rails/rails@d07b2f3 as a work-around for If Rack didn't unescape legit pchar as query components we wouldn't be here. See also TL;DR #265 is still a major bug and needs to be corrected. |
Problem
Rack::File
unescape the file path, https://github.com/rack/rack/blob/master/lib/rack/file.rb#L41 , So the middleware that is setting that up is suppose to escape it, for instance on rails https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/static.rb#L24 .The problem is, that the logger, doesn't unescape, and end up showing this log:
Solution
call unescape before logging it.
[ref rails/rails#11816]