Fix path traversal vulnerabilities

Author: ryanmelt

openc3-cosmos-cmd-tlm-api/app/controllers/application_controller.rb

@@ -57,4 +57,34 @@ def authorization(permission, target_name: nil)
     end
     true
   end
+
+  def sanitize_params(param_list, require_params: true, allow_forward_slash: false)
+    if require_params
+      result = params.require(param_list)
+    else
+      result = []
+      param_list.each do |param|
+        result << params[param]
+      end
+    end
+    result.each_with_index do |arg, index|
+      if arg
+        # Prevent the code scanner detects:
+        # "Uncontrolled data used in path expression"
+        # This method is taken directly from the Rails source:
+        #   https://api.rubyonrails.org/v5.2/classes/ActiveStorage/Filename.html#method-i-sanitized
+        if allow_forward_slash
+          # Sometimes we have forward slashes so optionally allow those
+          value = arg.encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;\t\r\n\\", "-")
+        else
+          value = arg.encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;/\t\r\n\\", "-")
+        end
+        if value != arg
+          render(json: { status: 'error', message: "Invalid parameter #{param_list[index]}" }, status: 400)
+          return false
+        end
+      end
+    end
+    return result
+  end
 end

openc3-cosmos-cmd-tlm-api/app/controllers/plugins_controller.rb

@@ -35,14 +35,17 @@ def create(update = false)
     return unless authorization('admin')
     file = params[:plugin]
     if file
+      scope = sanitize_params([:scope])
+      return unless scope
+      scope = scope[0]
       temp_dir = Dir.mktmpdir
       begin
         gem_file_path = temp_dir + '/' + file.original_filename
         FileUtils.cp(file.tempfile.path, gem_file_path)
         if @existing_model
-          result = OpenC3::PluginModel.install_phase1(gem_file_path, existing_variables: @existing_model['variables'], existing_plugin_txt_lines: @existing_model['plugin_txt_lines'], scope: params[:scope])
+          result = OpenC3::PluginModel.install_phase1(gem_file_path, existing_variables: @existing_model['variables'], existing_plugin_txt_lines: @existing_model['plugin_txt_lines'], scope: scope)
         else
-          result = OpenC3::PluginModel.install_phase1(gem_file_path, scope: params[:scope])
+          result = OpenC3::PluginModel.install_phase1(gem_file_path, scope: scope)
         end
         render :json => result
       rescue Exception => error
@@ -67,17 +70,22 @@ def update
   def install
     return unless authorization('admin')
     begin
+      scope = sanitize_params([:scope])
+      return unless scope
+      scope = scope[0]
       temp_dir = Dir.mktmpdir
       plugin_hash_filename = Dir::Tmpname.create(['plugin-instance-', '.json']) {}
       plugin_hash_file_path = File.join(temp_dir, File.basename(plugin_hash_filename))
       File.open(plugin_hash_file_path, 'wb') do |file|
         file.write(params[:plugin_hash])
       end
 
-      gem_name = params[:id].split("__")[0]
+      gem_name = sanitize_params([:id])
+      return unless gem_name
+      gem_name = gem_name[0].split("__")[0]
       result = OpenC3::ProcessManager.instance.spawn(
-        ["ruby", "/openc3/bin/openc3cli", "load", gem_name, params[:scope], plugin_hash_file_path, "force"], # force install
-        "plugin_install", params[:id], Time.now + 1.hour, temp_dir: temp_dir, scope: params[:scope]
+        ["ruby", "/openc3/bin/openc3cli", "load", gem_name, scope, plugin_hash_file_path, "force"], # force install
+        "plugin_install", params[:id], Time.now + 1.hour, temp_dir: temp_dir, scope: scope
       )
       render :json => result.name
     rescue Exception => error
@@ -88,7 +96,9 @@ def install
   def destroy
     return unless authorization('admin')
     begin
-      result = OpenC3::ProcessManager.instance.spawn(["ruby", "/openc3/bin/openc3cli", "unload", params[:id], params[:scope]], "plugin_uninstall", params[:id], Time.now + 1.hour, scope: params[:scope])
+      id, scope = sanitize_params([:id, scope])
+      return unless id and scope
+      result = OpenC3::ProcessManager.instance.spawn(["ruby", "/openc3/bin/openc3cli", "unload", id, scope], "plugin_uninstall", id, Time.now + 1.hour, scope: scope)
       render :json => result.name
     rescue Exception => error
       render(:json => { :status => 'error', :message => error.message }, :status => 500) and return

