OpenC3 COSMOS allows a path traversal via screen controller (`GHSL-2024-127`)

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -23,66 +23,59 @@
 require 'openc3/models/target_model'
 
 class TargetsController < ModelController
-  before_action :sanitize_scope
-
   def initialize
     @model_class = OpenC3::TargetModel
   end
 
   # All targets with indication of modified targets
   def all_modified
     return unless authorization('system')
-    render :json => @model_class.all_modified(scope: params[:scope])
+    scope = sanitize_params([:scope], require_params: true)
+    return unless scope
+    scope = scope[0]
+    render :json => @model_class.all_modified(scope: scope)
   end
 
   def modified_files
     return unless authorization('system')
+    scope, id = sanitize_params([:scope, :id], require_params: true)
+    return unless scope
     begin
-      render :json => @model_class.modified_files(params[:id], scope: params[:scope])
+      render :json => @model_class.modified_files(id, scope: scope)
     rescue Exception => e
-      OpenC3::Logger.info("Target '#{params[:id]} modified_files failed: #{e.message}", user: username())
+      OpenC3::Logger.info("Target '#{id} modified_files failed: #{e.message}", user: username())
       head :internal_server_error
     end
   end
 
   def delete_modified
     return unless authorization('system')
+    scope, id = sanitize_params([:scope, :id], require_params: true)
+    return unless scope
     begin
-      @model_class.delete_modified(params[:id], scope: params[:scope])
+      @model_class.delete_modified(id, scope: scope)
       head :ok
     rescue Exception => e
-      OpenC3::Logger.info("Target '#{params[:id]} delete_modified failed: #{e.message}", user: username())
+      OpenC3::Logger.info("Target '#{id} delete_modified failed: #{e.message}", user: username())
       head :internal_server_error
     end
   end
 
   def download
     return unless authorization('system')
+    scope, id = sanitize_params([:scope, :id], require_params: true)
+    return unless scope
     begin
-      file = @model_class.download(params[:id], scope: params[:scope])
+      file = @model_class.download(id, scope: scope)
       if file
         results = { 'filename' => file.filename, 'contents' => Base64.encode64(file.contents) }
         render json: results
       else
         head :not_found
       end
     rescue Exception => e
-      OpenC3::Logger.info("Target '#{params[:id]} download failed: #{e.message}", user: username())
+      OpenC3::Logger.info("Target '#{id} download failed: #{e.message}", user: username())
       render(json: { status: 'error', message: e.message }, status: 500) and return
     end
   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

@@ -67,4 +67,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

@@ -40,31 +40,39 @@ def ping
 
   def index
     return unless authorization('script_view')
-    render :json => Script.all(params[:scope])
+    scope = sanitize_params([:scope])
+    return unless scope
+    scope = scope[0]
+    render :json => Script.all(scope)
   end
 
   def delete_temp
     return unless authorization('script_edit')
-    render :json => Script.delete_temp(params[:scope])
+    scope = sanitize_params([:scope])
+    return unless scope
+    scope = scope[0]
+    render :json => Script.delete_temp(scope)
   end
 
   def body
     return unless authorization('script_view')
+    scope, name = sanitize_params([:scope, :name])
+    return unless scope
 
-    file = Script.body(params[:scope], params[:name])
+    file = Script.body(scope, name)
     if file
-      locked = Script.locked?(params[:scope], params[:name])
+      locked = Script.locked?(scope, name)
       unless locked
-        Script.lock(params[:scope], params[:name], username())
+        Script.lock(scope, name, username())
       end
-      breakpoints = Script.get_breakpoints(params[:scope], params[:name])
+      breakpoints = Script.get_breakpoints(scope, name)
       results = {
         contents: file,
         breakpoints: breakpoints,
         locked: locked
       }
-      if ((File.extname(params[:name]) == '.py') and (file =~ PYTHON_SUITE_REGEX)) or ((File.extname(params[:name]) != '.py') and (file =~ SUITE_REGEX))
-        results_suites, results_error, success = Script.process_suite(params[:name], file, username: username(), scope: params[:scope])
+      if ((File.extname(name) == '.py') and (file =~ PYTHON_SUITE_REGEX)) or ((File.extname(name) != '.py') and (file =~ SUITE_REGEX))
+        results_suites, results_error, success = Script.process_suite(name, file, username: username(), scope: scope)
         results['suites'] = results_suites
         results['error'] = results_error
         results['success'] = success
@@ -79,30 +87,37 @@ def body
 
   def create
     return unless authorization('script_edit')
