Sinatra leaks sensitive information from any exception with an `http_status` attribute #1518

garybernhardt · 2019-01-31T20:43:21Z

This is fundamentally the same issue as #1204. However, I think I have a clearer explanation of what's happening, including an easily-reproduced example. I also think the bug is a serious security problem and want to make that clear.

First, Stripe's errors have an http_status attribute. This attribute represents the status code returned by Stripe's servers. For example, here you can see Stripe::InvalidRequestError taking an http_status argument, which is 400 when the error is actually raised.

  class InvalidRequestError < StripeError
    attr_accessor :param

    def initialize(message, param, http_status: nil, http_body: nil, json_body: nil,
                   http_headers: nil, code: nil)

In our application, we were seeing this error raised for correct reasons (i.e., we had an actual error). But Sinatra will treat any exception with an http_status attribute as a Sinatra-level error that should be translated into that HTTP status code.

    def handle_exception!(boom)
      [...]

      if boom.respond_to? :http_status
        status(boom.http_status)

Note that the Sinatra code above does not respond to any configuration option. As far as I can tell, this problem exists regardless of how Sinatra is configured.

By these two code snippets' powers combined:

Stripe's servers are 400ing because we did something wrong.
That 400 response exception's text contains sensitive Stripe identifiers, like Stripe token IDs.
Sinatra sees the http_status attribute and incorrectly thinks that this exception is actually a Sinatra exception.
Sinatra returns that status code and the response text to the user, including sensitive information from Stripe.

You can reproduce this yourself by installing the "stripe" and "sinatra" gems and running this single-file Sinatra app. Go to localhost:4567/error_test and you'll see the "sensitive information from stripe" string leaked to the end user.

#!/usr/bin/env ruby

require "sinatra"
require "stripe"

get "/error_test" do
  raise Stripe::InvalidRequestError.new(
    "sensitive information from stripe", 2, http_status: 400)
end

The text was updated successfully, but these errors were encountered:

mwpastore · 2019-01-31T21:07:26Z

Add an error handler to your app to modify the response body.

error Stripe::InvalidRequestError do
  "boom"
end

jkowens · 2019-01-31T23:22:11Z

Looking at this, it seems the exception message will be printed to the body if it is http status 400 or 404. Still not good. See

sinatra/lib/sinatra/base.rb

Line 1132 in 26fa5c8

body boom_message || '<h1>Not Found</h1>'

.

I'm wondering why there isn't a Sinatra::Error class with internal errors extending from that. Seems like this is a case where checking type would be better than just checking if it quacks.

garybernhardt · 2019-01-31T23:33:38Z

@jkowens I also reprod it with 401 when I was diagnosing it. Didn't dig any deeper into the exact status codes affected, though.

garybernhardt · 2019-01-31T23:35:08Z

@mwpastore We'll work around it now that we know. But the problem is that I've never heard of this in ten years of using Sinatra. And we only discovered it because we happened to see an unhandled Stripe::InvalidRequestError during development and tracked it down. We could've just as easily found out because it happened in production, leaking sensitive billing data to the client. Like @jkowens, I wonder why this conditional isn't checking for a specific error class, rather than merely checking for the existence of a method.

By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.

jkowens · 2019-02-01T05:07:06Z

It looks like Sinatra has tried to set the http_status of an error as the response status back to version 1. The difference in 2.0 is that if http_status returns 404 or 400, the message of the error is sent as the body of the response by default. Clearly something that should only be reserved for internal Sinatra errors.

garybernhardt · 2019-02-01T19:28:06Z

Edited: Oops, this comment was suggesting what @jkowens' PR #1519 already does. Looks like we're on the same page. Thanks, @jkowens!

namusyaka · 2019-02-03T14:56:53Z

This is an API design issue, not a bug.
Sinatra provides several features by duck-typing according to the Ruby style.
Thus, I don't intend to take a patch from @jkowens right away (but thanks for working on this issue).

By reporting this issue, I could recognize the problem on the API design.
Indeed, we didn't mention it in our document explicitly.

At least, we can take a following action:

Towards 3.0.0, we can discuss the best API design.
Mention the http_status method and its duck-typing feature in our document (probably README?).

garybernhardt · 2019-02-03T19:37:09Z

I don't think that mentioning http_status in the docs solves the problem. Every Sinatra user would have to manually check for an http_status method in every exception class that can be raised by any library in the system.

namusyaka · 2019-02-03T19:44:25Z

@garybernhardt I don't say that mentioning in the document will solve this problem. I said that it's important to clarify the matter until the problem is solved in the future.
Please don't misunderstand

garybernhardt · 2019-02-03T20:11:40Z

@namusyaka Got it, thanks for clarifying and sorry for misunderstanding!

SwagDevOps · 2019-05-13T21:28:01Z

Interesting issue 👍

By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.

By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues #1204 and #1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.

jkowens · 2021-10-06T15:34:30Z

Closed by #1519. Will be released in v3.0.

By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues #1204 and #1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.

jkowens mentioned this issue Feb 1, 2019

Internal Sinatra errors now extend Sinatra::Error #1519

Merged

jkowens added the has attached pr label Aug 22, 2019

jkowens added this to the v3.0.0 milestone Aug 22, 2019

jkowens closed this as completed Oct 6, 2021

dentarg mentioned this issue Dec 16, 2022

Better handling of multipart parsing when request is missing body. rack/rack#1997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sinatra leaks sensitive information from any exception with an `http_status` attribute #1518

Sinatra leaks sensitive information from any exception with an `http_status` attribute #1518

garybernhardt commented Jan 31, 2019

mwpastore commented Jan 31, 2019 •

edited

Loading

jkowens commented Jan 31, 2019

garybernhardt commented Jan 31, 2019

garybernhardt commented Jan 31, 2019

jkowens commented Feb 1, 2019

garybernhardt commented Feb 1, 2019 •

edited

Loading

namusyaka commented Feb 3, 2019

garybernhardt commented Feb 3, 2019

namusyaka commented Feb 3, 2019

garybernhardt commented Feb 3, 2019

SwagDevOps commented May 13, 2019

jkowens commented Oct 6, 2021

Sinatra leaks sensitive information from any exception with an http_status attribute #1518

Sinatra leaks sensitive information from any exception with an http_status attribute #1518

Comments

garybernhardt commented Jan 31, 2019

mwpastore commented Jan 31, 2019 • edited Loading

jkowens commented Jan 31, 2019

garybernhardt commented Jan 31, 2019

garybernhardt commented Jan 31, 2019

jkowens commented Feb 1, 2019

garybernhardt commented Feb 1, 2019 • edited Loading

namusyaka commented Feb 3, 2019

garybernhardt commented Feb 3, 2019

namusyaka commented Feb 3, 2019

garybernhardt commented Feb 3, 2019

SwagDevOps commented May 13, 2019

jkowens commented Oct 6, 2021

Sinatra leaks sensitive information from any exception with an `http_status` attribute #1518

Sinatra leaks sensitive information from any exception with an `http_status` attribute #1518

mwpastore commented Jan 31, 2019 •

edited

Loading

garybernhardt commented Feb 1, 2019 •

edited

Loading