unescape path on commonlogger #655

Closed
wants to merge 1 commit into from

6 participants

@arthurnn

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:

"GET %2Ffavicon.ico HTTP/1.1"

Solution

call unescape before logging it.

[ref rails/rails#11816]

@arthurnn arthurnn unescape path on commonlogger a8132f4
@chneukirchen
Official Rack repositories member

This needs proper security review.

@arthurnn

ping

@raggi
Official Rack repositories member

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?

@arthurnn

The problem happens on unicorn and puma .. I guess is because they enable the CommonLogger https://github.com/defunkt/unicorn/blob/master/lib/unicorn.rb#L67.
We might be able to simulate that on other servers if enabling CommonLogger

@raggi raggi added this to the Rack 1.6 milestone Jul 6, 2014
@raggi
Official Rack repositories member

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.

@raggi raggi commented on the diff Aug 3, 2014
lib/rack/commonlogger.rb
@@ -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"]),
@raggi
Official Rack repositories member
raggi added a note Aug 3, 2014

We should catch encoding exceptions here, otherwise this could be abused to prevent evil requests from reaching the logs.

@arthurnn
arthurnn added a note Aug 5, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh
Official Rack repositories member
@tenderlove
Official Rack repositories member

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.

@jeremy
Official Rack repositories member

What escaped the / -> %2F in the first place? Makes sense to unescape path segments, but why were / escaped on the way in? Those are valid path chars.

@jeremy
Official Rack repositories member

This is a bug in ActionDispatch::Static. It's wrongly escaping PATH_INFO. Reopened rails/rails#11816

Think this should be closed.

@tenderlove
Official Rack repositories member

@jeremy ( ͡° ͜ʖ ͡°)

@tenderlove tenderlove closed this Aug 27, 2014
@arthurnn arthurnn deleted the arthurnn:unescape_logger branch Aug 27, 2014
@jeremy
Official Rack repositories member

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 Rack::File's incorrect PATH_INFO unescaping! :trollface: all the way down.

If Rack didn't unescape legit pchar as query components we wouldn't be here. See also Utils.escape vs Utils.escape_path.

TL;DR #265 is still a major bug and needs to be corrected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment