Add security tips about file and paths
This commit is contained in:
parent
c7db2661a5
commit
82eb0a44d7
|
@ -1,5 +1,8 @@
|
|||
# Guidelines for shell commands in the GitLab codebase
|
||||
|
||||
This document contains guidelines for working with processes and files in the GitLab codebase.
|
||||
These guidelines are meant to make your code more reliable _and_ secure.
|
||||
|
||||
## References
|
||||
|
||||
- [Google Ruby Security Reviewer's Guide](https://code.google.com/p/ruby-security/wiki/Guide)
|
||||
|
@ -109,3 +112,63 @@ logs = IO.popen(%W(git log), chdir: repo_dir).read
|
|||
```
|
||||
|
||||
Note that unlike `Gitlab::Popen.popen`, `IO.popen` does not capture standard error.
|
||||
|
||||
## Avoid user input at the start of path strings
|
||||
|
||||
Various methods for opening and reading files in Ruby can be used to read the
|
||||
standard output of a process instead of a file. The following two commands do
|
||||
roughly the same:
|
||||
|
||||
```
|
||||
`touch /tmp/pawned-by-backticks`
|
||||
File.read('|touch /tmp/pawned-by-file-read')
|
||||
```
|
||||
|
||||
The key is to open a 'file' whose name starts with a `|`.
|
||||
Affected methods include Kernel#open, File::read, File::open, IO::open and IO::read.
|
||||
|
||||
You can protect against this behavior of 'open' and 'read' by ensuring that an
|
||||
attacker cannot control the start of the filename string you are opening. For
|
||||
instance, the following is sufficient to protect against accidentally starting
|
||||
a shell command with `|`:
|
||||
|
||||
```
|
||||
# we assume repo_path is not controlled by the attacker (user)
|
||||
path = File.join(repo_path, user_input)
|
||||
# path cannot start with '|' now.
|
||||
File.read(path)
|
||||
```
|
||||
|
||||
## Guard against path traversal
|
||||
|
||||
Path traversal is a security where the program (GitLab) tries to restrict user
|
||||
access to a certain directory on disk, but the user manages to open a file
|
||||
outside that directory by taking advantage of the `../` path notation.
|
||||
|
||||
```
|
||||
# Suppose the user gave us a path and they are trying to trick us
|
||||
user_input = '../other-repo.git/other-file'
|
||||
|
||||
# We look up the repo path somewhere
|
||||
repo_path = 'repositories/user-repo.git'
|
||||
|
||||
# The intention of the code below is to open a file under repo_path, but
|
||||
# because the user used '..' she can 'break out' into
|
||||
# 'repositories/other-repo.git'
|
||||
full_path = File.join(repo_path, user_input)
|
||||
File.open(full_path) do # Oops!
|
||||
```
|
||||
|
||||
A good way to protect against this is to compare the full path with its
|
||||
'absolute path' according to Ruby's `File.absolute_path`.
|
||||
|
||||
```
|
||||
full_path = File.join(repo_path, user_input)
|
||||
if full_path != File.absolute_path(full_path)
|
||||
raise "Invalid path: #{full_path.inspect}"
|
||||
end
|
||||
|
||||
File.open(full_path) do # Etc.
|
||||
```
|
||||
|
||||
A check like this could have avoided CVE-2013-4583.
|
||||
|
|
Loading…
Reference in New Issue