Allow symlinks in cache directory if explicitly enabled in config (ru…

…bocop#3199)

The default location for RuboCop's result cache is `/tmp`, which on the vast
majority of systems is a world-writable directory. This means that a malicious
user might be able to create a symlink to either redirect RuboCop's output to
an unintended location, or cause RuboCop to read malicious input.

Previous work[1,2] introduced protection against such symlink attacks by
symlinks to be present in any cache locations.

However, often CI setups explicitly rely on the ability to symlink cache
locations, so that persistent results can be placed in shared storage, and
symlinked to a predictable location on build machines[3].

This commit adds a new configuration option,
`AllowSymlinksInCacheRootDirectory`, which lets a user permit symlinks if they
are certain that their cache location is secure.

[1] rubocop#2484
[2] rubocop#2516
[3] rubocop#3005

CHANGELOG.md

@@ -18,6 +18,7 @@
 
 ### Bug fixes
 
+* [#3005](https://github.com/bbatsov/rubocop/issues/3005): Symlink protection prevents use of caching in CI context. ([@urbanautomaton][])
 * [#3037](https://github.com/bbatsov/rubocop/issues/3037): `Style/StringLiterals` understands that a bare '#', not '#@variable' or '#{interpolation}', does not require double quotes. ([@alexdowad][])
 * [#2722](https://github.com/bbatsov/rubocop/issues/2722): `Style/ExtraSpacing` does not attempt to align an equals sign in an argument list with one in an assignment statement. ([@alexdowad][])
 * [#3133](https://github.com/bbatsov/rubocop/issues/3133): `Style/MultilineMethodCallBraceLayout` does not register offenses for single-line calls. ([@alexdowad][])

config/default.yml

@@ -65,6 +65,14 @@ AllCops:
   # which is "/tmp" on Unix-like systems, but could be something else on other
   # systems.
   CacheRootDirectory: /tmp
+  # The default cache root directory is /tmp, which on most systems is
+  # writable by any system user. This means that it is possible for a
+  # malicious user to anticipate the location of Rubocop's cache directory,
+  # and create a symlink in its place that could cause Rubocop to overwrite
+  # unintended files, or read malicious input. If you are certain that your
+  # cache location is secure from this kind of attack, and wish to use a
+  # symlinked cache location, set this value to "true".
+  AllowSymlinksInCacheRootDirectory: false
   # What version of the Ruby interpreter is the inspected code intended to
   # run on? (If there is more than one, set this to the lowest version.)
   TargetRubyVersion: 2.0

lib/rubocop/result_cache.rb

@@ -64,8 +64,14 @@ def self.cache_root(config_store)
       File.join(root, 'rubocop_cache')
     end
 
+    def self.allow_symlinks_in_cache_location?(config_store)
+      config_store.for('.').for_all_cops['AllowSymlinksInCacheRootDirectory']
+    end
+
     def initialize(file, options, config_store, cache_root = nil)
       cache_root ||= ResultCache.cache_root(config_store)
+      @allow_symlinks_in_cache_location =
+        ResultCache.allow_symlinks_in_cache_location?(config_store)
       @path = File.join(cache_root, rubocop_checksum,
                         relevant_options_digest(options),
                         file_checksum(file, config_store))
@@ -85,9 +91,9 @@ def save(offenses)
       FileUtils.mkdir_p(dir)
       preliminary_path = "#{@path}_#{rand(1_000_000_000)}"
       # RuboCop must be in control of where its cached data is stored. A
-      # symbolic link anywhere in the cache directory tree is an indication
-      # that a symlink attack is being waged.
-      return if any_symlink?(dir)
+      # symbolic link anywhere in the cache directory tree can be an
+      # indication that a symlink attack is being waged.
+      return if symlink_protection_triggered?(dir)
 
       File.open(preliminary_path, 'wb') do |f|
         f.write(@cached_data.to_json(offenses))
@@ -102,6 +108,10 @@ def save(offenses)
 
     private
 
+    def symlink_protection_triggered?(path)
+      !@allow_symlinks_in_cache_location && any_symlink?(path)
+    end
+
     def any_symlink?(path)
       while path != File.dirname(path)
         if File.symlink?(path)

spec/rubocop/result_cache_spec.rb

@@ -11,7 +11,7 @@
   end
   let(:file) { 'example.rb' }
   let(:options) { {} }
-  let(:config_store) { double('config_store') }
+  let(:config_store) { double('config_store', for: RuboCop::Config.new) }
   let(:cache_root) { "#{Dir.pwd}/rubocop_cache" }
   let(:offenses) do
     [RuboCop::Cop::Offense.new(:warning, location, 'unused var',
@@ -60,32 +60,62 @@ def abs(path)
         end
       end
 
-      context 'when a symlink attack is made' do
+      context 'when a symlink is present in the cache location' do
+        let(:cache2) do
+          described_class.new(file, options, config_store, cache_root)
+        end
+
         before(:each) do
           # Avoid getting "symlink() function is unimplemented on this
           # machine" on Windows.
           if RUBY_PLATFORM =~ /cygwin|mswin|mingw|bccwin|wince|emx/
             skip 'Symlinks not implemented on Windows'
           end
 
+          @attack_target_dir = Dir.mktmpdir
+
           cache.save(offenses)
           Find.find(cache_root) do |path|
             next unless File.basename(path) == '_'
             FileUtils.rm_rf(path)
-            FileUtils.ln_s('/bin', path)
+            FileUtils.ln_s(@attack_target_dir, path)
           end
           $stderr = StringIO.new
         end
-        after(:each) { $stderr = STDERR }
+        after(:each) do
+          FileUtils.rmdir(@attack_target_dir)
+          $stderr = STDERR
+        end
 
-        it 'is stopped' do
-          cache2 = described_class.new(file, options, config_store, cache_root)
-          cache2.save(offenses)
-          # The cache file has not been created because there was a symlink in
-          # its path.
-          expect(cache2.valid?).to eq(false)
-          expect($stderr.string)
-            .to match(/Warning: .* is a symlink, which is not allowed.\n/)
+        context 'and symlink attack protection is enabled' do
+          it 'prevents caching and prints a warning' do
+            cache2.save(offenses)
+            # The cache file has not been created because there was a symlink in
+            # its path.
+            expect(cache2.valid?).to eq(false)
+            expect($stderr.string)
+              .to match(/Warning: .* is a symlink, which is not allowed.\n/)
+          end
+        end
+
+        context 'and symlink attack protection is disabled' do
+          before do
+            allow(config_store).to receive(:for).with('.').and_return(
+              RuboCop::Config[
+                'AllCops' => {
+                  'AllowSymlinksInCacheRootDirectory' => true
+                }
+              ]
+            )
+          end
+
+          it 'permits caching and prints no warning' do
+            cache2.save(offenses)
+
+            expect(cache2.valid?).to eq(true)
+            expect($stderr.string)
+              .not_to match(/Warning: .* is a symlink, which is not allowed.\n/)
+          end
         end
       end
     end