Commit 87c5abfc authored by Nick Thomas's avatar Nick Thomas

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 ae216618
8.1.0 8.1.1
\ No newline at end of file
...@@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController ...@@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
include WorkhorseRequest include WorkhorseRequest
include SendFileUpload include SendFileUpload
skip_before_action :verify_workhorse_api!, only: [:download, :upload_finalize] skip_before_action :verify_workhorse_api!, only: :download
def download def download
lfs_object = LfsObject.find_by_oid(oid) lfs_object = LfsObject.find_by_oid(oid)
......
---
title: Verify that LFS upload requests are genuine
merge_request:
author:
type: security
...@@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do ...@@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do
end end
end end
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)
end
end
context 'and workhorse requests upload finalize for a new lfs object' do context 'and workhorse requests upload finalize for a new lfs object' do
before do before do
lfs_object.destroy lfs_object.destroy
...@@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do ...@@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do
context 'when pushing the same lfs object to the second project' do context 'when pushing the same lfs object to the second project' do
before do before do
finalize_headers = headers
.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file)
.merge(workhorse_internal_api_request_header)
put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}",
params: {}, params: {},
headers: headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file).compact headers: finalize_headers
end end
it 'responds with status 200' do it 'responds with status 200' do
...@@ -1370,7 +1380,7 @@ describe 'Git LFS API and storage' 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 put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", params: {}, headers: authorize_headers
end end
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 upload_path = LfsObjectUploader.workhorse_local_upload_path
file_path = upload_path + '/' + lfs_tmp if lfs_tmp file_path = upload_path + '/' + lfs_tmp if lfs_tmp
...@@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do ...@@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do
'file.name' => File.basename(file_path) 'file.name' => File.basename(file_path)
} }
put_finalize_with_args(args.merge(extra_args).compact) put_finalize_with_args(args.merge(extra_args).compact, verified: verified)
end end
def put_finalize_with_args(args) def put_finalize_with_args(args, verified:)
put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: headers 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
end end
def lfs_tmp_file 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