Fix cache vulnerability #2516
Merged
Fix cache vulnerability #2516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @jonas054! |
jonas054
force-pushed
the
2484_fix_cache_vulnerability
branch
from
8dcfd47
to
cfb9817
Compare
Do not allow any symlinks in the cache directory tree. Check before writing.
jonas054
force-pushed
the
2484_fix_cache_vulnerability
branch
from
cfb9817
to
7175d15
Compare
| @@ -84,7 +131,7 @@ def abs(path) | |||
|
|
|||
| describe '#save' do | |||
| context 'when the default internal encoding is UTF-8' do | |||
| let(:comments) { ["# Hello \xF0"] } | |||
| let(:comment_text) { '# Hello \xF0' } | |||
There was a problem hiding this comment.
Oops! Should still be double quotes here, but that makes the spec fail. I'll have to fix that.
jonas054
force-pushed
the
2484_fix_cache_vulnerability
branch
from
7175d15
to
468c402
Compare
|
Updated. |
Use JSON as serialization format instead of Marshal, which can result in execution of arbitrary code when unpacking objects that an attacker has packed and put where we expect to find cached results.
jonas054
force-pushed
the
2484_fix_cache_vulnerability
branch
from
468c402
to
4988f9b
Compare
|
Corrected typo. |
|
👍 |
urbanautomaton
added a commit
to urbanautomaton/rubocop
that referenced
this pull request
Jun 11, 2016
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
bbatsov
pushed a commit
that referenced
this pull request
Jun 11, 2016
) 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] #2484 [2] #2516 [3] #3005
Neodelf
pushed a commit
to Neodelf/rubocop
that referenced
this pull request
Oct 15, 2016
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the concerns raised in #2484. I really don't want to move the cache location, so I tried a different way.
For symlink attacks, there's no risk that the cache file itself is replaced by a symlink since we create a temporary cache file first and then rename it. There is however a chance that someone replaces a directory in the cache tree with a symlink, and we can avoid that problem by not allowing any symlinks (all the way up to the file system root directory) before writing the file. I disregard the risk of an attacker replacing a directory with a symlink just at the time between us checking and writing, since writing cache files to the wrong directory is a much smaller problem that overwriting existing files with cache data.
A Marshal attack is a more serious problem. I didn't realize this could happen, but I understand now that it can. See http://docs.ruby-lang.org/en/2.0.0/security_rdoc.html#label-Marshal.load
Replacing Marshal with JSON results in a small performance penalty when reading from the cache, around 25% on my system.