Merge branch 'stricter-exec_cmd' into 'master'

Stricter exec cmd

In response to the gitlab-shell 2.6.6-2.6.7 remote code execution
vulnerability.

See merge request !33

3 files changed, 54 insertions, 37 deletions

diff --git a/bin/gitlab-shell b/bin/gitlab-shell
index 084f0c9..f145a1b 100755
--- a/bin/gitlab-shell
+++ b/bin/gitlab-shell
@@ -17,7 +17,7 @@ require_relative '../lib/gitlab_init'
 #
 require File.join(ROOT_PATH, 'lib', 'gitlab_shell')
 
-if GitlabShell.new(key_id, original_cmd).exec
+if GitlabShell.new(key_id).exec(original_cmd)
   exit 0
 else
   exit 1
diff --git a/lib/gitlab_shell.rb b/lib/gitlab_shell.rb
index 4890e99..96ee1b7 100644
--- a/lib/gitlab_shell.rb
+++ b/lib/gitlab_shell.rb
@@ -11,37 +11,40 @@ class GitlabShell
 
   attr_accessor :key_id, :repo_name, :git_cmd, :repos_path, :repo_name
 
-  def initialize(key_id, origin_cmd)
+  def initialize(key_id)
     @key_id = key_id
-    @origin_cmd = origin_cmd
     @config = GitlabConfig.new
     @repos_path = @config.repos_path
   end
 
-  def exec
-    unless @origin_cmd
+  # The origin_cmd variable contains UNTRUSTED input. If the user ran
+  # ssh git@gitlab.example.com 'evil command', then origin_cmd contains
+  # 'evil command'.
+  def exec(origin_cmd)
+    unless origin_cmd
       puts "Welcome to GitLab, #{username}!"
       return true
     end
 
-    parse_cmd
+    args = Shellwords.shellwords(origin_cmd)
+    parse_cmd(args)
 
     verify_access
 
-    process_cmd
+    process_cmd(args)
 
     true
   rescue GitlabNet::ApiUnreachableError => ex
     $stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable"
     false
   rescue AccessDeniedError => ex
-    message = "gitlab-shell: Access denied for git command <#{@origin_cmd}> by #{log_username}."
+    message = "gitlab-shell: Access denied for git command <#{origin_cmd}> by #{log_username}."
     $logger.warn message
 
     $stderr.puts "GitLab: #{ex.message}"
     false
   rescue DisallowedCommandError => ex
-    message = "gitlab-shell: Attempt to execute disallowed command <#{@origin_cmd}> by #{log_username}."
+    message = "gitlab-shell: Attempt to execute disallowed command <#{origin_cmd}> by #{log_username}."
     $logger.warn message
 
     $stderr.puts "GitLab: Disallowed command"
@@ -53,8 +56,7 @@ class GitlabShell
 
   protected
 
-  def parse_cmd
-    args = Shellwords.shellwords(@origin_cmd)
+  def parse_cmd(args)
     @git_cmd = args.first
     @git_access = @git_cmd
 
@@ -91,13 +93,12 @@ class GitlabShell
     raise AccessDeniedError, status.message unless status.allowed?
   end
 
-  def process_cmd
+  def process_cmd(args)
     repo_full_path = File.join(repos_path, repo_name)
 
     if @git_cmd == 'git-annex-shell'
       raise DisallowedCommandError unless @config.git_annex_enabled?
 
-      args = Shellwords.shellwords(@origin_cmd)
       parsed_args =
         args.map do |arg|
           # Convert /~/group/project.git to group/project.git
@@ -119,6 +120,13 @@ class GitlabShell
 
   # This method is not covered by Rspec because it ends the current Ruby process.
   def exec_cmd(*args)
