Rodrigo Rosenfeld Rosas

Explicit request params binding in Ruby web apps (or "convenience can be inconvenient")

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

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

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

Powered by Disqus