Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check file path prefixes #1844

Merged
merged 5 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ After configuring OpenC3 COSMOS to talk to your hardware, you immediately can us

- Script Runner allows for running OpenC3 COSMOS test procedures or any other Ruby code from a graphical environment that highlights each line as it executes. At anytime during execution, the script can be paused or stopped. If a telemetry check fails or any other exception occurs, the script is immediately stopped and the user notified.

- Script Runner also allows you to break your operational or test procedures down into discret test cases that each complete with either SUCCESS or FAILURE. After running, a script report is automatically created for you. Convenient features such as the ability to loop testing help get the kinks out of your system before formal runs.
- Script Runner also allows you to break your operational or test procedures down into discrete test cases that each complete with either SUCCESS or FAILURE. After running, a script report is automatically created for you. Convenient features such as the ability to loop testing help get the kinks out of your system before formal runs.

1. **Packet Viewer**

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1792,7 +1792,7 @@ export default {
switch (data.method) {
case 'ask':
case 'ask_string':
// Reset values since this dialog can be re-used
// Reset values since this dialog can be reused
this.ask.default = null
this.ask.answerRequired = true
this.ask.password = false
Expand Down
15 changes: 13 additions & 2 deletions openc3/lib/openc3/utilities/local_mode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ def self.zip_target(target_name, zip, scope:)

def self.put_target_file(path, io_or_string, scope:)
full_folder_path = "#{OPENC3_LOCAL_MODE_PATH}/#{path}"
return unless File.expand_path(full_folder_path).start_with?(OPENC3_LOCAL_MODE_PATH)
FileUtils.mkdir_p(File.dirname(full_folder_path))
File.open(full_folder_path, 'wb') do |file|
if String === io_or_string
Expand All @@ -393,7 +394,10 @@ def self.put_target_file(path, io_or_string, scope:)

def self.open_local_file(path, scope:)
full_path = "#{OPENC3_LOCAL_MODE_PATH}/#{scope}/targets_modified/#{path}"
return File.open(full_path, 'rb')
if File.expand_path(full_path).start_with?(OPENC3_LOCAL_MODE_PATH)
return File.open(full_path, 'rb')
Dismissed Show dismissed Hide dismissed
end
nil
rescue Errno::ENOENT
nil
end
Expand Down Expand Up @@ -446,14 +450,17 @@ def self.sync_tool_config()
def self.save_tool_config(scope, tool, name, data)
json = JSON.parse(data, :allow_nan => true, :create_additions => true)
config_path = "#{OPENC3_LOCAL_MODE_PATH}/#{scope}/tool_config/#{tool}/#{name}.json"
return unless File.expand_path(config_path).start_with?(OPENC3_LOCAL_MODE_PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're building this so isn't it always going to start with OPENC3_LOCAL_MODE_PATH? Or are we protecting from someone adding ../../../ or whatever in the scope, tool, or name portion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we should implement these same checks in local_mode.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is better protection against traversing up the file tree. Basic attacks will be caught by this, but calling expand_path and making sure that it starts in the correct dir is a better way to protect against theoretically more sophisticated attacks

Ah I see, I missed part of local_mode.py, I'll update that... Is there a reason most of that file is commented out? Can we remove the large blocks of commented out code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that's part of the plugin installation process can be removed because that's Ruby only. So yeah you can remove it all.

FileUtils.mkdir_p(File.dirname(config_path))
File.open(config_path, 'w') do |file|
file.write(JSON.pretty_generate(json, :allow_nan => true))
end
end

def self.delete_tool_config(scope, tool, name)
FileUtils.rm_f("#{OPENC3_LOCAL_MODE_PATH}/#{scope}/tool_config/#{tool}/#{name}.json")
config_path = "#{OPENC3_LOCAL_MODE_PATH}/#{scope}/tool_config/#{tool}/#{name}.json"
return unless File.expand_path(config_path).start_with?(OPENC3_LOCAL_MODE_PATH)
FileUtils.rm_f(config_path)
end

def self.sync_settings()
Expand All @@ -471,6 +478,7 @@ def self.sync_settings()

def self.save_setting(scope, name, data)
config_path = "#{OPENC3_LOCAL_MODE_PATH}/#{scope}/settings/#{name}.json"
return unless File.expand_path(config_path).start_with?(OPENC3_LOCAL_MODE_PATH)
FileUtils.mkdir_p(File.dirname(config_path))
# Anything can be stored as a setting so write it out directly
File.write(config_path, data)
Expand All @@ -480,19 +488,22 @@ def self.save_setting(scope, name, data)

def self.sync_remote_to_local(bucket, key)
local_path = "#{OPENC3_LOCAL_MODE_PATH}/#{key}"
return unless File.expand_path(local_path).start_with?(OPENC3_LOCAL_MODE_PATH)
FileUtils.mkdir_p(File.dirname(local_path))
bucket.get_object(bucket: ENV['OPENC3_CONFIG_BUCKET'], key: key, path: local_path)
end

def self.sync_local_to_remote(bucket, key)
local_path = "#{OPENC3_LOCAL_MODE_PATH}/#{key}"
return unless File.expand_path(local_path).start_with?(OPENC3_LOCAL_MODE_PATH)
File.open(local_path, 'rb') do |read_file|
bucket.put_object(bucket: ENV['OPENC3_CONFIG_BUCKET'], key: key, body: read_file)
end
end

def self.delete_local(key)
local_path = "#{OPENC3_LOCAL_MODE_PATH}/#{key}"
return unless File.expand_path(local_path).start_with?(OPENC3_LOCAL_MODE_PATH)
File.delete(local_path) if File.exist?(local_path)
nil
end
Expand Down
Loading
Loading