BigW Consortium Gitlab

shell_commands.md 3.59 KB
Newer Older
Jacob Vosmaer committed
1 2
# Guidelines for shell commands in the GitLab codebase

3 4 5 6
## References

- [Google Ruby Security Reviewer's Guide](https://code.google.com/p/ruby-security/wiki/Guide)
- [OWASP Command Injection](https://www.owasp.org/index.php/Command_Injection)
dosire committed
7
- [Ruby on Rails Security Guide Command Line Injection](http://guides.rubyonrails.org/security.html#command-line-injection)
8

Jacob Vosmaer committed
9 10
## Use File and FileUtils instead of shell commands

11
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>
Jacob Vosmaer committed
12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30

```ruby
# Wrong
system "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"

# Wrong
contents = `cat #{filename}`
# Correct
contents = File.read(filename)
```

This coding style could have prevented CVE-2013-4490.

## Bypass the shell by splitting commands into separate tokens

31
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
Jacob Vosmaer committed
32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50

```ruby
# Wrong
system "cd /home/git/gitlab && bundle exec rake db:#{something} RAILS_ENV=production"
# Correct
system({'RAILS_ENV' => 'production'}, *%W(bundle exec rake db:#{something}), chdir: '/home/git/gitlab')

# Wrong
system "touch #{myfile}"
# Better
system "touch", myfile
# Best (do not run a shell command at all)
FileUtils.touch myfile
```

This coding style could have prevented CVE-2013-4546.

## Separate options from arguments with --

51
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.
Jacob Vosmaer committed
52 53 54 55 56 57 58 59 60 61 62

To understand what `--` does, consider the problem below.

```
# Example
$ echo hello > -l
$ cat -l
cat: illegal option -- l
usage: cat [-benstuv] [file ...]
```

63
In the example above, the argument parser of `cat` assumes that `-l` is an option. The solution in the example above is to make it clear to `cat` that `-l` is really an argument, not an option. Many Unix command line tools follow the convention of separating options from arguments with `--`.
Jacob Vosmaer committed
64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83

```
# Example (continued)
$ cat -- -l
hello
```

In the GitLab codebase, we avoid the option/argument ambiguity by _always_ using `--`.

```ruby
# Wrong
system(*%W(git branch -d #{branch_name}))
# Correct
system(*%W(git branch -d -- #{branch_name}))
```

This coding style could have prevented CVE-2013-4582.

## Do not use the backticks

84
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 use `Gitlab::Popen.popen` instead.
Jacob Vosmaer committed
85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105

```ruby
# Wrong
logs = `cd #{repo_dir} && git log`
# Correct
logs, exit_status = Gitlab::Popen.popen(%W(git log), repo_dir)

# Wrong
user = `whoami`
# Correct
user, exit_status = Gitlab::Popen.popen(%W(whoami))
```

In other repositories, such as gitlab-shell you can also use `IO.popen`.

```ruby
# Safe IO.popen example
logs = IO.popen(%W(git log), chdir: repo_dir).read
```

Note that unlike `Gitlab::Popen.popen`, `IO.popen` does not capture standard error.