Skip to content

Commit

Permalink
Escape newlines in common logger lines
Browse files Browse the repository at this point in the history
This uses the same approach I used in Rack.  I don't consider this
a security issue, because expoiting it requires a broken server
that is including newlines in things that it shouldn't.  Attempts
to exploit it even in that case leave a clear trail in the log,
since there will be a partial line directly before an attempt to
inject a line.
  • Loading branch information
jeremyevans committed Feb 12, 2025
1 parent 8a3d9b6 commit b3b906c
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 3 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
=== master

* Escape newlines in common logger lines (jeremyevans)

=== 3.89.0 (2025-02-12)

* Support passing keyword arguments to mailer plugin mail/sendmail class methods (jeremyevans)
Expand Down
7 changes: 4 additions & 3 deletions lib/roda/plugins/common_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,15 @@ def _roda_after_90__common_logger(result)

env = @_request.env

line = "#{env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-"} - #{env["REMOTE_USER"] || "-"} [#{Time.now.strftime("%d/%b/%Y:%H:%M:%S %z")}] \"#{env["REQUEST_METHOD"]} #{env["SCRIPT_NAME"]}#{env["PATH_INFO"]}#{"?#{env["QUERY_STRING"]}" if ((qs = env["QUERY_STRING"]) && !qs.empty?)} #{@_request.http_version}\" #{status} #{((length = headers[RodaResponseHeaders::CONTENT_LENGTH]) && (length unless length == '0')) || '-'} #{elapsed_time}\n"
line = "#{env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-"} - #{env["REMOTE_USER"] || "-"} [#{Time.now.strftime("%d/%b/%Y:%H:%M:%S %z")}] \"#{env["REQUEST_METHOD"]} #{env["SCRIPT_NAME"]}#{env["PATH_INFO"]}#{"?#{env["QUERY_STRING"]}" if ((qs = env["QUERY_STRING"]) && !qs.empty?)} #{@_request.http_version}\" #{status} #{((length = headers[RodaResponseHeaders::CONTENT_LENGTH]) && (length unless length == '0')) || '-'} #{elapsed_time} "
if MUTATE_LINE
line.gsub!(/[^[:print:]\n]/){|c| sprintf("\\x%x", c.ord)}
line.gsub!(/[^[:print:]]/){|c| sprintf("\\x%x", c.ord)}
# :nocov:
else
line = line.gsub(/[^[:print:]\n]/){|c| sprintf("\\x%x", c.ord)}
line = line.gsub(/[^[:print:]]/){|c| sprintf("\\x%x", c.ord)}
# :nocov:
end
line[-1] = "\n"
opts[:common_logger_meth].call(line)
end

Expand Down
8 changes: 8 additions & 0 deletions spec/plugin/common_logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ def cl_app(&block)
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\/1.0" 200 1 0.\d\d\d\d\n\z/)

unless ENV["LINT"]
@logger.rewind
@logger.truncate(0)
body("HTTP_VERSION"=>"HTTP\n1.0").must_equal '/'
@logger.rewind
@logger.read.must_match(/\A- - - \[\d\d\/[A-Z][a-z]{2}\/\d\d\d\d:\d\d:\d\d:\d\d [-+]\d\d\d\d\] "GET \/ HTTP\\xa1.0" 200 1 0.\d\d\d\d\n\z/)
end

@logger.rewind
@logger.truncate(0)
body('/b', 'REMOTE_ADDR'=>'1.1.1.2', 'QUERY_STRING'=>'foo=bar', "HTTP_VERSION"=>'HTTP/1.0').must_equal '/b'
Expand Down

0 comments on commit b3b906c

Please sign in to comment.