Rodrigo Rosenfeld Rosas

Fri, 13 Oct 2017 19:50:00 +0000

The Ruby ecosystem is famous for providing convenient ways of doing things. Very often security concerns are traded for more convenience. That makes me feel out of place because I'm always struggling to change the default route since I'm not interested in trading security with convenience when I have to make a choice.

Since it's Friday 13, let's talk a bit about my fears ;)

I remember that several of the security issues that were disclosed in the past few years in the Ruby community only existed in the first place because of this idea that we should try to deliver features the most convenient way. Like allowing YAML to dump/load Ruby objects, for example, when people were used to use it to serialize/deserialize. Thankfully it seems JSON is more popular these days even if more limited - you can't serialize times or dates, for example, as allowed in YAML.

Here are some episodes I can remember of regarding how convenience was the reason behind many vulnerabilities:

I remember that for a long while I was used to always explicitly convert params to the expected format, like params[:name].to_s and that alone was enough to protect my application from many of the disclosed vulnerabilities. But my application was still vulnerable to the first mentioned in the list above and the worst part is that we never ever used XML or YAML in our controllers but we were affected by that bug in the name of convenience (for others, not us).

Why is this a major issue with Ruby web applications?

Any other web framework providing seamless params binding depending on how the params keys are formatted are vulnerable for the same reasons but most (all?) people doing web development with Ruby these days will rely on Rack::Request somehow. And it will automatically convert your params to array if they are formatted like ?a[]=1&a[]=2 or hashes if they are formatted like ?a[x]=1&a[y]=2. This is built-in and you can't change this behavior for your specific application. I mean, you could replace Rack::Utils.default_query_parser and implement parse_nested_query as parse_query for your own custom parser but then that would apply to other Rack apps mounted in your app (think of Sidekiq web, for example) and you don't know whether or not they're relying on such conveniences.

How to improve things

I've been bothered by the inconvenience of having to add .to_s to all string params (in name of providing more convenience, which is ironic anyway) for many reasons, and wanted a more convenient way of accessing params safely for years. As you can see, what is convenient to some can be inconvenient to others. But that would require a manual inspection in all controllers to review all cases where a param is fetched from the request. I wasn't that much bothered after all, so I thought it wouldn't worth the effort for such a big app.

Recently I noticed Rack recently deprecated Rack::Request#[] and I used it a lot as not only it was more convenient calling request['name'] instead of request.params['name'] but most examples in Roda's README used that convenient #[] method (the examples were updated after it was deprecated). Since eventually I'd have to fix all usage of such method, and once they were used all over the places in our Roda apps (think of controllers - we use the multi_run plugin), I decided to finally take a step further and fix the old problem as well.

Fetching params through an specialized safer class

