xml_in is vulnerable to file inclusion if misused in request

[newint33h]

The method xml_in can be used to read system files in XML format instead of the expected user input.

Proof of Concept

Lets suppose we have in Sinatra the following code:

require 'sinatra'
require 'xmlsimple'
require 'json'

post '/echo_xml' do
  parser = XmlSimple.new
  data = parser.xml_in(request.body.read, {})
  JSON.pretty_generate(data)
end

If we do:

➜  curl http://localhost:4567/echo_xml -X POST --data "<a>hello</a>"
"hello"%

And lets suppose that we also have a file in the same directory named "secrets.xml", with the following content:

<password>S3CR3T</password

If we do:

➜  curl http://localhost:4567/echo_xml -X POST --data "secrets.xml"
"S3CR3T"%

It is very easy to forget to validate the input, causing potential vulnerabilities when parsing input received by the clients.

[maik]

The problem is in the initialize function that supports a lot of different types of input:

    if string.is_a?(String)
      if string =~ /<.*?>/m
        @doc = parse(string)
      elsif string == '-'
        @doc = parse($stdin.read)
      else
        filename = find_xml_file(string, @options['searchpath'])

        if @options.has_key?('cache')
          @options['cache'].each { |scheme|
            case(scheme)
            when 'storable'
              content = @@cache.restore_storable(filename)
            when 'mem_share'
              content = @@cache.restore_mem_share(filename)
            when 'mem_copy'
              content = @@cache.restore_mem_copy(filename)
            else
              raise ArgumentError, "Unsupported caching scheme: <#{scheme}>."
            end
            return content if content
          }
        end

        @doc = load_xml_file(filename)
      end
    elsif string.respond_to?(:read)
      @doc = parse(string.read)
    else
      raise ArgumentError, "Could not parse object of type: <#{string.class}>."
    end

For example, you can pass an XML document as a string but you can also pass the name of an XML file. xml_in is happy with both of them.

This is purely for convenience and I totally agree: it might become a problem under certain circumstances. At least the behavior is documented and checking user input is always important.

I do not see an optimal way to fix this without breaking backwards compatibility. I could, for example, add functions such as xml_in_from_file and xml_in_from_string, but they would only help, if I'd remove xml_in.

[newint33h]

Adding functions for specific input types would help a lot to avoid an extra validation in the application. It is also important to mention that the validation must include the regular expression /<.*?>/m (used in the gem to check it the input is XML), otherwise it could be possible workarounds to the validation. Adding a warning in the docs about the importance of input validation and the deprecation of the xml_in method in a future could be a first step to a mayor version upgrade of the gem.