Remote Addresses automatically including Proxies?

UXabre · November 9, 2018, 9:00am

In the current documentation it is stated that the remote_addr setting needs to be configured to hold extra addresses from which a call can be made, this can also be observed in the code:

github.com

theforeman/foreman/blob/develop/app/controllers/unattended_controller.rb#L218




  # @host has been changed even if the save fails, so we have to change it back
  old_ip = @host.ip
  @host.ip = old_ip unless @host.update({'ip' => ip})
end


def ip_from_request_env
  ip = request.env['REMOTE_ADDR']


  # Check if someone is asking on behalf of another system (load balancer etc)
  if request.env['HTTP_X_FORWARDED_FOR'].present? && (ip =~ Regexp.new(Setting[:remote_addr]))
    ip = request.env['HTTP_X_FORWARDED_FOR']
  end


  ip
end


def safe_render(template)
  render :plain => template.render(host: @host, params: params).html_safe
rescue StandardError => error
  msg = _("There was an error rendering the %s template: ") % template.name

However, would it not help also check against the registered proxies list? Perhaps just the ones that have the template plugin?

Implementation wise it seems easy, but perhaps it’s too easy and this induces a (big) security hole?

Pros

Configuration of tokenless provisioning is made easier

Cons

Less control of which proxies to allow

lzap · November 12, 2018, 10:41am

Hello,

when it comes to security, it’s not bad to require to be explicit. I think this is the case. However we are open to ideas how to improve the user experience, I understand this can be challenging to figure out why it does not work.

ekohl · November 12, 2018, 12:33pm

I agree with @UXabre and think we should really refactor this because it’s insecure. Normally you don’t trust these HTTP headers unless the come from a known (configured) reverse proxy. IMHO the proper way is to use ActionDispatch::Remoteip and configuring trusted proxies. Then we rely on the remote ip in every controller.

UXabre · November 20, 2018, 8:39am

Would it make sense to add this as a general setting instead? This way, people can opt to be more explicit by disabling the above-mentioned functionality.

I believe this would at least get us out of situation where a user puts in an “unsafe” regex, like (.*) just so it would work;

with the above-mentioned solution it would at least work out-of-the-box with a safe default; drastically decreasing the need to change the regex to something potentially unsafe.