Harden YARD and git clone against untrusted-repo RCE

app/models/github_fetcher/repo.rb

@@ -1,7 +1,23 @@
 # frozen_string_literal: true
 
+require "open3"
+
 module GithubFetcher
   class Repo < Resource
+    UnsafeCloneUrlError = Class.new(StandardError)
+
+    # Only clone from https://github.com. This rejects git's dangerous
+    # transports (file://, ssh://, ext::…) and any host other than GitHub, in
+    # case the API response is unexpected or has been tampered with.
+    def self.safe_clone_url?(url)
+      return false unless url.is_a?(String)
+
+      uri = URI.parse(url)
+      uri.scheme == "https" && uri.host == "github.com"
+    rescue URI::InvalidURIError
+      false
+    end
+
     def initialize(options)
       @api_path = File.join(
         "repos",
@@ -18,13 +34,39 @@ def default_branch
     # TODO - does this really belong here? Seems like it (and Repo#populate_docs!)
     #   should move into the PopulateDocs job
     def clone
-      out = `cd #{dir} && git clone #{clone_url} 2>&1`
-      raise "Error executing git clone #{clone_url.inspect}: #{out.inspect}" unless $?.success?
+      url = clone_url
+      unless self.class.safe_clone_url?(url)
+        raise UnsafeCloneUrlError, "Refusing to clone unexpected URL: #{url.inspect}"
+      end
+
+      # Array form runs git directly with no shell, so the URL can never be
+      # interpreted by /bin/sh. GIT_TERMINAL_PROMPT=0 keeps a bad URL from
+      # blocking on a credential prompt, and chdir lands the checkout in our
+      # scratch dir (git creates a subdirectory named after the repo).
+      out, status = Open3.capture2e(
+        {"GIT_TERMINAL_PROMPT" => "0"},
+        *git_clone_argv(url),
+        chdir: dir
+      )
+      raise "Error executing git clone #{url.inspect}: #{out.inspect}" unless status.success?
       dir
     end
 
     private
 
+    # Build the git argv as an array so it runs without a shell, and place the
+    # URL after `--` so a URL beginning with `-` can't be parsed as a git
+    # option. Shallow/single-branch/no-tags keep the checkout small.
+    def git_clone_argv(url)
+      [
+        "git", "clone",
+        "--depth", "1",
+        "--single-branch",
+        "--no-tags",
+        "--", url
+      ]
+    end
+
     # TODO - moves w/ clone
     def dir
       @dir ||= Dir.mktmpdir
@@ -35,9 +77,8 @@ def clone_url
       as_json["clone_url"]
     end
 
-    # TODO - this appears to be uncalled... remove? or do we need it and should use it?
-    def cleanup
-      FileUtils.remove_entry(dir)
+    public def cleanup
+      FileUtils.remove_entry(@dir) if @dir
     end
   end
 end

app/models/repo.rb

@@ -74,6 +74,8 @@ def populate_docs!(commit_sha: commit_sha_fetcher.commit_sha, location: nil, has
     parser.process
     parser.store(self)
     :success
+  ensure
+    fetcher.cleanup
   end
 
   def background_populate_issues!

lib/docs_doctor/parsers/ruby/yard.rb

@@ -86,7 +86,18 @@ def hash_for_entity(obj, repo)
 
         def process(exclude = DEFAULT_EXCLUDE)
           require "yard"
+
+          # We run YARD against untrusted, user-submitted repositories, so it
+          # must never execute code that ships in the repo. Safe mode disables
+          # YARD's code-execution features (`-e/--load`, `--query`, custom
+          # template paths), and ignoring the repo's `.yardopts` file stops YARD
+          # from reading attacker-controlled options at all. Without this a repo
+          # could gain remote code execution via a malicious `.yardopts`
+          # (the same class of RCE abused against RubyDoc.info).
+          YARD::Config.options[:safe_mode] = true
+
           yard = YARD::CLI::Yardoc.new
+          yard.use_yardopts_file = false
 
           # yard.files       = files
           yard.excluded = exclude # http://rubydoc.org/gems/yard/YARD/Parser/SourceParser#parse-class_method

test/jobs/parse_docs_test.rb

@@ -34,4 +34,35 @@ class ParseDocsTest < ActiveJob::TestCase
       ).in_fork { raise "foo" }
     end
   end
+
+  test "process enables YARD safe mode" do
+    require "yard"
+    YARD::Config.options[:safe_mode] = false
+
+    Dir.mktmpdir do |repo_dir|
+      FileUtils.mkdir_p(File.join(repo_dir, "lib"))
+      File.write(File.join(repo_dir, "lib", "thing.rb"), "class Thing\n  def hello\n  end\nend\n")
+
+      DocsDoctor::Parsers::Ruby::Yard.new(repo_dir).process
+
+      assert YARD::Config.options[:safe_mode],
+        "Expected process to run YARD in safe mode"
+    end
+  end
+
+  test "process ignores the repo's .yardopts and still parses methods" do
+    Dir.mktmpdir do |repo_dir|
+      # If this .yardopts were honored, `--exclude lib` would drop lib/thing.rb
+      # from parsing and Thing#hello would be missing.
+      File.write(File.join(repo_dir, ".yardopts"), "--exclude lib\n")
+      FileUtils.mkdir_p(File.join(repo_dir, "lib"))
+      File.write(File.join(repo_dir, "lib", "thing.rb"), "class Thing\n  def hello\n  end\nend\n")
+
+      parser = DocsDoctor::Parsers::Ruby::Yard.new(repo_dir)
+      parser.process
+
+      assert(parser.yard_objects.any? { |o| o.respond_to?(:path) && o.path == "Thing#hello" },
+        "Expected the repo's .yardopts to be ignored so Thing#hello is still parsed")
+    end
+  end
 end

test/test_helper.rb

@@ -8,6 +8,8 @@
 require "rails/test_help"
 require "capybara/rails"
 require "webmock/minitest"
+require "tmpdir"
+require "fileutils"
 
 class ActiveSupport::TestCase
   # Setup all fixtures in test/fixtures/*.(yml|csv) for all tests in alphabetical order.

test/unit/github_fetcher/repo_test.rb

@@ -43,4 +43,55 @@ def fetcher(repo)
       assert_nil fetcher.default_branch
     end
   end
+
+  test "#safe_clone_url? only allows https github.com URLs" do
+    assert GithubFetcher::Repo.safe_clone_url?("https://github.com/schneems/get_process_mem.git")
+
+    refute GithubFetcher::Repo.safe_clone_url?("http://github.com/a/b.git")
+    refute GithubFetcher::Repo.safe_clone_url?("https://evil.com/a/b.git")
+    refute GithubFetcher::Repo.safe_clone_url?("https://github.com.evil.com/a/b.git")
+    refute GithubFetcher::Repo.safe_clone_url?("ssh://git@github.com/a/b.git")
+    refute GithubFetcher::Repo.safe_clone_url?("file:///etc/passwd")
+    refute GithubFetcher::Repo.safe_clone_url?("ext::sh -c id")
+    refute GithubFetcher::Repo.safe_clone_url?("--upload-pack=touch /tmp/pwn")
+    refute GithubFetcher::Repo.safe_clone_url?(nil)
+  end
+
+  test "#clone refuses an unsafe clone_url before running git" do
+    fetcher = fetcher(repos(:scene_hub_v2))
+    fetcher.stubs(:clone_url).returns("file:///tmp/not-a-real-repo-xyz")
+
+    error = assert_raises(StandardError) { fetcher.clone }
+    assert_match(/refus/i, error.message)
+  end
+
+  test "#git_clone_argv runs git without a shell and guards option injection" do
+    fetcher = fetcher(repos(:scene_hub_v2))
+    url = "https://github.com/schneems/get_process_mem.git"
+
+    argv = fetcher.send(:git_clone_argv, url)
+
+    assert_equal "git", argv[0]
+    assert_equal "clone", argv[1]
+    assert_includes argv, "--depth"
+    separator_index = argv.index("--")
+    assert separator_index, "expected a -- separator so the URL cannot be parsed as a git option"
+    assert_equal url, argv[separator_index + 1]
+  end
+
+  test "#cleanup removes the temporary working directory it created" do
+    fetcher = fetcher(repos(:scene_hub_v2))
+    dir = fetcher.send(:dir) # #clone checks the repo out into this scratch dir
+    assert Dir.exist?(dir)
+
+    fetcher.cleanup
+
+    refute Dir.exist?(dir), "expected the cloned scratch dir to be removed"
+  end
+
+  test "#cleanup is a safe no-op when nothing was cloned" do
+    fetcher = fetcher(repos(:scene_hub_v2))
+
+    assert_nothing_raised { fetcher.cleanup }
+  end
 end

test/unit/repo_test.rb

@@ -113,4 +113,36 @@ class RepoTest < ActiveSupport::TestCase
     assert_not repos.include?(subscribed_repo)
     assert repos.include?(unsubscribed_repo)
   end
+
+  test "#populate_docs! removes the working directory it cloned" do
+    repo = repos(:get_process_mem)
+    cloned_dir = nil
+
+    repo.fetcher.define_singleton_method(:clone) do
+      dir = send(:dir)
+      cloned_dir = dir
+      FileUtils.mkdir_p(File.join(dir, "lib"))
+      File.write(File.join(dir, "lib", "thing.rb"), "class Thing\n  def hello\n  end\nend\n")
+      dir
+    end
+
+    repo.populate_docs!(commit_sha: "abc123", has_subscribers: true)
+
+    refute_nil cloned_dir
+    refute Dir.exist?(cloned_dir),
+      "expected populate_docs! to clean up the temp dir it cloned"
+  end
+
+  test "#populate_docs! does not delete a caller-provided location" do
+    repo = repos(:get_process_mem)
+    location = Dir.mktmpdir
+    FileUtils.mkdir_p(File.join(location, "lib"))
+    File.write(File.join(location, "lib", "thing.rb"), "class Thing\n  def hello\n  end\nend\n")
+
+    repo.populate_docs!(commit_sha: "abc123", location: location, has_subscribers: true)
+
+    assert Dir.exist?(location), "a caller-provided location must not be deleted"
+  ensure
+    FileUtils.remove_entry(location) if location && Dir.exist?(location)
+  end
 end