Merge branch 'doc/file_read' into 'master'
Add security tips about file and paths These are some extra guidelines to guard against code execution and path traversal. See merge request !1317
This commit is contained in:
commit
3febfce1ae
|
@ -1,5 +1,8 @@
|
||||||
# Guidelines for shell commands in the GitLab codebase
|
# 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
|
## References
|
||||||
|
|
||||||
- [Google Ruby Security Reviewer's Guide](https://code.google.com/p/ruby-security/wiki/Guide)
|
- [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.
|
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