Address command line injection in Git::Lib#fetch

jcouball · jcouball · commit 291ca0946bec · 2022-04-13T10:32:28.000-07:00
Signed-off-by: James Couball &lt;jcouball@yahoo.com&gt;
diff --git a/lib/git/lib.rb b/lib/git/lib.rb
@@ -875,14 +875,15 @@ def tag(name, *opts)
       command('tag', arr_opts)
     end
 
-
     def fetch(remote, opts)
-      arr_opts = [remote]
-      arr_opts << opts[:ref] if opts[:ref]
+      arr_opts = []
       arr_opts << '--tags' if opts[:t] || opts[:tags]
       arr_opts << '--prune' if opts[:p] || opts[:prune]
       arr_opts << '--unshallow' if opts[:unshallow]
       arr_opts << '--depth' << opts[:depth] if opts[:depth]
+      arr_opts << '--'
+      arr_opts << remote
+      arr_opts << opts[:ref] if opts[:ref]
 
       command('fetch', arr_opts)
     end
diff --git a/tests/units/test_remotes.rb b/tests/units/test_remotes.rb
@@ -23,29 +23,29 @@ def test_add_remote
       assert(local.remotes.map{|b| b.name}.include?('testremote2'))
 
       local.add_remote('testremote3', remote, :track => 'master')
-      
-      assert(local.branches.map{|b| b.full}.include?('master')) #We actually a new branch ('test_track') on the remote and track that one intead. 
+
+      assert(local.branches.map{|b| b.full}.include?('master')) #We actually a new branch ('test_track') on the remote and track that one intead.
       assert(local.remotes.map{|b| b.name}.include?('testremote3'))
-    end 
+    end
   end
 
   def test_remove_remote_remove
     in_temp_dir do |path|
       local = Git.clone(@wbare, 'local')
       remote = Git.clone(@wbare, 'remote')
-      
+
       local.add_remote('testremote', remote)
       local.remove_remote('testremote')
-      
+
       assert(!local.remotes.map{|b| b.name}.include?('testremote'))
 
       local.add_remote('testremote', remote)
       local.remote('testremote').remove
-      
+
       assert(!local.remotes.map{|b| b.name}.include?('testremote'))
     end
   end
-  
+
   def test_set_remote_url
     in_temp_dir do |path|
       local = Git.clone(@wbare, 'local')
@@ -65,33 +65,33 @@ def test_remote_fun
     in_temp_dir do |path|
       loc = Git.clone(@wbare, 'local')
       rem = Git.clone(@wbare, 'remote')
-        
+
       r = loc.add_remote('testrem', rem)
 
       Dir.chdir('remote') do
         new_file('test-file1', 'blahblahblah1')
         rem.add
         rem.commit('master commit')
-        
+
         rem.branch('testbranch').in_branch('tb commit') do
           new_file('test-file3', 'blahblahblah3')
           rem.add
-          true          
+          true
         end
       end
       assert(!loc.status['test-file1'])
       assert(!loc.status['test-file3'])
-    
+
       r.fetch
-      r.merge   
+      r.merge
       assert(loc.status['test-file1'])
-      
+
       loc.merge(loc.remote('testrem').branch('testbranch'))
-      assert(loc.status['test-file3'])    
-      
+      assert(loc.status['test-file3'])
+
       #puts loc.remotes.map { |r| r.to_s }.inspect
-      
-      #r.remove  
+
+      #r.remove
       #puts loc.remotes.inspect
     end
   end
@@ -123,18 +123,37 @@ def test_fetch
     end
   end
 
+  def test_fetch_command_injection
+    test_file = 'VULNERABILITY_EXISTS'
+    vulnerability_exists = false
+    in_temp_dir do |_path|
+      git = Git.init('test_project')
+      origin = "--upload-pack=touch #{test_file};"
+      begin
+        git.fetch(origin, { ref: 'some/ref/head' })
+      rescue Git::GitExecuteError
+        # This is expected
+      else
+        raise 'Expected Git::GitExecuteError to be raised'
+      end
+
+      vulnerability_exists = File.exist?(test_file)
+    end
+    assert(!vulnerability_exists)
+  end
+
   def test_fetch_ref_adds_ref_option
     in_temp_dir do |path|
       loc = Git.clone(@wbare, 'local')
       rem = Git.clone(@wbare, 'remote', :config => 'receive.denyCurrentBranch=ignore')
       loc.add_remote('testrem', rem)
-      
+
       loc.chdir do
         new_file('test-file1', 'gonnaCommitYou')
         loc.add
         loc.commit('master commit 1')
         first_commit_sha = loc.log.first.sha
-        
+
         new_file('test-file2', 'gonnaCommitYouToo')
         loc.add
         loc.commit('master commit 2')
@@ -146,46 +165,46 @@ def test_fetch_ref_adds_ref_option
 
         # Make sure fetch message only has the second commit when we fetch the second commit
         assert(loc.fetch('origin', {:ref => second_commit_sha}).include?(second_commit_sha))
-        assert(!loc.fetch('origin', {:ref => second_commit_sha}).include?(first_commit_sha))        
-      end      
+        assert(!loc.fetch('origin', {:ref => second_commit_sha}).include?(first_commit_sha))
+      end
     end
   end
-  
+
   def test_push
     in_temp_dir do |path|
       loc = Git.clone(@wbare, 'local')
       rem = Git.clone(@wbare, 'remote', :config => 'receive.denyCurrentBranch=ignore')
-        
+
       loc.add_remote('testrem', rem)
 
       loc.chdir do
         new_file('test-file1', 'blahblahblah1')
         loc.add
         loc.commit('master commit')
         loc.add_tag('test-tag')
-        
+
         loc.branch('testbranch').in_branch('tb commit') do
           new_file('test-file3', 'blahblahblah3')
           loc.add
-          true          
+          true
         end
       end
       assert(!rem.status['test-file1'])
       assert(!rem.status['test-file3'])
-      
+
       loc.push('testrem')
 
-      assert(rem.status['test-file1'])    
-      assert(!rem.status['test-file3'])    
+      assert(rem.status['test-file1'])
+      assert(!rem.status['test-file3'])
       assert_raise Git::GitTagNameDoesNotExist do
         rem.tag('test-tag')
       end
-      
+
       loc.push('testrem', 'testbranch', true)
 
       rem.checkout('testbranch')
-      assert(rem.status['test-file1'])    
-      assert(rem.status['test-file3'])    
+      assert(rem.status['test-file1'])
+      assert(rem.status['test-file3'])
       assert(rem.tag('test-tag'))
     end
   end