+    # If you want to call a command without arguments, use
+    # exec_cmd(['my_command', 'my_command']) . Otherwise use
+    # exec_cmd('my_command', 'my_argument', ...).
+    if args.count == 1 && !args.first.is_a?(Array)
+      raise DisallowedCommandError
+    end
+
     env = {
       'HOME' => ENV['HOME'],
       'PATH' => ENV['PATH'],
diff --git a/spec/gitlab_shell_spec.rb b/spec/gitlab_shell_spec.rb
index 62e0d36..86d72f4 100644
--- a/spec/gitlab_shell_spec.rb
+++ b/spec/gitlab_shell_spec.rb
@@ -13,7 +13,7 @@ describe GitlabShell do
 
   subject do
     ARGV[0] = key_id
-    GitlabShell.new(key_id, ssh_cmd).tap do |shell|
+    GitlabShell.new(key_id).tap do |shell|
       shell.stub(exec_cmd: :exec_called)
       shell.stub(api: api)
     end
@@ -44,10 +44,10 @@ describe GitlabShell do
   describe :parse_cmd do
     describe 'git' do
       context 'w/o namespace' do
-        let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
+        let(:ssh_args) { %W(git-upload-pack gitlab-ci.git) }
 
         before do
-          subject.send :parse_cmd
+          subject.send :parse_cmd, ssh_args
         end
 
         its(:repo_name) { should == 'gitlab-ci.git' }
@@ -55,10 +55,10 @@ describe GitlabShell do
       end
 
       context 'namespace' do
-        let(:ssh_cmd) { 'git-upload-pack dmitriy.zaporozhets/gitlab-ci.git' }
+        let(:ssh_args) { %W(git-upload-pack dmitriy.zaporozhets/gitlab-ci.git) }
 
         before do
-          subject.send :parse_cmd
+          subject.send :parse_cmd, ssh_args
         end
 
         its(:repo_name) { should == 'dmitriy.zaporozhets/gitlab-ci.git' }
@@ -66,10 +66,10 @@ describe GitlabShell do
       end
 
       context 'with an invalid number of arguments' do
-        let(:ssh_cmd) { 'foobar' }
+        let(:ssh_args) { %W(foobar) }
 
         it "should raise an DisallowedCommandError" do
-          expect { subject.send :parse_cmd }.to raise_error(GitlabShell::DisallowedCommandError)
+          expect { subject.send :parse_cmd, ssh_args }.to raise_error(GitlabShell::DisallowedCommandError)
         end
       end
     end
@@ -77,7 +77,7 @@ describe GitlabShell do
     describe 'git-annex' do
       let(:repo_path) { File.join(tmp_repos_path, 'dzaporozhets/gitlab.git') }
 
-      let(:ssh_cmd) { 'git-annex-shell inannex /~/dzaporozhets/gitlab.git SHA256E' }
+      let(:ssh_args) { %W(git-annex-shell inannex /~/dzaporozhets/gitlab.git SHA256E) }
 
       before do
         GitlabConfig.any_instance.stub(git_annex_enabled?: true)
@@ -87,7 +87,7 @@ describe GitlabShell do
         cmd = %W(git --git-dir=#{repo_path} init --bare)
         system(*cmd)
 
-        subject.send :parse_cmd
+        subject.send :parse_cmd, ssh_args
       end
 
       its(:repo_name) { should == 'dzaporozhets/gitlab.git' }
@@ -98,7 +98,7 @@ describe GitlabShell do
       end
 
       context 'with git-annex-shell gcryptsetup' do
-        let(:ssh_cmd) { 'git-annex-shell gcryptsetup /~/dzaporozhets/gitlab.git' }
+        let(:ssh_args) { %W(git-annex-shell gcryptsetup /~/dzaporozhets/gitlab.git) }
 
         it 'should not init git-annex' do
           File.exists?(File.join(tmp_repos_path, 'dzaporozhets/gitlab.git/annex')).should be_false
@@ -110,10 +110,10 @@ describe GitlabShell do
   describe :exec do
     context 'git-upload-pack' do
       let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
-      after { subject.exec }
+      after { subject.exec(ssh_cmd) }
 
       it "should process the command" do
-        subject.should_receive(:process_cmd).with()
+        subject.should_receive(:process_cmd).with(%W(git-upload-pack gitlab-ci.git))
       end
 
       it "should execute the command" do
@@ -135,10 +135,10 @@ describe GitlabShell do
 
     context 'git-receive-pack' do
       let(:ssh_cmd) { 'git-receive-pack gitlab-ci.git' }
-      after { subject.exec }
+      after { subject.exec(ssh_cmd) }
 
       it "should process the command" do
-        subject.should_receive(:process_cmd).with()
+        subject.should_receive(:process_cmd).with(%W(git-receive-pack gitlab-ci.git))
       end
 
       it "should execute the command" do
@@ -155,7 +155,7 @@ describe GitlabShell do
 
     context 'arbitrary command' do
       let(:ssh_cmd) { 'arbitrary command' }
-      after { subject.exec }
+      after { subject.exec(ssh_cmd) }
 
       it "should not process the command" do
         subject.should_not_receive(:process_cmd)
@@ -172,7 +172,7 @@ describe GitlabShell do
     end
 
     context 'no command' do      
-      after { subject.exec }
+      after { subject.exec(nil) }
 
       it "should call api.discover" do
         api.should_receive(:discover).with(key_id)
@@ -185,7 +185,7 @@ describe GitlabShell do
       before {
         api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError)
       }
-      after { subject.exec }
+      after { subject.exec(ssh_cmd) }
 
       it "should not process the command" do
         subject.should_not_receive(:process_cmd)
@@ -203,7 +203,7 @@ describe GitlabShell do
         GitlabConfig.any_instance.stub(git_annex_enabled?: true)
       end
 
-      after { subject.exec }
+      after { subject.exec(ssh_cmd) }
 
       it "should execute the command" do
         subject.should_receive(:exec_cmd).with("git-annex-shell", "commit", File.join(tmp_repos_path, 'gitlab-ci.git'), "SHA256")
@@ -213,7 +213,7 @@ describe GitlabShell do
 
   describe :validate_access do
     let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
-    after { subject.exec }
+    after { subject.exec(ssh_cmd) }
 
     it "should call api.check_access" do
       api.should_receive(:check_access).
@@ -229,24 +229,33 @@ describe GitlabShell do
   end
 
   describe :exec_cmd do
-    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
+    let(:shell) { GitlabShell.new(key_id) }
     before { Kernel.stub!(:exec) }
 
     it "uses Kernel::exec method" do
-      Kernel.should_receive(:exec).with(kind_of(Hash), 1, unsetenv_others: true).once
-      shell.send :exec_cmd, 1
+      Kernel.should_receive(:exec).with(kind_of(Hash), 1, 2, unsetenv_others: true).once
+      shell.send :exec_cmd, 1, 2
+    end
+
+    it "refuses to execute a lone non-array argument" do
+      expect { shell.send :exec_cmd, 1 }.to raise_error(GitlabShell::DisallowedCommandError)
+    end
+
+    it "allows one argument if it is an array" do
+      Kernel.should_receive(:exec).with(kind_of(Hash), [1, 2], unsetenv_others: true).once
+      shell.send :exec_cmd, [1, 2]
     end
   end
 
   describe :api do
-    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
+    let(:shell) { GitlabShell.new(key_id) }
     subject { shell.send :api }
 
     it { should be_a(GitlabNet) }
   end
 
   describe :escape_path do
-    let(:shell) { GitlabShell.new(key_id, ssh_cmd) }
+    let(:shell) { GitlabShell.new(key_id) }
     before { File.stub(:absolute_path) { 'y' } }
     subject { -> { shell.send(:escape_path, 'z') } }