Commit 712daa41 authored by Jacob Vosmaer's avatar Jacob Vosmaer

Limit availability of SSH_ORIGINAL_COMMAND

Hoping this makes it more obvious when code touches the very
unsafe contents of this variable.
parent c4ea06e5
...@@ -17,7 +17,7 @@ require_relative '../lib/gitlab_init' ...@@ -17,7 +17,7 @@ require_relative '../lib/gitlab_init'
# #
require File.join(ROOT_PATH, 'lib', 'gitlab_shell') 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 exit 0
else else
exit 1 exit 1
......
...@@ -11,37 +11,37 @@ class GitlabShell ...@@ -11,37 +11,37 @@ class GitlabShell
attr_accessor :key_id, :repo_name, :git_cmd, :repos_path, :repo_name 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 @key_id = key_id
@origin_cmd = origin_cmd
@config = GitlabConfig.new @config = GitlabConfig.new
@repos_path = @config.repos_path @repos_path = @config.repos_path
end end
def exec def exec(origin_cmd)
unless @origin_cmd unless origin_cmd
puts "Welcome to GitLab, #{username}!" puts "Welcome to GitLab, #{username}!"
return true return true
end end
parse_cmd args = Shellwords.shellwords(origin_cmd)
parse_cmd(args)
verify_access verify_access
process_cmd process_cmd(args)
true true
rescue GitlabNet::ApiUnreachableError => ex rescue GitlabNet::ApiUnreachableError => ex
$stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable" $stderr.puts "GitLab: Failed to authorize your Git request: internal API unreachable"
false false
rescue AccessDeniedError => ex 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 $logger.warn message
$stderr.puts "GitLab: #{ex.message}" $stderr.puts "GitLab: #{ex.message}"
false false
rescue DisallowedCommandError => ex 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 $logger.warn message
$stderr.puts "GitLab: Disallowed command" $stderr.puts "GitLab: Disallowed command"
...@@ -53,8 +53,7 @@ class GitlabShell ...@@ -53,8 +53,7 @@ class GitlabShell
protected protected
def parse_cmd def parse_cmd(args)
args = Shellwords.shellwords(@origin_cmd)
@git_cmd = args.first @git_cmd = args.first
@git_access = @git_cmd @git_access = @git_cmd
...@@ -91,13 +90,12 @@ class GitlabShell ...@@ -91,13 +90,12 @@ class GitlabShell
raise AccessDeniedError, status.message unless status.allowed? raise AccessDeniedError, status.message unless status.allowed?
end end
def process_cmd def process_cmd(args)
repo_full_path = File.join(repos_path, repo_name) repo_full_path = File.join(repos_path, repo_name)
if @git_cmd == 'git-annex-shell' if @git_cmd == 'git-annex-shell'
raise DisallowedCommandError unless @config.git_annex_enabled? raise DisallowedCommandError unless @config.git_annex_enabled?
args = Shellwords.shellwords(@origin_cmd)
parsed_args = parsed_args =
args.map do |arg| args.map do |arg|
# Convert /~/group/project.git to group/project.git # Convert /~/group/project.git to group/project.git
......
...@@ -13,7 +13,7 @@ describe GitlabShell do ...@@ -13,7 +13,7 @@ describe GitlabShell do
subject do subject do
ARGV[0] = key_id 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(exec_cmd: :exec_called)
shell.stub(api: api) shell.stub(api: api)
end end
...@@ -44,10 +44,10 @@ describe GitlabShell do ...@@ -44,10 +44,10 @@ describe GitlabShell do
describe :parse_cmd do describe :parse_cmd do
describe 'git' do describe 'git' do
context 'w/o namespace' 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 before do
subject.send :parse_cmd subject.send :parse_cmd, ssh_args
end end
its(:repo_name) { should == 'gitlab-ci.git' } its(:repo_name) { should == 'gitlab-ci.git' }
...@@ -55,10 +55,10 @@ describe GitlabShell do ...@@ -55,10 +55,10 @@ describe GitlabShell do
end end
context 'namespace' do 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 before do
subject.send :parse_cmd subject.send :parse_cmd, ssh_args
end end
its(:repo_name) { should == 'dmitriy.zaporozhets/gitlab-ci.git' } its(:repo_name) { should == 'dmitriy.zaporozhets/gitlab-ci.git' }
...@@ -66,10 +66,10 @@ describe GitlabShell do ...@@ -66,10 +66,10 @@ describe GitlabShell do
end end
context 'with an invalid number of arguments' do context 'with an invalid number of arguments' do
let(:ssh_cmd) { 'foobar' } let(:ssh_args) { %W(foobar) }
it "should raise an DisallowedCommandError" do 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 end
end end
...@@ -77,7 +77,7 @@ describe GitlabShell do ...@@ -77,7 +77,7 @@ describe GitlabShell do
describe 'git-annex' do describe 'git-annex' do
let(:repo_path) { File.join(tmp_repos_path, 'dzaporozhets/gitlab.git') } 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 before do
GitlabConfig.any_instance.stub(git_annex_enabled?: true) GitlabConfig.any_instance.stub(git_annex_enabled?: true)
...@@ -87,7 +87,7 @@ describe GitlabShell do ...@@ -87,7 +87,7 @@ describe GitlabShell do
cmd = %W(git --git-dir=#{repo_path} init --bare) cmd = %W(git --git-dir=#{repo_path} init --bare)
system(*cmd) system(*cmd)
subject.send :parse_cmd subject.send :parse_cmd, ssh_args
end end
its(:repo_name) { should == 'dzaporozhets/gitlab.git' } its(:repo_name) { should == 'dzaporozhets/gitlab.git' }
...@@ -98,7 +98,7 @@ describe GitlabShell do ...@@ -98,7 +98,7 @@ describe GitlabShell do
end end
context 'with git-annex-shell gcryptsetup' do 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 it 'should not init git-annex' do
File.exists?(File.join(tmp_repos_path, 'dzaporozhets/gitlab.git/annex')).should be_false File.exists?(File.join(tmp_repos_path, 'dzaporozhets/gitlab.git/annex')).should be_false
...@@ -110,10 +110,10 @@ describe GitlabShell do ...@@ -110,10 +110,10 @@ describe GitlabShell do
describe :exec do describe :exec do
context 'git-upload-pack' do context 'git-upload-pack' do
let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' } let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should process the command" do 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 end
it "should execute the command" do it "should execute the command" do
...@@ -135,10 +135,10 @@ describe GitlabShell do ...@@ -135,10 +135,10 @@ describe GitlabShell do
context 'git-receive-pack' do context 'git-receive-pack' do
let(:ssh_cmd) { 'git-receive-pack gitlab-ci.git' } let(:ssh_cmd) { 'git-receive-pack gitlab-ci.git' }
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should process the command" do 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 end
it "should execute the command" do it "should execute the command" do
...@@ -155,7 +155,7 @@ describe GitlabShell do ...@@ -155,7 +155,7 @@ describe GitlabShell do
context 'arbitrary command' do context 'arbitrary command' do
let(:ssh_cmd) { 'arbitrary command' } let(:ssh_cmd) { 'arbitrary command' }
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should not process the command" do it "should not process the command" do
subject.should_not_receive(:process_cmd) subject.should_not_receive(:process_cmd)
...@@ -172,7 +172,7 @@ describe GitlabShell do ...@@ -172,7 +172,7 @@ describe GitlabShell do
end end
context 'no command' do context 'no command' do
after { subject.exec } after { subject.exec(nil) }
it "should call api.discover" do it "should call api.discover" do
api.should_receive(:discover).with(key_id) api.should_receive(:discover).with(key_id)
...@@ -185,7 +185,7 @@ describe GitlabShell do ...@@ -185,7 +185,7 @@ describe GitlabShell do
before { before {
api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError) api.stub(:check_access).and_raise(GitlabNet::ApiUnreachableError)
} }
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should not process the command" do it "should not process the command" do
subject.should_not_receive(:process_cmd) subject.should_not_receive(:process_cmd)
...@@ -203,7 +203,7 @@ describe GitlabShell do ...@@ -203,7 +203,7 @@ describe GitlabShell do
GitlabConfig.any_instance.stub(git_annex_enabled?: true) GitlabConfig.any_instance.stub(git_annex_enabled?: true)
end end
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should execute the command" do 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") 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 ...@@ -213,7 +213,7 @@ describe GitlabShell do
describe :validate_access do describe :validate_access do
let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' } let(:ssh_cmd) { 'git-upload-pack gitlab-ci.git' }
after { subject.exec } after { subject.exec(ssh_cmd) }
it "should call api.check_access" do it "should call api.check_access" do
api.should_receive(:check_access). api.should_receive(:check_access).
...@@ -229,24 +229,24 @@ describe GitlabShell do ...@@ -229,24 +229,24 @@ describe GitlabShell do
end end
describe :exec_cmd do describe :exec_cmd do
let(:shell) { GitlabShell.new(key_id, ssh_cmd) } let(:shell) { GitlabShell.new(key_id) }
before { Kernel.stub!(:exec) } before { Kernel.stub!(:exec) }
it "uses Kernel::exec method" do it "uses Kernel::exec method" do
Kernel.should_receive(:exec).with(kind_of(Hash), 1, unsetenv_others: true).once Kernel.should_receive(:exec).with(kind_of(Hash), 1, 2, unsetenv_others: true).once
shell.send :exec_cmd, 1 shell.send :exec_cmd, 1, 2
end end
end end
describe :api do describe :api do
let(:shell) { GitlabShell.new(key_id, ssh_cmd) } let(:shell) { GitlabShell.new(key_id) }
subject { shell.send :api } subject { shell.send :api }
it { should be_a(GitlabNet) } it { should be_a(GitlabNet) }
end end
describe :escape_path do describe :escape_path do
let(:shell) { GitlabShell.new(key_id, ssh_cmd) } let(:shell) { GitlabShell.new(key_id) }
before { File.stub(:absolute_path) { 'y' } } before { File.stub(:absolute_path) { 'y' } }
subject { -> { shell.send(:escape_path, 'z') } } subject { -> { shell.send(:escape_path, 'z') } }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment