New commits to private projects visible in forks created while project was public

⚠ Please read the process on how to fix security issues before starting to work on the issue. Vulnerabilities must be fixed in a security mirror.

HackerOne report #1944500 by pwnie on 2023-04-13, assigned to @greg:

Report | How To Reproduce

Report

Summary

When a public project is forked and then privated, new commits to the repo can be viewed still.

Steps to reproduce

Create a public project (project 1) on gitlab.com with account 1
On another account (account 2), fork project 1
Set project 1 to private
Create a new file in project 1 named "secret.txt" with secret inside
Go to https://gitlab.com/account2/project2/-/compare/main...main?straight=true (replace account2 and project2 with corresponding names)
You should see secret.txt

Impact

Upstream private repository content is not private.

P.S. This bug could potentially be chained with another bug that allows a user to create an arbitrary forked_from_project relationship in the Project model. Perhaps import functionality, although I was not successful in my efforts since everything seems to run through a relationship factory. Though if someone could, this would allow them to exfiltrate data from any private repository without having to fork it first, like the CTF flag you guys have in your policy for example. It would technically be possible to dump that flag if I could get forked_from_project to be an arbitrary project.

What is the current bug behavior?

Commits from private upstream repositories are visible to forks.

What is the expected correct behavior?

Proper access controls should be implemented to prevent upstream commits from being fetched

Cause

I haven't dug that deep into why commits from upstream repositories are visible even after being privated, or why commits from upstream repositories are resolved in the first place. Though what led me to find this bug was the following code in app/models/compare.rb:

  # target == start_ref == from  
  def target_project  
    strong_memoize(:target_project) do  
      next source_project.default_merge_request_target unless compare_params.key?(:from_project_id)  
      next source_project if compare_params[:from_project_id].to_i == source_project.id

      target_project = target_projects(source_project).find_by_id(compare_params[:from_project_id])

      # Just ignore the field if it points at a non-existent or hidden project  
      next source_project unless target_project && can?(current_user, :read_code, target_project)

      target_project  
    end  
  end

  # source == head_ref == to  
  def source_project  
    strong_memoize(:source_project) do  
      # Eager load project's avatar url to prevent batch loading  
      # for all forked projects  
      project&.tap(&:avatar_url)  
    end  
  end

  def compare  
    return [@]compare if defined?([@]compare)

    [@]compare = CompareService.new(source_project, head_ref).execute(target_project, start_ref, straight: straight)  
  end

Notice next source_project.default_merge_request_target unless compare_params.key?(:from_project_id) returns a default value if from_project_id is not present. The compare controller passes from_project_id along by default and fails to load the commits at target_project && can?(current_user, :read_code, target_project) since we wouldn't have :read_code permissions on a private project.

Simply removing :from_project_id loads the default_merge_request_target with no access controls, which is ironically the same value as the default from_project_id. That of course gets passed to CompareService.new and the rest is self explanatory.

Impact

Upstream private repository content is not private.

How To Reproduce

Please add reproducibility information to this section: