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 reliableandsecure.
References
- Google Ruby Security Reviewer's Guide
- OWASP Command Injection
- Ruby on Rails Security Guide Command Line Injection
Use File and FileUtils instead of shell commands
Sometimes we invoke basic Unix commands via the shell when there is also a Ruby API for doing it. Use the Ruby API if it exists.http://www.ruby-doc.org/stdlib-2.0.0/libdoc/fileutils/rdoc/FileUtils.html#module-FileUtils-label-Module+Functions
# Wrongsystem"mkdir -p tmp/special/directory"# Better (separate tokens)system*%W(mkdir -p tmp/special/directory)# Best (do not use a shell command)FileUtils.mkdir_p"tmp/special/directory"# Wrongcontents=“猫#{filename}`# Correctcontents=File.read(filename)# Sometimes a shell command is just the best solution. The example below has no# user input, and is hard to implement correctly in Ruby: delete all files and# directories older than 120 minutes under /some/path, but not /some/path# itself.Gitlab::Popen.popen(%W(find /some/path -not -path /some/path -mmin +120 -delete))
This coding style could have prevented CVE-2013-4490.
Always use the configurable git binary path for git commands
# Wrongsystem(*%W(git branch -d --#{branch_name}))# Correctsystem(*%W(#{Gitlab.config.git.bin_path}branch -d --#{branch_name}))
Bypass the shell by splitting commands into separate tokens
When we pass shell commands as a single string to Ruby, Ruby will let/bin/sh
evaluate the entire string. Essentially, we are asking the shell to evaluate a one-line script. This creates a risk for shell injection attacks. It is better to split the shell command into tokens ourselves. Sometimes we use the scripting capabilities of the shell to change the working directory or set environment variables. All of this can also be achieved securely straight from Ruby
# Wrongsystem"cd /home/git/gitlab && bundle exec rake db:#{something}RAILS_ENV=production"# Correctsystem({'RAILS_ENV'=>'production'},*%W(bundle exec rake db:#{something}),chdir:'/home/git/gitlab')# Wrongsystem"touch#{myfile}"# Bettersystem"touch",myfile# Best (do not run a shell command at all)FileUtils.touchmyfile
This coding style could have prevented CVE-2013-4546.
Separate options from arguments with --
Make the difference between options and arguments clear to the argument parsers of system commands with--
. This is supported by many but not all Unix commands.
To understand what--
does, consider the problem below.
# Example$ echo hello > -l$ cat -lcat: illegal option -- lusage: cat [-benstuv] [file ...]
In the example above, the argument parser ofcat
assumes that-l
is an option. The solution in the example above is to make it clear tocat
that-l
是一个论点,而不是一个选择。很多你nix command line tools follow the convention of separating options from arguments with--
.
# Example (continued)$ cat -- -lhello
In the GitLab codebase, we avoid the option/argument ambiguity byalwaysusing--
.
# Wrongsystem(*%W(#{Gitlab.config.git.bin_path}branch -d#{branch_name}))# Correctsystem(*%W(#{Gitlab.config.git.bin_path}branch -d --#{branch_name}))
This coding style could have prevented CVE-2013-4582.
Do not use the backticks
Capturing the output of shell commands with backticks reads nicely, but you are forced to pass the command as one string to the shell. We explained above that this is unsafe. In the main GitLab codebase, the solution is to useGitlab::Popen.popen
instead.
# Wronglogs=`cd#{repo_dir}&&#{Gitlab.config.git.bin_path}log`# Correctlogs,exit_status=Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path}log),repo_dir)# Wronguser=`whoami`# Correctuser,exit_status=Gitlab::Popen.popen(%W(whoami))
In other repositories, such as gitlab-shell you can also useIO.popen
.
# Safe IO.popen examplelogs=IO.popen(%W(#{Gitlab.config.git.bin_path}log),chdir:repo_dir){|p|p.read}
Note that unlikeGitlab::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)
If you have to use user input a relative path, prefix./
to the path.
Prefixing user-supplied paths also offers extra protection against paths starting with-
(see the discussion about using--
above).
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 ususer_input='../other-repo.git/other-file'# We look up the repo path somewhererepo_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'sFile.absolute_path
.
full_path=File.join(repo_path,user_input)iffull_path!=File.absolute_path(full_path)raise"Invalid path:#{full_path.inspect}"endFile.open(full_path)do# Etc.
A check like this could have avoided CVE-2013-4583.
适当的锚圣正则表达式art and end of strings
When using regular expressions to validate user input that is passed as an argument to a shell command, make sure to use the\A
and\z
anchors that designate the start and end of the string, rather than^
and$
, or no anchors at all.
If you don't, an attacker could use this to execute commands with potentially harmful effect.
For example, when a project'simport_url
is validated like below, the user could trick GitLab into cloning from a Git repository on the local filesystem.
validates:import_url,format:{with:URI.regexp(%w(ssh git http https))}# URI.regexp(%w(ssh git http https)) roughly evaluates to /(ssh|git|http|https):(something_that_looks_like_a_url)/
Suppose the user submits the following as their import URL:
file://git:/tmp/lol
Since there are no anchors in the used regular expression, thegit:/tmp/lol
in the value would match, and the validation would pass.
When importing, GitLab would execute the following command, passing theimport_url
as an argument:
git clone file://git:/tmp/lol
Git would simply ignore thegit:
part, interpret the path asfile:///tmp/lol
and import the repository into the new project, in turn potentially giving the attacker access to any repository in the system, whether private or not.