Tighten validation for generic packages names

Add file path traversal validation.

Related to
https://gitlab.com/gitlab-org/gitlab/-/issues/235493.

lib/api/generic_packages.rb

@@ -30,7 +30,7 @@ module API
           route_setting :authentication, job_token_allowed: true
 
           params do
-            requires :package_name, type: String, desc: 'Package name'
+            requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.generic_package_name_regex, file_path: true
             requires :package_version, type: String, desc: 'Package version', regexp: Gitlab::Regex.generic_package_version_regex
             requires :file_name, type: String, desc: 'Package file name', regexp: Gitlab::Regex.generic_package_file_name_regex, file_path: true
           end
@@ -44,7 +44,7 @@ module API
           end
 
           params do
-            requires :package_name, type: String, desc: 'Package name'
+            requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.generic_package_name_regex, file_path: true
             requires :package_version, type: String, desc: 'Package version', regexp: Gitlab::Regex.generic_package_version_regex
             requires :file_name, type: String, desc: 'Package file name', regexp: Gitlab::Regex.generic_package_file_name_regex, file_path: true
             requires :file, type: ::API::Validations::Types::WorkhorseFile, desc: 'The package file to be published (generated by Multipart middleware)'
@@ -70,8 +70,12 @@ module API
             forbidden!
           end
 
+          desc 'Download package file' do
+            detail 'This feature was introduced in GitLab 13.5'
+          end
+
           params do
-            requires :package_name, type: String, desc: 'Package name'
+            requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.generic_package_name_regex, file_path: true
             requires :package_version, type: String, desc: 'Package version', regexp: Gitlab::Regex.generic_package_version_regex
             requires :file_name, type: String, desc: 'Package file name', regexp: Gitlab::Regex.generic_package_file_name_regex, file_path: true
           end

lib/gitlab/regex.rb

@@ -109,9 +109,13 @@ module Gitlab
         /\A\d+\.\d+\.\d+\z/
       end
 
-      def generic_package_file_name_regex
+      def generic_package_name_regex
         maven_file_name_regex
       end
+
+      def generic_package_file_name_regex
+        generic_package_name_regex
+      end
     end
 
     extend self

spec/lib/gitlab/regex_spec.rb

@@ -465,6 +465,20 @@ RSpec.describe Gitlab::Regex do
     it { is_expected.not_to match('') }
   end
 
+  describe '.generic_package_name_regex' do
+    subject { described_class.generic_package_name_regex }
+
+    it { is_expected.to match('123') }
+    it { is_expected.to match('foo') }
+    it { is_expected.to match('foo.bar.baz-2.0-20190901.47283-1') }
+    it { is_expected.not_to match('../../foo') }
+    it { is_expected.not_to match('..\..\foo') }
+    it { is_expected.not_to match('%2f%2e%2e%2f%2essh%2fauthorized_keys') }
+    it { is_expected.not_to match('$foo/bar') }
+    it { is_expected.not_to match('my file name') }
+    it { is_expected.not_to match('!!()()') }
+  end
+
   describe '.generic_package_file_name_regex' do
     subject { described_class.generic_package_file_name_regex }
 

spec/requests/api/generic_packages_spec.rb

@@ -33,6 +33,18 @@ RSpec.describe API::GenericPackages do
     { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => value || ci_build.token }
   end
 
+  shared_examples 'secure endpoint' do
+    before do
+      project.add_developer(user)
+    end
+
+    it 'rejects malicious request' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+  end
+
   describe 'PUT /api/v4/projects/:id/packages/generic/:package_name/:package_version/:file_name/authorize' do
     context 'with valid project' do
       using RSpec::Parameterized::TableSyntax
@@ -73,38 +85,46 @@ RSpec.describe API::GenericPackages do
         end
 
         it "responds with #{params[:expected_status]}" do
-          headers = workhorse_header.merge(auth_header)
-          url = "/projects/#{project.id}/packages/generic/mypackage/0.0.1/myfile.tar.gz/authorize"
-
-          put api(url), headers: headers
+          authorize_upload_file(workhorse_header.merge(auth_header))
 
           expect(response).to have_gitlab_http_status(expected_status)
         end
       end
     end
 
-    it 'rejects a malicious request' do
-      project.add_developer(user)
-      headers = workhorse_header.merge(personal_access_token_header)
-      url = "/projects/#{project.id}/packages/generic/mypackage/0.0.1/%2e%2e%2f.ssh%2fauthorized_keys/authorize"
+    context 'application security' do
+      using RSpec::Parameterized::TableSyntax
 
