Unverified Commit 5b075413 authored by Nick Thomas's avatar Nick Thomas Committed by Yorick Peterse

Verify that LFS upload requests are genuine

LFS uploads are handled in concert by workhorse and rails. In normal
use, workhorse:

* Authorizes the request with rails (upload_authorize)
* Handles the upload of the file to a tempfile - disk or object storage
* Validates the file size and contents
* Hands off to rails to complete the upload (upload_finalize)

In `upload_finalize`, the LFS object is linked to the project. As LFS
objects are deduplicated across all projects, it may already exist. If
not, the temporary file is copied to the correct place, and will be
used by all future LFS objects with the same OID.

Workhorse uses the Content-Type of the request to decide to follow this
routine, as the URLs are ambiguous. If the Content-Type is anything but
"application/octet-stream", the request is proxied directly to rails,
on the assumption that this is a normal file edit request. If it's an
actual LFS request with a different content-type, however, it is routed
to the Rails `upload_finalize` action, which treats it as an LFS upload
just as it would a workhorse-modified request.

The outcome is that users can upload LFS objects that don't match the
declared size or OID. They can also create links to LFS objects they
don't really own, allowing them to read the contents of files if they
know just the size or OID.

We can close this hole by requiring requests to `upload_finalize` to be
sourced from Workhorse. The mechanism to do this already exists.
parent 66744469
\ No newline at end of file
......@@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
include WorkhorseRequest
include SendFileUpload
skip_before_action :verify_workhorse_api!, only: [:download, :upload_finalize]
skip_before_action :verify_workhorse_api!, only: :download
def download
lfs_object = LfsObject.find_by_oid(oid)
title: Verify that LFS upload requests are genuine
type: security
......@@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do
context 'and request to finalize the upload is not sent by gitlab-workhorse' do
it 'fails with a JWT decode error' do
expect { put_finalize(lfs_tmp_file, verified: false) }.to raise_error(JWT::DecodeError)
context 'and workhorse requests upload finalize for a new lfs object' do
before do
......@@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do
context 'when pushing the same lfs object to the second project' do
before do
finalize_headers = headers
.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file)
put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}",
params: {},
headers: headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file).compact
headers: finalize_headers
it 'responds with status 200' do
......@@ -1370,7 +1380,7 @@ describe 'Git LFS API and storage' do
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", params: {}, headers: authorize_headers
def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, args: {})
def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, verified: true, args: {})
upload_path = LfsObjectUploader.workhorse_local_upload_path
file_path = upload_path + '/' + lfs_tmp if lfs_tmp
......@@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do
'file.name' => File.basename(file_path)
put_finalize_with_args(args.merge(extra_args).compact, verified: verified)
def put_finalize_with_args(args)
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: headers
def put_finalize_with_args(args, verified:)
finalize_headers = headers
finalize_headers.merge!(workhorse_internal_api_request_header) if verified
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: finalize_headers
def lfs_tmp_file
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