Skip to content

Restrict LFS file download to project-bound objects

Emma Parkrequested to merge
510292/disclosure-LFS into master

What does this MR do and why?

Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/510292+

The problem

Previously, LfsStorageController#download allowed any user with download_code permission on any project to download any globally stored LFS object, simply by knowing its SHA—regardless of whether the object belonged to the project.

This was possible because GitLab's LFS backend stores objects globally, indexed only by their oid, without verifying if they are linked to the project making the request.

The fix

This change ensures that only LFS objects explicitly linked to the project can be downloaded.

Qeury plan

rails c> puts lfs_object.project_allowed_access?(project).sql returned three queries.

ForkNetworkMember Load (0.6ms) SELECT "fork_network_members".* FROM "fork_network_members" WHERE "fork_network_members"."project_id" = 92 LIMIT 1 /application:console,db_config_database:gitlabhq_development,db_config_name:main,console_hostname:epark--20240603-GPQ0R,console_username:emmapark,line:/app/models/lfs_object.rb:36:in `project_allowed_access?'/

ForkNetwork Load (1.8ms) SELECT "fork_networks".* FROM "fork_networks" INNER JOIN "fork_network_members" ON "fork_networks"."id" = "fork_network_members"."fork_network_id" WHERE "fork_network_members"."project_id" = 92 LIMIT 1 /application:console,db_config_database:gitlabhq_development,db_config_name:main,console_hostname:epark--20240603-GPQ0R,console_username:emmapark,line:/app/models/lfs_object.rb:38:in `project_allowed_access?'/

LfsObjectsProject Exists? (2.4ms) SELECT 1 AS one FROM "lfs_objects_projects" WHERE "lfs_objects_projects"."lfs_object_id" = 28 AND (EXISTS(SELECT 1 FROM "fork_network_members" WHERE "fork_network_members"."fork_network_id" = 26 AND (fork_network_members.project_id = lfs_objects_projects.project_id))) LIMIT 1 /application:console,db_config_database:gitlabhq_development,db_config_name:main,console_hostname:epark--20240603-GPQ0R,console_username:emmapark,line:/app/models/lfs_object.rb:39:in `project_allowed_access?'/

The first query: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/40047/commands/123247

Limit  (cost=0.43..3.45 rows=1 width=16) (actual time=0.015..0.015 rows=1 loops=1)
   Buffers: shared hit=4
   I/O Timings: read=0.000 write=0.000
   ->  Index Scan using index_fork_network_members_on_project_id on public.fork_network_members  (cost=0.43..3.45 rows=1 width=16) (actual time=0.014..0.014 rows=1 loops=1)
         Index Cond: (fork_network_members.project_id = 70236228)
         Buffers: shared hit=4
         I/O Timings: read=0.000 write=0.000
Settings: effective_cache_size = '472585MB', jit = 'off', work_mem = '100MB', random_page_cost = '1.5', seq_page_cost = '4'
Time: 0.601 ms
  - planning: 0.559 ms
  - execution: 0.042 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 4 (~32.00 KiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

The second query: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/40047/commands/123248

 Limit  (cost=0.85..6.89 rows=1 width=70) (actual time=0.026..0.027 rows=1 loops=1)
   Buffers: shared hit=8
   I/O Timings: read=0.000 write=0.000
   ->  Nested Loop  (cost=0.85..6.89 rows=1 width=70) (actual time=0.025..0.026 rows=1 loops=1)
         Buffers: shared hit=8
         I/O Timings: read=0.000 write=0.000
         ->  Index Scan using index_fork_network_members_on_project_id on public.fork_network_members  (cost=0.43..3.45 rows=1 width=16) (actual time=0.015..0.016 rows=1 loops=1)
               Index Cond: (fork_network_members.project_id = 70236228)
               Buffers: shared hit=4
               I/O Timings: read=0.000 write=0.000
         ->  Index Scan using fork_networks_pkey on public.fork_networks  (cost=0.42..3.44 rows=1 width=54) (actual time=0.006..0.006 rows=1 loops=1)
               Index Cond: (fork_networks.id = fork_network_members.fork_network_id)
               Buffers: shared hit=4
               I/O Timings: read=0.000 write=0.000
Settings: work_mem = '100MB', random_page_cost = '1.5', seq_page_cost = '4', effective_cache_size = '472585MB', jit = 'off'
Time: 1.294 ms
  - planning: 1.205 ms
  - execution: 0.089 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 8 (~64.00 KiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

The second query: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/40047/commands/123249

 Limit  (cost=1.00..28.60 rows=1 width=4) (actual time=12.118..12.121 rows=1 loops=1)
   Buffers: shared hit=4 read=5
   I/O Timings: read=12.035 write=0.000
   ->  Nested Loop  (cost=1.00..28.60 rows=1 width=4) (actual time=12.116..12.118 rows=1 loops=1)
         Buffers: shared hit=4 read=5
         I/O Timings: read=12.035 write=0.000
         ->  Index Scan using index_fork_network_members_on_fork_network_id on public.fork_network_members  (cost=0.43..8.18 rows=9 width=4) (actual time=0.736..0.736 rows=1 loops=1)
               Index Cond: (fork_network_members.fork_network_id = 2777470)
               Buffers: shared hit=3 read=1
               I/O Timings: read=0.704 write=0.000
         ->  Index Only Scan using index_lfs_objects_projects_on_project_id_and_lfs_object_id on public.lfs_objects_projects  (cost=0.57..2.26 rows=1 width=4) (actual time=11.375..11.375 rows=1 loops=1)
               Index Cond: ((lfs_objects_projects.project_id = fork_network_members.project_id) AND (lfs_objects_projects.lfs_object_id = 145365631))
               Heap Fetches: 0
               Buffers: shared hit=1 read=4
               I/O Timings: read=11.331 write=0.000
Settings: work_mem = '100MB', random_page_cost = '1.5', seq_page_cost = '4', effective_cache_size = '472585MB', jit = 'off'
Time: 13.532 ms
  - planning: 1.359 ms
  - execution: 12.173 ms
    - I/O read: 12.035 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 4 (~32.00 KiB) from the buffer pool
  - reads: 5 (~40.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Emma Park

Merge request reports