-    Script.create(params.permit(:scope, :name, :text, breakpoints: []))
+    scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true)
+    return unless scope
+    args = params.permit(:text, breakpoints: [])
+    args[:scope] = scope
+    args[:name] = name
+    Script.create(args)
     results = {}
-    if ((File.extname(params[:name]) == '.py') and (params[:text] =~ PYTHON_SUITE_REGEX)) or ((File.extname(params[:name]) != '.py') and (params[:text] =~ SUITE_REGEX))
-      results_suites, results_error, success = Script.process_suite(params[:name], params[:text], username: username(), scope: params[:scope])
+    if ((File.extname(name) == '.py') and (params[:text] =~ PYTHON_SUITE_REGEX)) or ((File.extname(name) != '.py') and (params[:text] =~ SUITE_REGEX))
+      results_suites, results_error, success = Script.process_suite(name, params[:text], username: username(), scope: scope)
       results['suites'] = results_suites
       results['error'] = results_error
       results['success'] = success
     end
-    OpenC3::Logger.info("Script created: #{params[:name]}", scope: params[:scope], user: username()) if success
+    OpenC3::Logger.info("Script created: #{name}", scope: scope, user: username()) if success
     render :json => results
   rescue => e
     render(json: { status: 'error', message: e.message }, status: 500)
   end
 
   def run
+    scope, name = sanitize_params([:scope, :name], :allow_forward_slash => true)
+    return unless scope
     # Extract the target that this script lives under
-    target_name = params[:name].split('/')[0]
+    target_name = name.split('/')[0]
     return unless authorization('script_run', target_name: target_name)
     suite_runner = params[:suiteRunner] ? params[:suiteRunner].as_json(:allow_nan => true) : nil
     disconnect = params[:disconnect] == 'disconnect'
     environment = params[:environment]
-    running_script_id = Script.run(params[:scope], params[:name], suite_runner, disconnect, environment, user_full_name(), username())
+    running_script_id = Script.run(scope, name, suite_runner, disconnect, environment, user_full_name(), username())
     if running_script_id
-      OpenC3::Logger.info("Script started: #{params[:name]}", scope: params[:scope], user: username())
+      OpenC3::Logger.info("Script started: #{name}", scope: scope, user: username())
       render :plain => running_script_id.to_s
     else
       head :not_found
@@ -111,31 +126,40 @@ def run
 
   def lock
     return unless authorization('script_edit')
-    Script.lock(params[:scope], params[:name], username())
+    scope, name = sanitize_params([:scope, :name])
+    return unless scope
+    Script.lock(scope, name, username())
     render status: 200
   end
 
   def unlock
     return unless authorization('script_edit')
-    locked_by = Script.locked?(params[:scope], params[:name])
-    Script.unlock(params[:scope], params[:name]) if username() == locked_by
+    scope, name = sanitize_params([:scope, :name])
+    return unless scope
+    locked_by = Script.locked?(scope, name)
+    Script.unlock(scope, name) if username() == locked_by
     render status: 200
   end
 
   def destroy
     return unless authorization('script_edit')
-    Script.destroy(*params.require([:scope, :name]))
-    OpenC3::Logger.info("Script destroyed: #{params[:name]}", scope: params[:scope], user: username())
+    scope, name = sanitize_params([:scope, :name])
+    return unless scope
+    Script.destroy(scope, name)
+    OpenC3::Logger.info("Script destroyed: #{name}", scope: scope, user: username())
     head :ok
   rescue => e
     render(json: { status: 'error', message: e.message }, status: 500)
   end
 
   def syntax
     # Extract the target that this script lives under
-    target_name = params[:name].split('/')[0]
+    name = sanitize_params([:name], :allow_forward_slash => true)
+    return unless name
+    name = name[0]
+    target_name = name.split('/')[0]
     return unless authorization('script_run', target_name: target_name)
-    script = Script.syntax(params[:name], request.body.read)
+    script = Script.syntax(name, request.body.read)
     if script
       render :json => script
     else
@@ -145,7 +169,10 @@ def syntax
 
   def instrumented
     return unless authorization('script_view')
-    script = Script.instrumented(params[:name], request.body.read)
+    name = sanitize_params([:name], :allow_forward_slash => true)
+    return unless name
+    name = name[0]
+    script = Script.instrumented(name, request.body.read)
     if script
       render :json => script
     else
@@ -155,7 +182,10 @@ def instrumented
 
   def delete_all_breakpoints
     return unless authorization('script_edit')
-    OpenC3::Store.del("#{params[:scope]}__script-breakpoints")
+    scope = sanitize_params([:scope])
+    return unless scope
+    scope = scope[0]
+    OpenC3::Store.del("#{scope}__script-breakpoints")
     head :ok
   end
 end