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