openc3-cosmos-cmd-tlm-api/app/controllers/screens_controller.rb

@@ -19,12 +19,17 @@
 class ScreensController < ApplicationController
   def index
     return unless authorization('system')
-    render :json => Screen.all(*params.require([:scope]))
+    scope = sanitize_params([:scope])
+    return unless scope
+    scope = scope[0]
+    render :json => Screen.all(scope)
   end
 
   def show
     return unless authorization('system')
-    screen = Screen.find(*params.require([:scope, :target, :screen]))
+    result = sanitize_params([:scope, :target, :screen])
+    return unless result
+    screen = Screen.find(*result)
     if screen
       render :json => screen
     else
@@ -34,7 +39,11 @@ def show
 
   def create
     return unless authorization('system_set')
-    screen = Screen.create(*params.require([:scope, :target, :screen, :text]))
+    result = sanitize_params([:scope, :target, :screen])
+    return unless result
+    text = params.require([:text])[0]
+    result << text
+    screen = Screen.create(*result)
     OpenC3::Logger.info("Screen saved: #{params[:target]} #{params[:screen]}", scope: params[:scope], user: username())
     render :json => screen
   rescue => e
@@ -43,7 +52,9 @@ def create
 
   def destroy
     return unless authorization('system_set')
-    screen = Screen.destroy(*params.require([:scope, :target, :screen]))
+    result = sanitize_params([:scope, :target, :screen])
+    return unless result
+    screen = Screen.destroy(*result)
     OpenC3::Logger.info("Screen deleted: #{params[:target]} #{params[:screen]}", scope: params[:scope], user: username())
     head :ok
   rescue => e

openc3-cosmos-cmd-tlm-api/app/controllers/storage_controller.rb

@@ -25,6 +25,7 @@
 
 class StorageController < ApplicationController
   def buckets
+    return unless authorization('system')
     # ENV.map returns a big array of mostly nils which is why we compact
     # The non-nil are MatchData objects due to the regex match
     matches = ENV.map { |key, _value| key.match(/^OPENC3_(.+)_BUCKET$/) }.compact
@@ -35,6 +36,7 @@ def buckets
   end
 
   def volumes
+    return unless authorization('system')
     # ENV.map returns a big array of mostly nils which is why we compact
     # The non-nil are MatchData objects due to the regex match
     matches = ENV.map { |key, _value| key.match(/^OPENC3_(.+)_VOLUME$/) }.compact

openc3-cosmos-cmd-tlm-api/app/controllers/tables_controller.rb

@@ -23,17 +23,20 @@
 require 'base64'
 
 class TablesController < ApplicationController
-  before_action :sanitize_scope
-
   def index
     return unless authorization('system')
-    render json: Table.all(params[:scope])
+    scope = sanitize_params([:scope])
+    return unless scope
+    scope = scope[0]
+    render json: Table.all(scope)
   end
 
   def binary
     return unless authorization('system')
