Merge branch 'upstream' into feature/update_to_upstream

* upstream:
  Version 1.7.4
  Bypass the shell in GitlabProjects
  Refactor hook creation in GitlabProjects
  Version 1.7.3
  Use Kernel#open to append lines to authorized_keys
  Version up to 1.7.2
  More detailed warning about symlinks in repos_path
  Execute command directly without using shell    use Shellwords.shellwords for splitting origin_cmd instead of .split(' ')

Conflicts:
	lib/gitlab_keys.rb
	lib/gitlab_projects.rb
	lib/gitlab_shell.rb
	spec/gitlab_keys_spec.rb
	spec/gitlab_projects_spec.rb

CHANGELOG

@@ -1,3 +1,12 @@
+v1.7.4
+  - More protection against shell injection
+
+v1.7.3
+  - Use Kernel#open to append lines to authorized_keys
+
+v1.7.2
+  - More safe command execution
+
 v1.7.1
   - Fixed issue when developers are able to push to protected branches that contain a '/' in the branch name.
 

VERSION

@@ -1 +1 @@
-1.7.1
+1.7.4

config.yml.example

@@ -12,7 +12,9 @@ http_settings:
   self_signed_cert: false
 
 # Repositories path
-# REPOS_PATH MUST NOT BE A SYMLINK!!!
+# Give the canonicalized absolute pathname,
+# REPOS_PATH MUST NOT CONTAIN ANY SYMLINK!!!
+# Check twice that none of the components is a symlink, including "/home".
 repos_path: "/home/git/repositories"
 
 # File used as authorized_keys for gitlab user

lib/gitlab_keys.rb

@@ -32,9 +32,6 @@ def exec
   def add_key
     $logger.info "Adding key #{@key_id} => #{@key.inspect}"
     cmd = "command=\"#{@config.gitlab_shell_path}/bin/gitlab-shell #{@key_id}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty #{@key}"
-    # http://blog.gitlab.org/gitlab-ce-6-2-and-5-4-security-release/
-    #cmd = "echo \'#{cmd}\' >> #{auth_file}"
-    #system(cmd)
     open(auth_file, 'a') { |file| file.puts(cmd) }
   end
 

lib/gitlab_projects.rb

@@ -51,38 +51,41 @@ def exec
   def create_branch
     branch_name = ARGV.shift
     ref = ARGV.shift || "HEAD"