Since I realized that it wouldn't be possible to make Rack parse queries in a more simpler way, I decided to build a solution that would wrap around Rack parsed params. For a Roda app, like ours, writing a Roda plugin for that makes perfect sense, so this is what I did:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 # apps/plugins/safe_request_params.rb require ' rack/request ' require ' json ' module AppPlugins module SafeRequestParams class Params attr_reader :files , :arrays , :hashes def initialize ( env : nil , request : nil ) request ||= Rack :: Request .new(env) @params = {} @files = {} @arrays = {} @hashes = {} request.params.each do |name, value| case value when String then @params [name] = value when Array then @arrays [name] = value when Hash if value.key? :tempfile @files [name] = UploadedFile .new value else @hashes [name] = value end end # ignore if none of the above end end # a hash representing all string values and their names # pass the keys you're interested at optionally as an array def to_h (keys = nil ) return @params unless keys keys.each_with_object({}) do |k, r| k = to_s k next unless key? k r[k] = self [k] end end # has a string value for that key name? def key? (name) @params .key?(to_s name) end def file? (name) @files .key?(to_s name) end # WARNING: be extra careful to verify the array is in the expected format def array (name) @arrays [to_s name] end # has an array value with that key name? def array? (name) @arrays .key?(to_s name) end # WARNING: be extra careful to verify the hash is in the expected format def hash_value (name) @hashes [to_s name] end # has a hash value with that key name? def hash? (name) @hashes .key?(to_s name) end # returns either a string or nil def [] (name, nil_if_empty : true , strip : true ) value = @params [to_s name] value = value&.strip if strip return value unless nil_if_empty value&.empty? ? nil : value end def file (name) @files [to_s name] end # raises if it can't convert with Integer(value, 10) def int (name, nil_if_empty : true , strip : true ) return nil unless value = self [name, nil_if_empty : nil_if_empty, strip : strip] to_int value end # converts a comma separated list of numbers to an array of Integer # raises if it can't convert with Integer(value, 10) def intlist (name, nil_if_empty : true , strip : nil ) return nil unless value = self [name, nil_if_empty : nil_if_empty, strip : strip] value.split( ' , ' ).map{|v| to_int v } end # converts an array of strings to an array of Integer. The query string is formatted like: # ids[]=1&ids[]=2&... def intarray (name) return nil unless value = array(name) value.map{|v| to_int v } end # WARNING: be extra careful to verify the parsed JSON is in the expected format # raises if JSON is invalid def json (name, nil_if_empty : true ) return nil unless value = self [name, nil_if_empty : nil_if_empty] JSON .parse value end private def to_s (name) Symbol === name ? name.to_s : name end def to_int (value) Integer(value, 10 ) end class UploadedFile ATTRS = [ :tempfile , :filename , :name , :type , :head ] attr_reader * ATTRS def initialize (file) @file = file @tempfile , @filename , @name , @type , @head = file.values_at * ATTRS end def to_h @file end end end module InstanceMethods def params env[ ' app.params ' ] ||= Params .new( request : request) end end end end Roda :: RodaPlugins .register_plugin :app_safe_request_params , AppPlugins :: SafeRequestParams

Here's how it's used in apps (controllers):

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 require_relative ' base ' module Apps class MyApp < Base def process (r) # r is an alias to self.request r.post( ' save ' ){ save } end private def save assert params[ :name ] === params[ ' name ' ] # Suppose a file is passed as the "file_param" assert params[ ' file_param ' ].nil? refute params.file( ' file_param ' ).tempfile.nil? p params.files.map(& :filename ) p params.json( :json_param )[ ' name ' ] p [ params.int( :age ), params.intlist( :ids ) ] assert params[ ' age ' ] == ' 36 ' assert params.int( :age ) == 36 # we don't currently use this in our application, but in case we wanted to take advantage # of the convenient query parsing that will automatically convert params to hashes or arrays: children = params.array ' children ' assert params[ ' children ' ].nil? user = params.hash_value :user name = user[ ' name ' ].to_s # some convenient behavior we appreciate in our application: assert request.params[ ' child_name ' ] == ' ' assert params[ ' child_name ' ].nil? # we call strip on the values and convert to nil if empty end end

An idea for those wanting to expand the safeness of the Params class above to the unsafe methods (json, array, hash_value) one could implement it in such a way that any hashes would be wrapped in a Params instance. However they should probably consider more specialized solutions in those cases, such as dry-validation or surrealist.

Final notes

In web frameworks developed in static languages this isn't often a common reason for vulnerability because it's harder to implement solutions like the one adopted by Rack as one would have to use some generic type such as Object for mappings params keys to their values, which is usually avoided in typed languages. Also, method signatures are often more explicit which prevents an specially crafted param to be interpreted as being of a different type than expected by methods. This is even more true in languages that don't support method overloading, such as Java.

That's one of the reasons I like the idea of introducing optional typing to Ruby, as I once proposed. I do like the flexibility of Ruby and that's one of the reasons why I often preferred script languages over static ones for general purpose programming (I used to do Perl programming in my initial days when developing to the web).

But if Ruby was flexible enough to also allow me to specify optional typing, like Groovy does, it would be even better in my opinion. Until there, even though I'm not an security expert by any means, I feel like the recent changes on how our app fetch params from the request should significantly reduce the possibility of introducing bugs caused by params injection in general.

After all, security is already a quite complex topic to me and I don't even want to have to think about what would be the impact of doing something like MyModel.where(username: params['username']) and have to think what could possibly go wrong if someone would inject some special array or hash in the username param. Security is already hard to get it right. No need to make it even harder by providing automatic params binding through the same method out of the box in the name of convenience.