+    scope, binary, definition, table = sanitize_params([:scope, :binary, :definition, :table], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      file = Table.binary(params[:scope], params[:binary], params[:definition], params[:table])
+      file = Table.binary(scope, binary, definition, table)
       results = { 'filename' => file.filename, 'contents' => Base64.encode64(file.contents) }
       render json: results
     rescue Table::NotFound => e
@@ -44,8 +47,10 @@ def binary
 
   def definition
     return unless authorization('system')
+    scope, definition, table = sanitize_params([:scope, :definition, :table], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      file = Table.definition(params[:scope], params[:definition], params[:table])
+      file = Table.definition(scope, definition, table)
       render json: { 'filename' => file.filename, 'contents' => file.contents }
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
@@ -55,8 +60,10 @@ def definition
 
   def report
     return unless authorization('system')
+    scope, binary, definition, table = sanitize_params([:scope, :binary, :definition, :table], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      file = Table.report(params[:scope], params[:binary], params[:definition], params[:table])
+      file = Table.report(scope, binary, definition, table)
       render json: { 'filename' => file.filename, 'contents' => file.contents }
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
@@ -66,17 +73,19 @@ def report
 
   def body
     return unless authorization('system')
+    scope, name = sanitize_params([:scope, :name], require_params: true, allow_forward_slash: true)
+    return unless scope
     # body doesn't raise if not found ... it returns nil
-    file = Table.body(params[:scope], params[:name])
+    file = Table.body(scope, name)
     if file
       results = {}
 
-      if File.extname(params[:name]) == '.txt'
+      if File.extname(name) == '.txt'
         results = { 'contents' => file }
       else
-        locked = Table.locked?(params[:scope], params[:name])
+        locked = Table.locked?(scope, name)
         unless locked
-          Table.lock(params[:scope], params[:name], username())
+          Table.lock(scope, name, username())
         end
         results = { 'contents' => Base64.encode64(file), 'locked' => locked }
       end
@@ -91,8 +100,10 @@ def body
 
   def load
     return unless authorization('system')
+    scope, binary, definition = sanitize_params([:scope, :binary, :definition], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      render json: Table.load(params[:scope], params[:binary], params[:definition])
+      render json: Table.load(scope, binary, definition)
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
         return
@@ -101,8 +112,10 @@ def load
 
   def save
     return unless authorization('system')
+    scope, binary, definition = sanitize_params([:scope, :binary, :definition], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      Table.save(params[:scope], params[:binary], params[:definition], params[:tables])
+      Table.save(scope, binary, definition, params[:tables])
       head :ok
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
@@ -112,8 +125,10 @@ def save
 
   def save_as
     return unless authorization('system')
+    scope, name, new_name = sanitize_params([:scope, :name, :new_name], require_params: true, allow_forward_slash: true)
+    return unless scope
     begin
-      Table.save_as(params[:scope], params[:name], params[:new_name])
+      Table.save_as(scope, name, new_name)
       head :ok
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
@@ -123,8 +138,10 @@ def save_as
 
   def generate
     return unless authorization('system')
+    scope, definition = sanitize_params([:scope, :definition], require_params: false, allow_forward_slash: true)
+    return unless scope
     begin
-      filename = Table.generate(params[:scope], params[:definition])
+      filename = Table.generate(scope, definition)
       render json: { 'filename' => filename }
     rescue Table::NotFound => e
       render(json: { status: 'error', message: e.message }, status: 404) and
@@ -134,40 +151,32 @@ def generate
 
   def lock
     return unless authorization('system')
-    Table.lock(params[:scope], params[:name], username())
+    scope, name = sanitize_params([:scope, :name], require_params: true, allow_forward_slash: true)
+    return unless scope
+    Table.lock(scope, name, username())
     render status: 200
   end
 
   def unlock
     return unless authorization('system')
-    locked_by = Table.locked?(params[:scope], params[:name])
-    Table.unlock(params[:scope], params[:name]) if username() == locked_by
+    scope, name = sanitize_params([:scope, :name], require_params: true, allow_forward_slash: true)
+    return unless scope
+    locked_by = Table.locked?(scope, name)
+    Table.unlock(scope, name) if username() == locked_by
     render status: 200
   end
 
   def destroy
     return unless authorization('system')
+    scope, name = sanitize_params([:scope, :name], require_params: true, allow_forward_slash: true)
+    return unless scope
     # destroy returns no indication of success or failure so just assume it worked
-    Table.destroy(params[:scope], params[:name])
+    Table.destroy(scope, name)
     OpenC3::Logger.info(
-      "Table destroyed: #{params[:name]}",
-      scope: params[:scope],
+      "Table destroyed: #{name}",
+      scope: scope,
       user: username()
     )
     head :ok
   end
-
-  private
-
-  def sanitize_scope
-    # scope is passed as a parameter and we use it to create paths in local_mode,
-    # thus we have to sanitize it or the code scanner detects:
-    # "Uncontrolled data used in path expression"
-    # This method is taken directly from the Rails source:
-    #   https://api.rubyonrails.org/v5.2/classes/ActiveStorage/Filename.html#method-i-sanitized
-    scope = params[:scope].encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;/\t\r\n\\", "-")
-    if scope != params[:scope]
-      render(json: { status: 'error', message: "Invalid scope: #{params[:scope]}" }, status: 400)
-    end
-  end
 end