From b47f6196aaf405f17197d4bb312d94ec84042343 Mon Sep 17 00:00:00 2001
From: Tomas Jelinek <tojeline@redhat.com>
Date: Tue, 25 Aug 2015 16:46:46 +0200
Subject: [PATCH] fixed command injection vulnerability
---
pcsd/fenceagent.rb | 53 ++++++++++++++++++++++++++++++++++-------------------
pcsd/pcsd.rb | 6 +++---
pcsd/remote.rb | 18 +++++++++---------
pcsd/resource.rb | 27 +++++++++++++++++++++++----
4 files changed, 69 insertions(+), 35 deletions(-)
diff --git a/pcsd/fenceagent.rb b/pcsd/fenceagent.rb
index b7674fd..b52ad6f 100644
--- a/pcsd/fenceagent.rb
+++ b/pcsd/fenceagent.rb
@@ -1,4 +1,4 @@
-def getFenceAgents(fence_agent = nil)
+def getFenceAgents(session, fence_agent = nil)
fence_agent_list = {}
agents = Dir.glob('/usr/sbin/fence_' + '*')
agents.each { |a|
@@ -7,7 +7,7 @@ def getFenceAgents(fence_agent = nil)
next if fa.name == "fence_ack_manual"
if fence_agent and a.sub(/.*\//,"") == fence_agent.sub(/.*:/,"")
- required_options, optional_options, advanced_options, info = getFenceAgentMetadata(fa.name)
+ required_options, optional_options, advanced_options, info = getFenceAgentMetadata(session, fa.name)
fa.required_options = required_options
fa.optional_options = optional_options
fa.advanced_options = advanced_options
@@ -18,13 +18,42 @@ def getFenceAgents(fence_agent = nil)
fence_agent_list
end
-def getFenceAgentMetadata(fenceagentname)
+def getFenceAgentMetadata(session, fenceagentname)
+ options_required = {}
+ options_optional = {}
+ options_advanced = {
+ "priority" => "",
+ "pcmk_host_argument" => "",
+ "pcmk_host_map" => "",
+ "pcmk_host_list" => "",
+ "pcmk_host_check" => ""
+ }
+ for a in ["reboot", "list", "status", "monitor", "off"]
+ options_advanced["pcmk_" + a + "_action"] = ""
+ options_advanced["pcmk_" + a + "_timeout"] = ""
+ options_advanced["pcmk_" + a + "_retries"] = ""
+ end
+
# There are bugs in stonith_admin & the new fence_agents interaction
# eventually we'll want to switch back to this, but for now we directly
# call the agent to get metadata
#metadata = `stonith_admin --metadata -a #{fenceagentname}`
- metadata = `/usr/sbin/#{fenceagentname} -o metadata`
- doc = REXML::Document.new(metadata)
+ if not fenceagentname.start_with?('fence_') or fenceagentname.include?('/')
+ $logger.error "Invalid fence agent '#{fenceagentname}'"
+ return [options_required, options_optional, options_advanced]
+ end
+ stdout, stderr, retval = run_cmd(
+ session, "/usr/sbin/#{fenceagentname}", '-o', 'metadata'
+ )
+ metadata = stdout.join
+ begin
+ doc = REXML::Document.new(metadata)
+ rescue REXML::ParseException => e
+ $logger.error(
+ "Unable to parse metadata of fence agent '#{resourcepath}': #{e}"
+ )
+ return [options_required, options_optional, options_advanced]
+ end
short_desc = ""
long_desc = ""
@@ -40,20 +69,6 @@ def getFenceAgentMetadata(fenceagentname)
long_desc = ld.text ? ld.text.strip : ld.text
}
- options_required = {}
- options_optional = {}
- options_advanced = {
- "priority" => "",
- "pcmk_host_argument" => "",
- "pcmk_host_map" => "",
- "pcmk_host_list" => "",
- "pcmk_host_check" => ""
- }
- for a in ["reboot", "list", "status", "monitor", "off"]
- options_advanced["pcmk_" + a + "_action"] = ""
- options_advanced["pcmk_" + a + "_timeout"] = ""
- options_advanced["pcmk_" + a + "_retries"] = ""
- end
doc.elements.each('resource-agent/parameters/parameter') { |param|
temp_array = []
if param.elements["shortdesc"]
diff --git a/pcsd/pcsd.rb b/pcsd/pcsd.rb
index e4b4c25..1f26fe5 100644
--- a/pcsd/pcsd.rb
+++ b/pcsd/pcsd.rb
@@ -401,7 +401,7 @@ if not DISABLE_GUI
if @resources.length == 0
@cur_resource = nil
- @resource_agents = getFenceAgents()
+ @resource_agents = getFenceAgents(session)
else
@cur_resource = @resources[0]
if params[:fencedevice]
@@ -413,7 +413,7 @@ if not DISABLE_GUI
end
end
@cur_resource.options = getResourceOptions(session, @cur_resource.id)
- @resource_agents = getFenceAgents(@cur_resource.agentname)
+ @resource_agents = getFenceAgents(session, @cur_resource.agentname)
end
erb :fencedevices, :layout => :main
end
@@ -477,7 +477,7 @@ if not DISABLE_GUI
# }
# }
@resource_agents = getResourceAgents(session)
- @stonith_agents = getFenceAgents()
+ @stonith_agents = getFenceAgents(session)
# @nodes = @nodes.sort_by{|k,v|k}
erb :nodes, :layout => :main
end
diff --git a/pcsd/remote.rb b/pcsd/remote.rb
index cb5b176..4655756 100644
--- a/pcsd/remote.rb
+++ b/pcsd/remote.rb
@@ -1370,11 +1370,11 @@ def resource_form(params, request, session)
@cur_resource_ms = @cur_resource.get_master
@resource = ResourceAgent.new(@cur_resource.agentname)
if @cur_resource.provider == 'heartbeat'
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(HEARTBEAT_AGENTS_DIR + @cur_resource.type)
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, HEARTBEAT_AGENTS_DIR + @cur_resource.type)
elsif @cur_resource.provider == 'pacemaker'
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(PACEMAKER_AGENTS_DIR + @cur_resource.type)
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, PACEMAKER_AGENTS_DIR + @cur_resource.type)
elsif @cur_resource._class == 'nagios'
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(NAGIOS_METADATA_DIR + @cur_resource.type + '.xml')
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, NAGIOS_METADATA_DIR + @cur_resource.type + '.xml')
end
@existing_resource = true
if @resource
@@ -1395,7 +1395,7 @@ def fence_device_form(params, request, session)
@cur_resource = get_resource_by_id(params[:resource], get_cib_dom(session))
if @cur_resource.instance_of?(ClusterEntity::Primitive) and @cur_resource.stonith
- @resource_agents = getFenceAgents(@cur_resource.agentname)
+ @resource_agents = getFenceAgents(session, @cur_resource.agentname)
@existing_resource = true
@fenceagent = @resource_agents[@cur_resource.type]
erb :fenceagentform
@@ -1531,7 +1531,7 @@ def get_avail_fence_agents(params, request, session)
if not allowed_for_local_cluster(session, Permissions::READ)
return 403, 'Permission denied'
end
- agents = getFenceAgents()
+ agents = getFenceAgents(session)
return JSON.generate(agents)
end
@@ -1545,11 +1545,11 @@ def resource_metadata(params, request, session)
@resource = ResourceAgent.new(params[:resourcename])
if class_provider == "ocf:heartbeat"
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(HEARTBEAT_AGENTS_DIR + resource_name)
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, HEARTBEAT_AGENTS_DIR + resource_name)
elsif class_provider == "ocf:pacemaker"
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(PACEMAKER_AGENTS_DIR + resource_name)
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, PACEMAKER_AGENTS_DIR + resource_name)
elsif class_provider == 'nagios'
- @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(NAGIOS_METADATA_DIR + resource_name + '.xml')
+ @resource.required_options, @resource.optional_options, @resource.info = getResourceMetadata(session, NAGIOS_METADATA_DIR + resource_name + '.xml')
end
@new_resource = params[:new]
@resources, @groups = getResourcesGroups(session)
@@ -1563,7 +1563,7 @@ def fence_device_metadata(params, request, session)
end
return 200 if not params[:resourcename] or params[:resourcename] == ""
@fenceagent = FenceAgent.new(params[:resourcename])
- @fenceagent.required_options, @fenceagent.optional_options, @fenceagent.advanced_options, @fenceagent.info = getFenceAgentMetadata(params[:resourcename])
+ @fenceagent.required_options, @fenceagent.optional_options, @fenceagent.advanced_options, @fenceagent.info = getFenceAgentMetadata(session, params[:resourcename])
@new_fenceagent = params[:new]
erb :fenceagentform
diff --git a/pcsd/resource.rb b/pcsd/resource.rb
index c6b513b..6f8f7fe 100644
--- a/pcsd/resource.rb
+++ b/pcsd/resource.rb
@@ -1,4 +1,5 @@
require 'pp'
+require 'pathname'
def getResourcesGroups(session, get_fence_devices = false, get_all_options = false,
get_operations=false
@@ -302,12 +303,24 @@ def getColocationConstraints(session, resource_id)
return together,apart
end
-def getResourceMetadata(resourcepath)
+def getResourceMetadata(session, resourcepath)
options_required = {}
options_optional = {}
long_desc = ""
short_desc = ""
+ resourcepath = Pathname.new(resourcepath).cleanpath.to_s
+ resource_dirs = [
+ HEARTBEAT_AGENTS_DIR, PACEMAKER_AGENTS_DIR, NAGIOS_METADATA_DIR,
+ ]
+ if not resource_dirs.any? { |allowed| resourcepath.start_with?(allowed) }
+ $logger.error(
+ "Unable to get metadata of resource agent '#{resourcepath}': " +
+ 'path not allowed'
+ )
+ return [options_required, options_optional, [short_desc, long_desc]]
+ end
+
if resourcepath.end_with?('.xml')
begin
metadata = IO.read(resourcepath)
@@ -316,12 +329,16 @@ def getResourceMetadata(resourcepath)
end
else
ENV['OCF_ROOT'] = OCF_ROOT
- metadata = `#{resourcepath} meta-data`
+ stdout, stderr, retval = run_cmd(session, resourcepath, 'meta-data')
+ metadata = stdout.join
end
begin
doc = REXML::Document.new(metadata)
- rescue REXML::ParseException
+ rescue REXML::ParseException => e
+ $logger.error(
+ "Unable to parse metadata of resource agent '#{resourcepath}': #{e}"
+ )
return [options_required, options_optional, [short_desc, long_desc]]
end
@@ -381,7 +398,9 @@ def getResourceAgents(session, resource_agent=nil)
if resource_agent and (a.start_with?("ocf:heartbeat:") or a.start_with?("ocf:pacemaker:"))
split_agent = ra.name.split(/:/)
path = OCF_ROOT + '/resource.d/' + split_agent[1] + "/" + split_agent[2]
- required_options, optional_options, resource_info = getResourceMetadata(path)
+ required_options, optional_options, resource_info = getResourceMetadata(
+ session, path
+ )
ra.required_options = required_options
ra.optional_options = optional_options
ra.info = resource_info
--
1.9.1