Harden file-backed session storage

Gemfile

@@ -1,5 +1,8 @@
 source "https://rubygems.org"
 
+gem "pstore", git: "https://github.com/nobu/pstore.git",
+              ref: "1d7b517a700addb0839c3472419203e764243160"
+
 group :development do
   gem "bundler"
   gem "rake"

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"

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,15 +240,31 @@ 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"
       end
       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

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

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)