diff --git a/Gemfile b/Gemfile index fdc8165..a53a068 100644 --- a/Gemfile +++ b/Gemfile @@ -1,5 +1,8 @@ source "https://rubygems.org" +gem "pstore", git: "https://github.com/nobu/pstore.git", + branch: "keyword-follow_symlink" + group :development do gem "bundler" gem "rake" diff --git a/cgi.gemspec b/cgi.gemspec index 213d377..d91cf21 100644 --- a/cgi.gemspec +++ b/cgi.gemspec @@ -33,6 +33,8 @@ Gem::Specification.new do |spec| spec.require_paths = ["lib"] + spec.add_runtime_dependency "pstore" + if Gem::Platform === spec.platform and spec.platform =~ 'java' or RUBY_ENGINE == 'jruby' spec.platform = 'java' spec.require_paths << "ext/java/org/jruby/ext/cgi/escape/lib" diff --git a/lib/cgi/session.rb b/lib/cgi/session.rb index b69857b..119720a 100644 --- a/lib/cgi/session.rb +++ b/lib/cgi/session.rb @@ -149,10 +149,20 @@ class CGI # session.close # class Session + # :stopdoc: + common_mode = File::BINARY + common_mode |= File::NOFOLLOW if File.const_defined?(:NOFOLLOW) + READ_MODE = File::RDONLY|File::SHARE_DELETE|common_mode + LOCK_MODE = File::CREAT|File::RDWR|common_mode + NEW_MODE = File::CREAT|File::EXCL|File::WRONLY|common_mode + # :startdoc: class NoSession < RuntimeError #:nodoc: end + class UnsafeSessionFileError < RuntimeError #:nodoc: + end + # The id of this session. attr_reader :session_id, :new_session @@ -230,8 +240,10 @@ def new_store_file(option={}) # :nodoc: path << digest path << suffix if suffix if File::exist? path + self.class.open_store_file(path).close hash = nil elsif new_session + File.open(path, NEW_MODE, 0600) {} hash = {} else raise NoSession, "uninitialized session" @@ -239,6 +251,20 @@ def new_store_file(option={}) # :nodoc: return path, hash end + # Windows does not conform to the POSIX permission model. + MODE_MASK = /mswin|mingw|bccwin|wince/ =~ RUBY_PLATFORM ? 0 : 0o077 # :nodoc: + private_constant :MODE_MASK + + def self.open_store_file(path, mode = READ_MODE) + f = File.open(path, mode) + stat = f.stat + unless stat.owned? and (stat.mode & MODE_MASK).zero? + f.close + raise UnsafeSessionFileError, "not owned session file" + end + f + end + # Create a new CGI::Session object for +request+. # # +request+ is an instance of the +CGI+ class (see cgi.rb). @@ -437,9 +463,10 @@ def restore unless @hash @hash = {} begin - lockf = File.open(@path+".lock", "r") + lockf = File.open(@path+".lock", READ_MODE) lockf.flock File::LOCK_SH - f = File.open(@path, 'r') + raise UnsafeSessionFileError, "not owned lock file" unless lockf.stat.owned? + f = CGI::Session.open_store_file(@path) for line in f line.chomp! k, v = line.split('=',2) @@ -457,9 +484,9 @@ def restore def update return unless @hash begin - lockf = File.open(@path+".lock", File::CREAT|File::RDWR, 0600) + lockf = File.open(@path+".lock", LOCK_MODE, 0600) lockf.flock File::LOCK_EX - f = File.open(@path+".new", File::CREAT|File::TRUNC|File::WRONLY, 0600) + f = File.open(@path+".new", NEW_MODE, 0600) for k,v in @hash f.printf "%s=%s\n", CGI.escape(k), CGI.escape(String(Marshal.dump(v))) end diff --git a/lib/cgi/session/pstore.rb b/lib/cgi/session/pstore.rb index 6e3d10f..3096981 100644 --- a/lib/cgi/session/pstore.rb +++ b/lib/cgi/session/pstore.rb @@ -24,6 +24,14 @@ class Session # library file pstore.rb. Session data is marshalled and stored # in a file. File locking and transaction services are provided. class PStore + PSTORE_OPT = {} # :nodoc: + if ::PStore.instance_method(:initialize) + .parameters + .include?([:key, :follow_symlink]) + PSTORE_OPT[:follow_symlink] = false + end + PSTORE_OPT.freeze + # Create a new CGI::Session::PStore instance # # This constructor is used internally by CGI::Session. The @@ -49,7 +57,13 @@ class PStore def initialize(session, option={}) option = {'suffix'=>''}.update(option) path, @hash = session.new_store_file(option) - @p = ::PStore.new(path) + + # In Ruby 2.6 or earlier, **{} is passed as a positional Hash. + @p = if PSTORE_OPT.empty? + ::PStore.new(path) + else + ::PStore.new(path, **PSTORE_OPT) + end @p.transaction do |p| File.chmod(0600, p.path) end diff --git a/test/cgi/test_cgi_session.rb b/test/cgi/test_cgi_session.rb index 381c149..6bdaaaf 100644 --- a/test/cgi/test_cgi_session.rb +++ b/test/cgi/test_cgi_session.rb @@ -187,8 +187,86 @@ def test_cgi_session_filestore_path assert_equal path_sha512, path end + def test_rejects_insecure_session_file + session_id = "insecure" + path = session_file_store_path("tmpdir"=>@session_dir, + "session_id"=>session_id) + File.write(path, "") + File.chmod(0644, path) + + assert_raise(CGI::Session::UnsafeSessionFileError) do + CGI::Session.new(Object.new, "tmpdir"=>@session_dir, + "session_id"=>session_id) + end + end unless CGI::Session.const_get(:MODE_MASK).zero? + + def test_filestore_update_rejects_existing_new_file + session = CGI::Session.new(Object.new, "tmpdir"=>@session_dir, + "session_id"=>"stale") + path = session.instance_variable_get(:@dbman).instance_variable_get(:@path) + new_path = path + ".new" + File.write(new_path, "existing") + session["key"] = "value" + + assert_raise(Errno::EEXIST) do + session.close + end + assert_equal("existing", File.read(new_path)) + ensure + session.delete if session + end + + def test_pstore_does_not_enable_thread_safety_for_compatibility + session = CGI::Session.new(Object.new, "tmpdir"=>@session_dir, + "session_id"=>"pstore-compat", + "database_manager"=>CGI::Session::PStore) + pstore = session.instance_variable_get(:@dbman).instance_variable_get(:@p) + + assert_equal(false, pstore.instance_variable_get(:@thread_safe)) + ensure + session.delete if session + end if defined?(::PStore) + + def test_pstore_rejects_session_file_replaced_by_symlink + omit("O_NOFOLLOW is not supported") unless nofollow_supported? + + session = CGI::Session.new(Object.new, "tmpdir"=>@session_dir, + "session_id"=>"pstore-symlink", + "database_manager"=>CGI::Session::PStore) + session["key"] = "secret" + pstore = session.instance_variable_get(:@dbman).instance_variable_get(:@p) + target = File.join(@session_dir, "target") + File.write(target, "") + File.unlink(pstore.path) + File.symlink("target", pstore.path) + + assert_raise(Errno::ELOOP) do + session.close + end + assert_empty(File.read(target)) + ensure + session.delete if session + end if defined?(::PStore) and !CGI::Session::PStore::PSTORE_OPT.empty? + private + def nofollow_supported? + return false unless File.const_defined?(:NOFOLLOW) + + target = File.join(@session_dir, "nofollow-target") + link = File.join(@session_dir, "nofollow-link") + File.write(target, "") + File.symlink(target, link) + File.open(link, File::RDONLY|File::NOFOLLOW).close + false + rescue Errno::ELOOP + true + rescue NotImplementedError, SystemCallError + false + ensure + [target, link].each {|file| File.unlink(file) if file} + end + def assert_session_filestore_path(path, dir: @session_dir, prefix: "cgi_sid_", suffix: nil) base = File.basename(path) assert_equal dir, File.dirname(path)