-      put api(url), headers: headers
+      where(:param_name, :param_value) do
+        :package_name | 'my-package/../'
+        :package_name | 'my-package%2f%2e%2e%2f'
+        :file_name    | '../.ssh%2fauthorized_keys'
+        :file_name    | '%2e%2e%2f.ssh%2fauthorized_keys'
+      end
 
-      expect(response).to have_gitlab_http_status(:bad_request)
+      with_them do
+        subject { authorize_upload_file(workhorse_header.merge(personal_access_token_header), param_name => param_value) }
+
+        it_behaves_like 'secure endpoint'
+      end
     end
 
     context 'generic_packages feature flag is disabled' do
       it 'responds with 404 Not Found' do
         stub_feature_flags(generic_packages: false)
         project.add_developer(user)
-        headers = workhorse_header.merge(personal_access_token_header)
-        url = "/projects/#{project.id}/packages/generic/mypackage/0.0.1/myfile.tar.gz/authorize"
 
-        put api(url), headers: headers
+        authorize_upload_file(workhorse_header.merge(personal_access_token_header))
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz')
+      url = "/projects/#{project.id}/packages/generic/#{package_name}/0.0.1/#{file_name}/authorize"
+
+      put api(url), headers: request_headers
+    end
   end
 
   describe 'PUT /api/v4/projects/:id/packages/generic/:package_name/:package_version/:file_name' do
@@ -246,17 +266,27 @@ RSpec.describe API::GenericPackages do
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
+    end
 
-      it 'rejects a malicious request' do
-        headers = workhorse_header.merge(personal_access_token_header)
-        upload_file(params, headers, file_name: '%2e%2e%2f.ssh%2fauthorized_keys')
+    context 'application security' do
+      using RSpec::Parameterized::TableSyntax
 
-        expect(response).to have_gitlab_http_status(:bad_request)
+      where(:param_name, :param_value) do
+        :package_name | 'my-package/../'
+        :package_name | 'my-package%2f%2e%2e%2f'
+        :file_name    | '../.ssh%2fauthorized_keys'
+        :file_name    | '%2e%2e%2f.ssh%2fauthorized_keys'
+      end
+
+      with_them do
+        subject { upload_file(params, workhorse_header.merge(personal_access_token_header), param_name => param_value) }
+
+        it_behaves_like 'secure endpoint'
       end
     end
 
-    def upload_file(params, request_headers, send_rewritten_field: true, file_name: 'myfile.tar.gz')
-      url = "/projects/#{project.id}/packages/generic/mypackage/0.0.1/#{file_name}"
+    def upload_file(params, request_headers, send_rewritten_field: true, package_name: 'mypackage', file_name: 'myfile.tar.gz')
+      url = "/projects/#{project.id}/packages/generic/#{package_name}/0.0.1/#{file_name}"
 
       workhorse_finalize(
         api(url),
@@ -329,7 +359,15 @@ RSpec.describe API::GenericPackages do
       it_behaves_like 'a gitlab tracking event', described_class.name, 'pull_package'
     end
 
-    it 'rejects a malicious request' do
+    it 'rejects a malicious file name request' do
+      project.add_developer(user)
+
+      download_file(personal_access_token_header, file_name: '../.ssh%2fauthorized_keys')
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
+    it 'rejects a malicious file name request' do
       project.add_developer(user)
 
       download_file(personal_access_token_header, file_name: '%2e%2e%2f.ssh%2fauthorized_keys')
@@ -337,6 +375,39 @@ RSpec.describe API::GenericPackages do
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
+    it 'rejects a malicious package name request' do
+      project.add_developer(user)
+
+      download_file(personal_access_token_header, package_name: 'my-package/../')
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
+    it 'rejects a malicious package name request' do
+      project.add_developer(user)
+
+      download_file(personal_access_token_header, package_name: 'my-package%2f%2e%2e%2f')
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
+    context 'application security' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:param_name, :param_value) do
+        :package_name | 'my-package/../'
+        :package_name | 'my-package%2f%2e%2e%2f'
+        :file_name    | '../.ssh%2fauthorized_keys'
+        :file_name    | '%2e%2e%2f.ssh%2fauthorized_keys'
+      end
+
+      with_them do
+        subject { download_file(personal_access_token_header, param_name => param_value) }
+
+        it_behaves_like 'secure endpoint'
+      end
+    end
+
     it 'responds with 404 Not Found for non existing package' do
       project.add_developer(user)