-    cmd = "cd #{full_path} && git branch #{branch_name} #{ref}"
-    system(cmd)
+    cmd = %W(git --git-dir=#{full_path} branch #{branch_name} #{ref})
+    system(*cmd)
   end
 
   def rm_branch
     branch_name = ARGV.shift
-    cmd = "cd #{full_path} && git branch -D #{branch_name}"
-    system(cmd)
+    cmd = %W(git --git-dir=#{full_path} branch -D #{branch_name})
+    system(*cmd)
   end
 
   def create_tag
     tag_name = ARGV.shift
     ref = ARGV.shift || "HEAD"
-    cmd = "cd #{full_path} && git tag #{tag_name} #{ref}"
-    system(cmd)
+    cmd = %W(git --git-dir=#{full_path} tag #{tag_name} #{ref})
+    system(*cmd)
   end
 
   def rm_tag
     tag_name = ARGV.shift
-    cmd = "cd #{full_path} && git tag -d #{tag_name}"
-    system(cmd)
+    cmd = %W(git --git-dir=#{full_path} tag -d #{tag_name})
+    system(*cmd)
   end
 
   def add_project
     $logger.info "Adding project #{@project_name} at <#{full_path}>."
     FileUtils.mkdir_p(full_path, mode: 0770)
-    cmd = "cd #{full_path} && git init --bare && #{create_hooks_cmd} && chmod -R g+w ./"
-    system(cmd)
+    cmd = %W(git --git-dir=#{full_path} init --bare)
+    system(*cmd) && create_hooks(full_path)
+    FileUtils.chmod_R 0770, full_path
   end
 
-  def create_hooks_cmd
-    create_hooks_to(full_path)
+  def create_hooks(path)
+    hooks = File.join(path, 'hooks')
+    FileUtils.rm_rf(hooks) if File.exists?(hooks)
+    File.symlink(File.join(@config.gitlab_shell_path, 'hooks'), hooks)
   end
 
   def rm_project
@@ -95,8 +98,8 @@ def rm_project
   def import_project
     @source = ARGV.shift
     $logger.info "Importing project #{@project_name} from <#{@source}> to <#{full_path}>."
-    cmd = "cd #{repos_path} && git clone --bare #{@source} #{project_name} && #{create_hooks_cmd}"
-    system(cmd)
+    cmd = %W(git clone --bare #{@source} #{project_name})
+    system(*cmd, chdir: repos_path) && create_hooks(full_path)
   end
 
   # Move repository from one directory to another
@@ -171,8 +174,8 @@ def fork_project
     end
 
     $logger.info "Forking project from <#{full_path}> to <#{full_destination_path}>."
-    cmd = "cd #{namespaced_path} && git clone --bare #{full_path} && #{create_hooks_to(full_destination_path)}"
-    system(cmd)
+    cmd = %W(git clone --bare #{full_path})
+    system(*cmd, chdir: namespaced_path) && create_hooks(full_destination_path)
   end
 
   def update_head
@@ -195,11 +198,4 @@ def update_head
     $logger.info "Update head in project #{project_name} to <#{new_head}>."
     true
   end
-
-  private
-
-  def create_hooks_to(dest_path)
-    hook_path = File.join(@config.gitlab_shell_path, 'hooks')
-    "rm -rf #{dest_path}/hooks && ln -s #{hook_path} #{dest_path}/hooks"
-  end
 end

lib/gitlab_shell.rb

@@ -1,4 +1,5 @@
 require 'open3'
+require 'shellwords'
 
 require_relative 'gitlab_net'
 
@@ -40,28 +41,28 @@ def exec
   protected
 
   def parse_cmd
-    args = @origin_cmd.split(' ')
-    @git_cmd = args.shift
-    @repo_name = args.shift
+    args = Shellwords.shellwords(@origin_cmd)
+    @git_cmd = args[0]
+    @repo_name = args[1]
   end
 
   def git_cmds
     %w(git-upload-pack git-receive-pack git-upload-archive)
   end
 
   def process_cmd
-    cmd = "#{@git_cmd} #{repo_full_path}"
-    $logger.info "gitlab-shell: executing git command <#{cmd}> for #{log_username}."
-    exec_cmd(cmd)
+    repo_full_path = File.join(repos_path, repo_name)
+    $logger.info "gitlab-shell: executing git command <#{@git_cmd} #{repo_full_path}> for #{log_username}."
+    exec_cmd(@git_cmd, repo_full_path)
     update_permission_for_group
   end
 
   def validate_access
     api.allowed?(@git_cmd, @repo_name, @key_id, '_any')
   end
 
-  def exec_cmd args
-    Kernel::exec args
+  def exec_cmd *args
+    Kernel::exec *args
   end
 
   def api

spec/gitlab_keys_spec.rb

@@ -16,15 +16,18 @@
 
   describe :add_key do
     let(:gitlab_keys) { build_gitlab_keys('add-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }
+    let(:file) { mock(:file) }
 
     it "should receive valid cmd" do
-      valid_cmd = "echo 'command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-741\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E' >> #{GitlabConfig.new.auth_file}"
-      gitlab_keys.should_receive(:system).with(valid_cmd)
+      auth_line = "command=\"#{GitlabConfig.new.gitlab_shell_path}/bin/gitlab-shell key-741\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E"
+      gitlab_keys.should_receive(:open).with(GitlabConfig.new.auth_file, 'a').and_yield(file)
+      file.should_receive(:puts).with(auth_line)
       gitlab_keys.send :add_key
     end
 
     it "should log an add-key event" do
       $logger.should_receive(:info).with('Adding key key-741 => "ssh-rsa AAAAB3NzaDAxx2E"')
+      gitlab_keys.stub(:open)
       gitlab_keys.send :add_key
     end
   end

spec/gitlab_projects_spec.rb

@@ -95,15 +95,15 @@
 
     it "should create a directory" do
       gl_projects.stub(system: true)
+      gl_projects.stub(create_hooks: true)
       gl_projects.exec
       File.exists?(tmp_repo_path).should be_true
     end
 
     it "should receive valid cmd" do
-      valid_cmd = "cd #{tmp_repo_path} && git init --bare"
-      valid_cmd << " && ln -s #{GitlabConfig.new.gitlab_shell_path}/hooks/post-receive #{tmp_repo_path}/hooks/post-receive"
-      valid_cmd << " && ln -s #{GitlabConfig.new.gitlab_shell_path}/hooks/update #{tmp_repo_path}/hooks/update"
-      gl_projects.should_receive(:system).with(valid_cmd)
+      valid_cmd = ['git', "--git-dir=#{tmp_repo_path}", 'init', '--bare']
+      gl_projects.should_receive(:system).with(*valid_cmd).and_return(true)
+      gl_projects.should_receive(:create_hooks).with(tmp_repo_path)
       gl_projects.exec
     end
 

spec/gitlab_shell_spec.rb

@@ -60,7 +60,7 @@
       end
 
       it "should execute the command" do
-        subject.should_receive(:exec_cmd).with("git-upload-pack #{File.join(repository_path, 'gitlab-ci.git')}")
+        subject.should_receive(:exec_cmd).with("git-upload-pack", File.join(repository_path, 'gitlab-ci.git'))
       end
 
       it "should set the GL_ID environment variable" do
@@ -89,7 +89,7 @@
       end
 
       it "should execute the command" do
-        subject.should_receive(:exec_cmd).with("git-receive-pack #{File.join(repository_path, 'gitlab-ci.git')}")
+        subject.should_receive(:exec_cmd).with("git-receive-pack", File.join(repository_path, 'gitlab-ci.git'))
       end
 
       it "should log the command execution" do