diff --git a/apps/dashboard/app/models/posix_file.rb b/apps/dashboard/app/models/posix_file.rb index 9a4f129442..fb617becd4 100644 --- a/apps/dashboard/app/models/posix_file.rb +++ b/apps/dashboard/app/models/posix_file.rb @@ -3,7 +3,7 @@ class PosixFile attr_reader :path, :stat - delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, :directory?, to: :path + delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, :directory?, :realpath, to: :path # include to give us number_to_human_size include ActionView::Helpers::NumberHelper @@ -95,7 +95,17 @@ def ls end def valid? - valid_encoding? + valid_realpath? && valid_encoding? + end + + def valid_realpath? + return false if stat.nil? || !path.exist? + + if stat.symlink? + AllowlistPolicy.default.permitted?(realpath) && AllowlistPolicy.default.permitted?(path) + else + AllowlistPolicy.default.permitted?(path) + end end def valid_encoding? diff --git a/apps/dashboard/test/models/files_test.rb b/apps/dashboard/test/models/files_test.rb index ff6040028e..d16e672d36 100644 --- a/apps/dashboard/test/models/files_test.rb +++ b/apps/dashboard/test/models/files_test.rb @@ -56,4 +56,65 @@ class FilesTest < ActiveSupport::TestCase test "Ensuring PosixFile.username(uid) returns string" do assert_equal "9999999", PosixFile.username(9999999) end -end \ No newline at end of file + + test 'files not in allowlist are invalid' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + invalid_file = "#{dir}/not_allowed/actual_file" + `touch #{invalid_file}` + + file = PosixFile.new(invalid_file) + refute(file.valid?) + end + end + end + + test 'symlinks outside allowlist are invalid' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + real_file = "#{dir}/not_allowed/actual_file" + symlink = "#{dir}/allowed/linked_file" + `touch #{real_file}` + `ln -s #{real_file} #{symlink}` + + real_file = PosixFile.new(real_file) + symlink = PosixFile.new(symlink) + + # both are invalid because they're not in the allowlist + refute(symlink.valid?) + refute(real_file.valid?) + end + end + end + + test 'ls output does not show invalid files' do + Dir.mktmpdir do |dir| + with_modified_env({ OOD_ALLOWLIST_PATH: "#{dir}/allowed" }) do + `mkdir -p #{dir}/allowed` + `mkdir -p #{dir}/not_allowed` + real_file = "#{dir}/not_allowed/actual_file" + symlink = "#{dir}/allowed/linked_file" + `touch #{real_file}` + + # symlink resolves outside of allowlist + `ln -s #{real_file} #{symlink}` + + # this file would be allowed - but is a broken symlink + `touch #{dir}/allowed/tmp` + `ln -s #{dir}/allowed/tmp #{dir}/allowed/broken_symlink` + `rm #{dir}/allowed/tmp` + + # they exist, but cannot be seen + assert_equal(1, PosixFile.new(dir).ls.size) + assert_equal(2, Dir.children(dir).size) + + assert_equal(0, PosixFile.new("#{dir}/allowed").ls.size) + assert_equal(2, Dir.children("#{dir}/allowed").size) + end + end + end +end diff --git a/apps/dashboard/test/system/files_test.rb b/apps/dashboard/test/system/files_test.rb index 5f8b74ad4c..e439620b15 100644 --- a/apps/dashboard/test/system/files_test.rb +++ b/apps/dashboard/test/system/files_test.rb @@ -310,4 +310,29 @@ class FilesTest < ApplicationSystemTestCase assert_selector('tbody span[title="file"]', count: 1) end end + + test 'symlinked files outside of allowlist' do + Dir.mktmpdir do |dir| + allowed_dir = "#{dir}/allowed" + with_modified_env({ OOD_ALLOWLIST_PATH: allowed_dir }) do + `mkdir -p #{allowed_dir}` + `mkdir -p #{allowed_dir}/some_dir` + `touch #{allowed_dir}/some_file` + + `mkdir -p #{dir}/not_allowed` + `ln -s #{dir}/not_allowed #{allowed_dir}/symlinked_dir` + + visit files_url(allowed_dir) + + # 3 things are actually in the directory + assert_equal(3, Dir.children(allowed_dir).size) + + # but only 2 things are shown in the UI (symlinked_dir is missing) + find('tbody a', exact_text: 'some_dir') + find('tbody a', exact_text: 'some_file') + assert_selector('tbody span[title="directory"]', count: 1) + assert_selector('tbody span[title="file"]', count: 1) + end + end + end end