Symlink protection prevents use of caching in CI context

[urbanautomaton]

Hi,

Recently, protection was introduced against a symlink attack on the default Rubocop cache directory:

• Default cache directory is vulnerable to symlink attacks (Default cache directory is vulnerable to symlink attacks #2484)
• Fix cache vulnerability (Fix cache vulnerability #2516)

However, this protection has the effect that symlinks can not be used for any cache location, even if the user has specified a non-default location in their config.

This is a particular problem on CI, for example, where cache directories are often symlinked to a shared filesystem so that they can be persisted between builds.

I would be happy to submit a PR to address this, but wanted to check first that my "expected behaviour" below is agreeable, or if another approach is preferred.

Expected behavior

Rubocop allows a cache directory that is a symlink, if the CacheRootDirectory is user-defined.

Actual behavior

Configure CacheRootDirectory to, e.g. mycache. If it, or any of its ancestors, is a symlink, Rubocop warns: Warning: mycache is a symlink, which is not allowed. and does not cache results.

Steps to reproduce the problem

gem install rubocop

mkdir real_cache

ln -sf real_cache symlink_cache

cat >.rubocop.yml <<EOS
AllCops:
  CacheRootDirectory: symlink_cache
EOS

cat >blah.rb <<EOS
class A; end
EOS

rubocop blah.rb

RuboCop version

$ rubocop -V
0.39.0 (using Parser 2.3.0.7, running on ruby 2.2.4 x86_64-darwin15)

[jonas054]

What does @aripollak think? Is it acceptable to allow symlinks in user-specified paths?

[aripollak]

Well, it's still possible for a user to configure the directory to be somewhere else in /tmp or another tmp-like directory, which doesn't seem so unlikely to me.

[jonas054]

Maybe if we add a configuration parameter AllowSymlinksInCacheRootDirectory: false, and describe the risks associated with changing it to true in config/default.yml.

[urbanautomaton]

Well, it's still possible for a user to configure the directory to be somewhere else in /tmp or another tmp-like directory, which doesn't seem so unlikely to me.

Hmm - I do wonder to what extent it's Rubocop's job to protect its users against generic attacks like this, when they're a result of the user's own configuration. There are any number of unsafe states I might leave my systems in, but I don't think it's Rubocop's responsibility to run heuristics to try to detect these, particularly if the heuristics have false positives that prevent normal filesystem practices.

I understand the motivation to protect users from the original, unsafe default value, but my feeling is that once the user has configured their cache directory themselves, it's up to them to ensure that its location is secure.

Maybe if we add a configuration parameter AllowSymlinksInCacheRootDirectory: false, and describe the risks associated with changing it to true in config/default.yml.

This seems like rather a lot of complexity to guard against what appears to be a very niche attack vector in the first place. I would probably favour adding some explanatory text to the warning message triggered when a symlink is detected in the default case, e.g.:

Warning: /tmp/cache is a symlink, which is not allowed. If your cache directory is
world-writable, it can be possible in some circumstances for an attacker to cause
rubocop to overwrite files using the current user's privileges.

If you are sure that your cache location is secure from this kind of attack, you can
explicitly set it in your configuration file:

AllCops:
  CacheRootDirectory: /tmp/cache

[urbanautomaton]

To summarise some of the various ways we could approach this:

1. Do nothing - you can't cache to a symlinked directory, full stop
2. Allow any cache location if it is user-specified
3. Add an AllowSymlinksInCacheRootDirectory setting to disable the safety check, defaulting to false
4. Expand detection heuristics somehow (checking for world-writable status, too?)

Since the cache feature is essentially unusable on SaaS CI products at the moment, I'd like to avoid 1). I'd be very happy to implement either of 2) or 3), if either of those is preferred. I don't know how I'd start going about 4), and suspect that we would still end up with a false-positive problem, so am reluctant to take that route (but would welcome input if there's a clear way of doing this).

I'll start work on a PR shortly - are there any strong opinions on which approach we should take?

[jonas054]

I'm fine with either 2 (with the expanded warning message you proposed) or 3.

[urbanautomaton]

Thanks @jonas054 - PR incoming. 👍

[mikegee]

Is this resolved now that #3199 is merged?

[bbatsov]

Guess so.