Redmine #3288, #3404, #3405
This is the lastest revision to the latest round of security updates, incorporating all feedback so far. The spec and feature suites still work after these patches are applied and everything should be good to go for approval
From: mmorsi mmorsi@redhat.com
--- src/app/controllers/application_controller.rb | 3 ++- src/app/controllers/permissions_controller.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index eb5931b..6b94efb 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -300,7 +300,8 @@ class ApplicationController < ActionController::Base
def sort_column(model, default = nil) return params[:order_field] if model.column_names.include?(params[:order_field]) - return default || "#{model.quoted_table_name}.name" + return default unless default.nil? || !model.column_names.include?(default) + return "#{model.quoted_table_name}.name" end
def sort_direction diff --git a/src/app/controllers/permissions_controller.rb b/src/app/controllers/permissions_controller.rb index acdbb2c..d621e6d 100644 --- a/src/app/controllers/permissions_controller.rb +++ b/src/app/controllers/permissions_controller.rb @@ -169,7 +169,8 @@ class PermissionsController < ApplicationController
def load_entities sort_order = params[:sort_by].nil? ? "name" : params[:sort_by] - @entities = paginate_collection(Entity.all(:order => sort_order), params[:page]) + @entities = paginate_collection(Entity.all(:order => sort_column(Entity, sort_order)), + params[:page]) end
def load_headers
--- src/app/controllers/permissions_controller.rb | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/permissions_controller.rb b/src/app/controllers/permissions_controller.rb index d621e6d..c4d1371 100644 --- a/src/app/controllers/permissions_controller.rb +++ b/src/app/controllers/permissions_controller.rb @@ -190,7 +190,11 @@ class PermissionsController < ApplicationController unless obj_type or id @permission_object = BasePermissionObject.general_permission_scope end - @permission_object = obj_type.constantize.find(id) if obj_type and id + if klass = ActiveRecord::Base.send(:subclasses).find{|c| c.name == obj_type} + @permission_object = klass.find(id) + else + raise RuntimeError, "invalid permission object type #{obj_type}" + end raise RuntimeError, "invalid permission object" if @permission_object.nil? if @permission_object == BasePermissionObject.general_permission_scope @return_path = permissions_path
--- src/app/controllers/application_controller.rb | 1 + src/app/controllers/pools_controller.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index 6b94efb..1907d23 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -122,6 +122,7 @@ class ApplicationController < ActionController::Base
# Returns an array of ids from params[:id], params[:ids]. def ids_list(other_attrs=[]) + other_attrs = Array(other_attrs) unless other_attrs.is_a?(Array) other_attrs.each do |attr_key| return Array(params[attr_key]) if params.include?(attr_key) end diff --git a/src/app/controllers/pools_controller.rb b/src/app/controllers/pools_controller.rb index 20a5572..0668217 100644 --- a/src/app/controllers/pools_controller.rb +++ b/src/app/controllers/pools_controller.rb @@ -244,7 +244,7 @@ class PoolsController < ApplicationController failed = [] permission_failed = [] error_messages = [] - Pool.find(ids_list('pools_selected')).each do |pool| + Pool.find(ids_list(['pools_selected'])).each do |pool| if pool.id == MetadataObject.lookup("self_service_default_pool").id error_messages << t("pools.flash.error.default_pool_not_deleted") elsif !check_privilege(Privilege::MODIFY, pool)
From: mmorsi mmorsi@redhat.com
--- .../controllers/hardware_profiles_controller.rb | 2 ++ src/app/controllers/instances_controller.rb | 2 ++ src/app/controllers/providers_controller.rb | 4 ++++ src/app/controllers/users_controller.rb | 2 +- 4 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/hardware_profiles_controller.rb b/src/app/controllers/hardware_profiles_controller.rb index 2b91d5d..94588a9 100644 --- a/src/app/controllers/hardware_profiles_controller.rb +++ b/src/app/controllers/hardware_profiles_controller.rb @@ -53,6 +53,8 @@ class HardwareProfilesController < ApplicationController @tab_captions = [t('hardware_profiles.tab_captions.properties'), t('hardware_profiles.tab_captions.history'), t('hardware_profiles.tab_captions.matching_provider_hwp')] @details_tab = params[:details_tab].blank? ? 'properties' : params[:details_tab] properties + @details_tab = 'properties' unless['properties', 'history', + 'matching_provider_hardware_profiles'].include?(@details_tab) find_matching_provider_hardware_profiles save_breadcrumb(hardware_profile_path(@hardware_profile), @hardware_profile.name)
diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb index 110f1d2..b9031bd 100644 --- a/src/app/controllers/instances_controller.rb +++ b/src/app/controllers/instances_controller.rb @@ -48,6 +48,8 @@ class InstancesController < ApplicationController save_breadcrumb(instance_path(@instance), @instance.name) @events = @instance.events.paginate(:page => params[:page] || 1) @view = params[:details_tab].blank? ? 'properties' : params[:details_tab] + @details_tab = 'properties' unless ['properties', 'history', + 'parameters', 'permissions'].include?(@details_tab) @tabs = [ {:name => t('properties'), :view => @view, :id => 'properties'}, {:name => t('instances.parameters.config_parameters'), :view => 'parameters', :id => 'parameters'}, diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb index ce6b91a..81ac69f 100644 --- a/src/app/controllers/providers_controller.rb +++ b/src/app/controllers/providers_controller.rb @@ -83,6 +83,9 @@ class ProvidersController < ApplicationController require_privilege(Privilege::VIEW, @provider) @tab_captions = [t("properties"), t('hw_profiles'), t('realm_s'), t("provider_accounts.index.provider_accounts"), t('services'), t('history'), t('permissions')] @details_tab = params[:details_tab].blank? ? t("properties") : params[:details_tab] + @details_tab = 'properties' unless ['properties', 'hw_profiles', 'realms', + 'provider_accounts', 'services', 'history', + 'permissions'].include?(@details_tab)
if params.delete :test_provider test_connection(@provider) @@ -305,6 +308,7 @@ class ProvidersController < ApplicationController ] add_permissions_tab(@provider, "edit_") details_tab_name = params[:details_tab].blank? ? 'connectivity' : params[:details_tab] + details_tab_name = 'connectivity' unless ['connectivity', 'accounts', 'realms'].include?(details_tab_name) @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
if @details_tab[:id] == 'accounts' diff --git a/src/app/controllers/users_controller.rb b/src/app/controllers/users_controller.rb index 010ebe1..d726370 100644 --- a/src/app/controllers/users_controller.rb +++ b/src/app/controllers/users_controller.rb @@ -72,7 +72,7 @@ class UsersController < ApplicationController @quota_resources = @user.quota.quota_resources save_breadcrumb(user_path(@user), @user.name) @tab_captions = ['Properties'] - @details_tab = params[:details_tab].blank? ? 'properties' : params[:details_tab] + @details_tab = 'properties' # currently the only supported details tab respond_to do |format| format.html format.js do
--- src/app/controllers/instances_controller.rb | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb index b9031bd..8c39de9 100644 --- a/src/app/controllers/instances_controller.rb +++ b/src/app/controllers/instances_controller.rb @@ -33,16 +33,6 @@ class InstancesController < ApplicationController end end
- def new - respond_to do |format| - format.js - format.json - end - end - - def create - end - def show load_instances save_breadcrumb(instance_path(@instance), @instance.name)
--- src/app/controllers/instances_controller.rb | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb index 8c39de9..121e7d2 100644 --- a/src/app/controllers/instances_controller.rb +++ b/src/app/controllers/instances_controller.rb @@ -272,6 +272,7 @@ class InstancesController < ApplicationController def check_inaccessible_instances # @instance is set only on stop action @instances_to_stop = @instance ? @instance.to_a : Instance.find(params[:instance_selected].to_a) + @instances_to_stop.reject! { |inst| !check_privilege(Privilege::USE, inst) } @inaccessible_instances = Instance.stoppable_inaccessible_instances(@instances_to_stop) if params[:terminate].blank? and @inaccessible_instances.any? respond_to do |format|
--- .../controllers/provider_accounts_controller.rb | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/provider_accounts_controller.rb b/src/app/controllers/provider_accounts_controller.rb index 4ea7db2..157251a 100644 --- a/src/app/controllers/provider_accounts_controller.rb +++ b/src/app/controllers/provider_accounts_controller.rb @@ -141,9 +141,10 @@ class ProviderAccountsController < ApplicationController end
def destroy + @provider_account = ProviderAccount.find(params[:id]) require_privilege(Privilege::MODIFY, @provider_account) @provider = Provider.find(params[:provider_id]) - if ProviderAccount.destroy(params[:id]) + if @provider_account.destroy flash[:notice] = t"provider_accounts.flash.notice.deleted" else flash[:error] = t"provider_accounts.flash.error.not_deleted"
--- src/app/controllers/config_servers_controller.rb | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/config_servers_controller.rb b/src/app/controllers/config_servers_controller.rb index 0fe1108..896c6af 100644 --- a/src/app/controllers/config_servers_controller.rb +++ b/src/app/controllers/config_servers_controller.rb @@ -37,12 +37,14 @@ class ConfigServersController < ApplicationController
def test config_server = ConfigServer.find(params[:id]) + provider_account = config_server.provider_account + require_privilege(Privilege::VIEW, provider_account) + if not config_server.connection_valid? flash[:error] = config_server.connection_error_msg else flash[:notice] = t('config_servers.flash.notice.test_successful') end - provider_account = config_server.provider_account provider = provider_account.provider redirect_to provider_provider_account_path(provider, provider_account) end
--- src/app/controllers/roles_controller.rb | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/app/controllers/roles_controller.rb b/src/app/controllers/roles_controller.rb index efa4456..60de8e4 100644 --- a/src/app/controllers/roles_controller.rb +++ b/src/app/controllers/roles_controller.rb @@ -19,6 +19,7 @@ class RolesController < ApplicationController before_filter :load_roles, :only => [:show]
def index + require_privilege(Privilege::PERM_VIEW) clear_breadcrumbs save_breadcrumb(roles_path) load_roles
--- .../controllers/hardware_profiles_controller.rb | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/app/controllers/hardware_profiles_controller.rb b/src/app/controllers/hardware_profiles_controller.rb index 94588a9..13e6887 100644 --- a/src/app/controllers/hardware_profiles_controller.rb +++ b/src/app/controllers/hardware_profiles_controller.rb @@ -17,7 +17,6 @@ class HardwareProfilesController < ApplicationController before_filter :require_user before_filter :load_hardware_profiles, :only => [:index, :show] - before_filter :load_hardware_profile, :only => [:show] before_filter :setup_new_hardware_profile, :only => [:new] before_filter :setup_hardware_profile, :only => [:new, :create, :matching_provider_hardware_profiles, :edit, :update]
@@ -217,6 +216,7 @@ class HardwareProfilesController < ApplicationController
begin @matching_hwps = HardwareProfile.matching_hardware_profiles(@hardware_profile) + @matching_hwps.reject! { |hwp| !check_privilege(Privilege::VIEW, hwp) } rescue Exception => e @matching_hwps = [] end @@ -259,10 +259,8 @@ class HardwareProfilesController < ApplicationController end end end - end
- def load_hardware_profile - @hardware_profile = HardwareProfile.find(params[:id]) + @hardware_profiles.reject! { |hwp| !check_privilege(Privilege::VIEW, hwp) } end
def build_hardware_profile(params)
From: mmorsi mmorsi@localhost.localdomain
--- src/app/controllers/images_controller.rb | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb index 5fb2143..dd91923 100644 --- a/src/app/controllers/images_controller.rb +++ b/src/app/controllers/images_controller.rb @@ -28,6 +28,8 @@ class ImagesController < ApplicationController { :name => t('images.index.last_rebuild'), :sortable => false }, ] @images = paginate_collection(Aeolus::Image::Warehouse::Image.all, params[:page], PER_PAGE) + @images.reject! { |i| !check_privilege(Privilege::VIEW, i.environment) } + respond_to do |format| format.html format.js { render :partial => 'list' } @@ -37,8 +39,9 @@ class ImagesController < ApplicationController def show @image = Aeolus::Image::Warehouse::Image.find(params[:id]) @environment = PoolFamily.where('name' => @image.environment).first + require_privilege(Privilege::VIEW, @environment) @push_started = params[:push_started] == 'true' - @pushed_provider_account = ProviderAccount.find(params[:provider_account_id]) if params[:provider_account_id].present? + pushed_provider_account = ProviderAccount.find(params[:provider_account_id]) if params[:provider_account_id].present?
if @image.imported? begin @@ -57,6 +60,8 @@ class ImagesController < ApplicationController @account_groups = ProviderAccount.enabled.group_by_type(@environment) end
+ @account_groups.reject! { |driver, group| !check_privilege(Privilege::VIEW, group[:accounts][:account]) } + # according to imagefactory Builder.first shouldn't be implemented yet # but it does what we need - returns builder object which contains # all builds @@ -89,7 +94,7 @@ class ImagesController < ApplicationController provider_type = account_group[:provider_type] target_image = @target_images_by_target[provider_type.deltacloud_driver] @images_by_provider_type << - load_build_status_for_target_image(account_group, target_image) + load_build_status_for_target_image(account_group, target_image, pushed_provider_account) end
@push_all_enabled = @@ -134,7 +139,8 @@ class ImagesController < ApplicationController accounts = @environment.provider_accounts target_images = @build.target_images accounts.each do |account| - if account.image_status(@image) == :not_pushed + if account.image_status(@image) == :not_pushed && + check_privilege(Privilege::USE, account) target = account.provider.provider_type.deltacloud_driver target_image = target_images.find { |ti| ti.target == target } provider_image = Aeolus::Image::Factory::ProviderImage.new( @@ -180,6 +186,7 @@ class ImagesController < ApplicationController account = ProviderAccount.find(params[:provider_account]) @environment = PoolFamily.find(params[:environment]) check_permissions + require_privilege(Privilege::USE, account)
xml = "<image><name>#{params[:name]}</name></image>" unless params[:name].blank? begin @@ -294,14 +301,6 @@ class ImagesController < ApplicationController redirect_to image_path(@image.id) end
- def edit - check_permissions - end - - def update - check_permissions - end - def destroy if image = Aeolus::Image::Warehouse::Image.find(params[:id]) @environment = PoolFamily.where('name' => image.environment).first @@ -351,6 +350,7 @@ class ImagesController < ApplicationController # For now, Image permissions hijack the previously-unused PoolFamily USE privilege def check_permissions require_privilege(Privilege::USE, @environment) + @environment.provider_accounts.reject! { |a| !check_privilege(Privilege::USE, a) } end
def latest_build?(build) @@ -371,13 +371,13 @@ class ImagesController < ApplicationController return true end
- def load_build_status_for_target_image(account_group, target_image) + def load_build_status_for_target_image(account_group, target_image, pushed_provider_account) provider_type = account_group[:provider_type] provider_images_by_provider_account = []
account_group[:accounts].each do |account| provider_images_by_provider_account << - load_provider_images(account, target_image) + load_provider_images(account, target_image, pushed_provider_account) end
active_build = @@ -429,7 +429,7 @@ class ImagesController < ApplicationController target_image_for_provider_type end
- def load_provider_images(account, target_image) + def load_provider_images(account, target_image, pushed_provider_account) provider_image = if target_image.present? target_image.find_provider_image_by_provider_and_account(account.provider.name, @@ -448,7 +448,7 @@ class ImagesController < ApplicationController end
push_started_for_account = - (@push_started && @pushed_provider_account == account) + (@push_started && pushed_provider_account == account)
active_push = if target_image.present?
On Thu, Jun 21, 2012 at 04:51:48PM -0400, Mo Morsi wrote:
Redmine #3288, #3404, #3405
This is the lastest revision to the latest round of security updates, incorporating all feedback so far. The spec and feature suites still work after these patches are applied and everything should be good to go for approval
ACK. This resend fixes the various issues with failing tests, and looks sane. Plus, yay for better security.
Did we ever do anything thorough about protecting against mass-assignment?
-- Matt
On 06/25/2012 11:18 AM, Matt Wagner wrote:
On Thu, Jun 21, 2012 at 04:51:48PM -0400, Mo Morsi wrote:
Redmine #3288, #3404, #3405
This is the lastest revision to the latest round of security updates, incorporating all feedback so far. The spec and feature suites still work after these patches are applied and everything should be good to go for approval
ACK. This resend fixes the various issues with failing tests, and looks sane. Plus, yay for better security.
Thanks for the ACK Matt, pushed the patches, closing out the security story for this sprint.
Did we ever do anything thorough about protecting against mass-assignment?
We are tackling the mass-assignment issue incrementally as we go through the model classes. For example Maros pushed this [1] with some other security fixes a little while ago adding the attribute protection to the credential table. The plan of attack is to continue to do this, analysing and updating each model class in the same manner as we are doing w/ the controllers.
Will be reorganizing and updating the security page on the wiki soon and sending a status report to the list w/ an overall assessment. Look for that in the near future.
-Mo
[1] https://github.com/aeolusproject/conductor/commit/274fb0b55fed6a6b9e597e87bc...
On 26/06/2012, at 3:53 AM, Mo Morsi wrote: <snip>
Will be reorganizing and updating the security page on the wiki soon and sending a status report to the list w/ an overall assessment. Look for that in the near future.
Good thinking. :)
+ Justin
-Mo
[1] https://github.com/aeolusproject/conductor/commit/274fb0b55fed6a6b9e597e87bc...
-- Aeolus Community Manager http://www.aeolusproject.org
aeolus-devel@lists.fedorahosted.org