Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add escaping to the static 404 page. #1645

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Add escaping to the static 404 page. #1645

merged 2 commits into from
Oct 6, 2020

Conversation

chrisgavin
Copy link
Contributor

This change escapes exception messages on error pages for "not found" and "bad request" errors when serving static content. In particular the message for 404 errors contains the request method and path which are user controlled and should probably be escaped.

This isn't really that exploitable because the path will be URL encoded when being requested by any sane browser, but it seems better to be safe and escape it anyway.

Example error page before the change:

$ curl "https://sinatra.example.com/a<script>"       
GET /a<script>

Example after:

$ curl "https://sinatra.example.com/a<script>"       
GET &#x2F;a&lt;script&gt;

@dentarg
Copy link
Member

dentarg commented Sep 28, 2020

I can't reproduce, can you show code or add a test?

$ curl -s -v "localhost:9292/a<script>"
*   Trying ::1:9292...
* Connected to localhost (::1) port 9292 (#0)
> GET /a<script> HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.72.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
< Content-Type: text/html; charset=ISO-8859-1
< Server: WEBrick/1.6.0 (Ruby/2.7.1/2020-03-31)
< Date: Mon, 28 Sep 2020 22:26:18 GMT
< Content-Length: 282
< Connection: close
<
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">
<HTML>
  <HEAD><TITLE>Bad Request</TITLE></HEAD>
  <BODY>
    <H1>Bad Request</H1>
    bad URI `/a&lt;script&gt;'.
    <HR>
    <ADDRESS>
     WEBrick/1.6.0 (Ruby/2.7.1/2020-03-31) at
     mort:9292
    </ADDRESS>
  </BODY>
</HTML>
* Closing connection 0

@chrisgavin
Copy link
Contributor Author

Hi @dentarg. Apologies for not including detailed enough reproduction steps. I didn't realize WEBrick would not allow a URI with angle brackets in (probably a sensible decision on their part). I was using Unicorn in my testing, which does allow this URL.

With your example above, if I add unicorn to the Gemfile then run it with unicorn that reproduces the behavior I was seeing:

$ echo "gem \"unicorn\"" >> Gemfile
$ bundle install
...
$ export RACK_ENV=production # To enable the production error pages rather than the development one which already fixed in #883.
$ bundle exec unicorn --port 9292
...

Then the Curl commands should return the expected results.

$ curl "http://localhost:9292/a<script>"
GET /a<script>
$ curl "http://localhost:9292/a<script>"
GET &#x2F;a&lt;script&gt;

I attempted to include a test, but unfortunately the testing framework also considers the URL invalid which makes it a bit tricky to test.

URI::InvalidURIError: bad URI(is not URI?): "/<script>.txt"
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/2.6.0/uri/rfc3986_parser.rb:67:in `split'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/2.6.0/uri/rfc3986_parser.rb:73:in `parse'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/2.6.0/uri/common.rb:234:in `parse'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rack-test-1.1.0/lib/rack/test.rb:214:in `parse_uri'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rack-test-1.1.0/lib/rack/test.rb:127:in `custom_request'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rack-test-1.1.0/lib/rack/test.rb:58:in `get'
    /home/chris/.rbenv/versions/2.6.6/lib/ruby/2.6.0/forwardable.rb:230:in `get'
    /tmp/sinatra/test/static_test.rb:80:in `block in <class:StaticTest>'

Thanks for your quick reply! :)

@jkowens
Copy link
Member

jkowens commented Oct 1, 2020

Thanks, this looks like a good update to me. @namusyaka what do you think?

@jkowens jkowens requested a review from namusyaka October 1, 2020 16:35
namusyaka
namusyaka reviewed Oct 1, 2020
View reviewed changes
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.
In that case the content-type will be text/plain but we should not depend on browsers respecting the content-type header for the security.

However, please try to test this behavior. You'll be able to reproduce the case by the following:

env = Rack::MockRequest.env_for("/dummy").tap { |env| env['PATH_INFO'] ='/<script>' }

@chrisgavin
Copy link
Contributor Author

Thanks @namusyaka, that's a neat trick. I don't think the content type is text/plain unfortunately.

< HTTP/1.1 404 Not Found
< Date: Mon, 05 Oct 2020 13:20:27 GMT
< Connection: close
< X-Cascade: pass
< Content-Type: text/html;charset=utf-8
< Content-Length: 13
< X-XSS-Protection: 1; mode=block
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< 
* Closing connection 0
GET /<script>

namusyaka
namusyaka approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@namusyaka namusyaka merged commit 0aa8d50 into sinatra:master Oct 6, 2020
@namusyaka namusyaka added this to the v2.1.1 milestone Oct 11, 2020
aldentea added a commit to aldentea/padrino-framework that referenced this pull request Jan 7, 2021
@aldentea
Adapt a test to sinatra/sinatra#1645.
97e1513
aldentea added a commit to aldentea/padrino-framework that referenced this pull request Jan 7, 2021
@aldentea
Adapt a test to sinatra/sinatra#1645.
c5448db
@raxoft
Copy link
Contributor

raxoft commented May 20, 2022

Interesting that while you were fixing this you didn't realize that including #1566 was not a good idea in the first place...

@raxoft raxoft mentioned this pull request May 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namusyaka namusyaka namusyaka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1.1
Development

Successfully merging this pull request may close these issues.
5 participants
@chrisgavin @dentarg @jkowens @raxoft @namusyaka