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.

Sign up for GitHub

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

Check file path prefixes #1844

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3b052f
Check file path prefixes
ryan-pratt Jan 17, 2025
ab58a84
Remove commented out code from local_mode.py
ryan-pratt Jan 21, 2025
0e425c4
Add file path prefix check to rest of local_mode.py
ryan-pratt Jan 22, 2025
7981ef0
Merge branch 'main' into file-path-prefixes
ryan-pratt Jan 22, 2025
5435330
fix spelling
ryan-pratt Jan 22, 2025
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
15 changes: 13 additions & 2 deletions openc3/lib/openc3/utilities/local_mode.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@

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.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')
end
nil
rescue Errno::ENOENT
nil
end
Expand Down Expand Up @@ -446,14 +450,17 @@
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.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.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
6 changes: 5 additions & 1 deletion openc3/python/openc3/utilities/local_mode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@ class LocalMode:
@classmethod
def put_target_file(cls, path, io_or_string, scope):
full_folder_path = f"{cls.LOCAL_MODE_PATH}/{path}"
if not os.path.normpath(full_folder_path).startswith(cls.LOCAL_MODE_PATH):
return
os.makedirs(os.path.dirname(full_folder_path), exist_ok=True)
flags = "w"
if isinstance(io_or_string, bytes):
Expand All @@ -347,7 +349,9 @@ def put_target_file(cls, path, io_or_string, scope):
def open_local_file(cls, path, scope):
try:
full_path = f"{cls.LOCAL_MODE_PATH}/{scope}/targets_modified/{path}"
return open(full_path, "rb")
if os.path.normpath(full_path).startswith(cls.LOCAL_MODE_PATH):
return open(full_path, "rb")
return None
except OSError:
return None

Expand Down
Loading