[PATCH/foreman 1/1] refs #1355 - Data Leak in Reports and Hosts pages

Development
1

This patch wraps an authentication check around:
Viewing a Host page directly (i.e going to the URL)
Editing a Host directly (via URL)
Listing reports only for the hosts in your filter
Viewing a report directly (via URL)

There are almost certainly other, smaller leaks, via
direct URLs or via the API. This will need to be checked

Other leaks include the Fact list needing to be filtered, the Dashboard,
and possibly the pie charts. This is considerably less important :slight_smile:

Signed-off-by: Greg Sutcliffe <gsutcliffe@ibahn.com>

··· --- app/controllers/hosts_controller.rb | 4 ++++ app/controllers/reports_controller.rb | 6 ++++-- app/models/report.rb | 9 +++++++++ 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/app/controllers/hosts_controller.rb b/app/controllers/hosts_controller.rb
index dbac446…1b6a874 100644
— a/app/controllers/hosts_controller.rb
+++ b/app/controllers/hosts_controller.rb
@@ -54,6 +54,8 @@ class HostsController < ApplicationController
end

def show

  • auth = User.current.admin? ? true : Host.my_hosts.include?(@host)
  • not_found and return unless auth
    respond_to do |format|
    format.html {
    # filter graph time range
    @@ -99,6 +101,8 @@ class HostsController < ApplicationController
    end

def edit

  • auth = User.current.admin? ? true : Host.my_hosts.include?(@host)
  • not_found and return unless auth
    load_vars_for_ajax
    end

diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb
index d697b8f…828d2a6 100644
— a/app/controllers/reports_controller.rb
+++ b/app/controllers/reports_controller.rb
@@ -12,7 +12,8 @@ class ReportsController < ApplicationController
filter_parameter_logging :report

def index

  • values = Report.search_for(params[:search], :order => params[:order])

  • my_reports = User.current.admin? ? Report : Report.my_reports

  • values = my_reports.search_for(params[:search], :order => params[:order])
    pagination_opts = { :page => params[:page], :per_page => params[:per_page] }
    respond_to do |format|
    format.html { @reports = values.paginate(pagination_opts.merge({ :include => :host })) }
    @@ -32,7 +33,8 @@ class ReportsController < ApplicationController

    return not_found if params[:id].blank?

  • @report = Report.find(params[:id], :include => { :logs => [:message, :source] })

  • my_reports = User.current.admin? ? Report : Report.my_reports

  • @report = my_reports.find(params[:id], :include => { :logs => [:message, :source] })
    respond_to do |format|
    format.html { @offset = @report.reported_at - @report.created_at }
    format.json { render :json => @report }
    diff --git a/app/models/report.rb b/app/models/report.rb
    index 6c8d9a6…780109a 100644
    — a/app/models/report.rb
    +++ b/app/models/report.rb
    @@ -21,6 +21,15 @@ class Report < ActiveRecord::Base
    scoped_search :on => :status, :offset => METRIC.index(“failed_restarts”), :word_size => BIT_NUM, :rename => :failed_restarts
    scoped_search :on => :status, :offset => METRIC.index(“skipped”), :word_size => BIT_NUM, :rename => :skipped

  • named_scope :my_reports, lambda {

  • user = User.current

  • conditions = sanitize_sql_for_conditions([" (reports.host_id in (?))",Host.my_hosts.map(&:id)])

  • conditions.sub!(/\s*()\s*/, “”)

  • conditions.sub!(/^(?:())?\s?(?:and|or)\s*/, “”)

  • conditions.sub!(/(\s*(?:or|and)\s*(/, “((”)

  • {:conditions => conditions}

  • }

  • returns recent reports

    named_scope :recent, lambda { |*args| {:conditions => [“reported_at > ?”, (args.first || 1.day.ago)]} }

–
1.7.7.4

2

Hi,

sadly this seems to be based on the 0.4 branch too, would you mind
rebasing as well?

thanks!
Ohad

··· On Fri, Nov 25, 2011 at 5:09 PM, Greg Sutcliffe wrote: > This patch wraps an authentication check around: > Viewing a Host page directly (i.e going to the URL) > Editing a Host directly (via URL) > Listing reports only for the hosts in your filter > Viewing a report directly (via URL) > > There are almost certainly other, smaller leaks, via > direct URLs or via the API. This will need to be checked > > Other leaks include the Fact list needing to be filtered, the Dashboard, > and possibly the pie charts. This is considerably less important :) > > Signed-off-by: Greg Sutcliffe > --- > app/controllers/hosts_controller.rb | 4 ++++ > app/controllers/reports_controller.rb | 6 ++++-- > app/models/report.rb | 9 +++++++++ > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/app/controllers/hosts_controller.rb b/app/controllers/hosts_controller.rb > index dbac446..1b6a874 100644 > --- a/app/controllers/hosts_controller.rb > +++ b/app/controllers/hosts_controller.rb > @@ -54,6 +54,8 @@ class HostsController < ApplicationController > end > > def show > + auth = User.current.admin? ? true : Host.my_hosts.include?(@host) > + not_found and return unless auth > respond_to do |format| > format.html { > # filter graph time range > @@ -99,6 +101,8 @@ class HostsController < ApplicationController > end > > def edit > + auth = User.current.admin? ? true : Host.my_hosts.include?(@host) > + not_found and return unless auth > load_vars_for_ajax > end > > diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb > index d697b8f..828d2a6 100644 > --- a/app/controllers/reports_controller.rb > +++ b/app/controllers/reports_controller.rb > @@ -12,7 +12,8 @@ class ReportsController < ApplicationController > filter_parameter_logging :report > > def index > - values = Report.search_for(params[:search], :order => params[:order]) > + my_reports = User.current.admin? ? Report : Report.my_reports > + values = my_reports.search_for(params[:search], :order => params[:order]) > pagination_opts = { :page => params[:page], :per_page => params[:per_page] } > respond_to do |format| > format.html { @reports = values.paginate(pagination_opts.merge({ :include => :host })) } > @@ -32,7 +33,8 @@ class ReportsController < ApplicationController > > return not_found if params[:id].blank? > > - @report = Report.find(params[:id], :include => { :logs => [:message, :source] }) > + my_reports = User.current.admin? ? Report : Report.my_reports > + @report = my_reports.find(params[:id], :include => { :logs => [:message, :source] }) > respond_to do |format| > format.html { @offset = @report.reported_at - @report.created_at } > format.json { render :json => @report } > diff --git a/app/models/report.rb b/app/models/report.rb > index 6c8d9a6..780109a 100644 > --- a/app/models/report.rb > +++ b/app/models/report.rb > @@ -21,6 +21,15 @@ class Report < ActiveRecord::Base > scoped_search :on => :status, :offset => METRIC.index("failed_restarts"), :word_size => BIT_NUM, :rename => :failed_restarts > scoped_search :on => :status, :offset => METRIC.index("skipped"), :word_size => BIT_NUM, :rename => :skipped > > + named_scope :my_reports, lambda { > + user = User.current > + conditions = sanitize_sql_for_conditions([" (reports.host_id in (?))",Host.my_hosts.map(&:id)]) > + conditions.sub!(/\s*\(\)\s*/, "") > + conditions.sub!(/^(?:\(\))?\s?(?:and|or)\s*/, "") > + conditions.sub!(/\(\s*(?:or|and)\s*\(/, "((") > + {:conditions => conditions} > + } > + > # returns recent reports > named_scope :recent, lambda { |*args| {:conditions => ["reported_at > ?", (args.first || 1.day.ago)]} } > > -- > 1.7.7.4 >
3

I'm sure I got it to apply - but that was a few weeks back, things
have no doubt moved on. Will get on to it.

Greg

··· On 13 December 2011 12:47, Ohad Levy wrote: > Hi, > > sadly this seems to be based on the 0.4 branch too, would you mind > rebasing as well? > > thanks! > Ohad