Prevent files with insufficient permissions from being cached

- Previously, we were altering file permissions when we unzipped the
resources for an app to give ourselves read/write access. The altered
files were then stored in the resource cache
- When attempting to push the app containing a file with insufficient
permissions for a second time, a resource match would be found. We
preserve the mode provided in the fingerprint given to us from the
client, rather than using the altered mode the file was cached with.
(endpoint: POST /v3/resource_matches)
- When the client reached the upload bits portion of push, it would pass
along the fingerprints matched in the step above. The request would then
error out, as it would run into a check we have to ensure that the mode
on all fingerprints is >= 0600. (endpoint: POST /v3/packages/:guid/upload)

To solve this issue we have done the following:
- Moved the portion of code that altered permissions on unzipping the
application. This is still necessary in allowing CAPI to clean up the
unzipped files, and is now done as part of `remove_dirs_from_zip`. If
changing the permissions fails, an error is logged but it does not cause
the call to fail.
- Enforced a file permission check before uploading any files to the
resource cache. This prevents files with permissions < 0600 from being
uploaded in the first place, ultimately preventing a resource match from
ever being returned for insufficiently permissioned files.

Users who have previously uploaded an insufficiently permissioned file
may need to clear their resource cache as these changes do not account
for any files already in the cache.

Github issue: cloudfoundry#1705
[finishes #173541786]

Authored-by: Sarah Weinstein <sweinstein@pivotal.io>

Author: sweinstein22

lib/cloud_controller/app_packager.rb

@@ -25,8 +25,6 @@ def unzip(destination_dir)
       logger.error("Unzipping had errors\n STDOUT: \"#{output}\"\n STDERR: \"#{error}\"")
       invalid_zip!
     end
-
-    fix_unzipped_permissions(destination_dir)
   end
 
   def append_dir_contents(additional_contents_dir)
@@ -43,8 +41,8 @@ def append_dir_contents(additional_contents_dir)
     end
   end
 
-  def fix_subdir_permissions
-    remove_dirs_from_zip(@path, get_dirs_from_zip(@path))
+  def fix_subdir_permissions(root_path, app_contents_path)
+    remove_dirs_from_zip(@path, get_dirs_from_zip(@path), root_path, app_contents_path)
   rescue Zip::Error
     invalid_zip!
   end
@@ -69,7 +67,10 @@ def logger
     @logger ||= Steno.logger('app_packager')
   end
 
-  def remove_dirs_from_zip(zip_path, dirs_from_zip)
+  def remove_dirs_from_zip(zip_path, dirs_from_zip, root_path, app_contents_path)
+    unless empty_directory?(root_path)
+      fix_permissions_for_file_deletion(app_contents_path)
+    end
     dirs_from_zip.each_slice(DIRECTORY_DELETE_BATCH_SIZE) do |directory_slice|
       remove_dir(zip_path, directory_slice)
     end
@@ -87,11 +88,10 @@ def remove_dir(zip_path, directories)
     end
   end
 
-  def fix_unzipped_permissions(destination_dir)
+  def fix_permissions_for_file_deletion(destination_dir)
     stdout, error, status = Open3.capture3(%(chmod -R u+rwX #{Shellwords.escape(destination_dir)}))
     unless status.success?
-      logger.error("Fixing zip file permissions error\n STDOUT: \"#{stdout}\"\n STDERR: \"#{error}\"")
-      invalid_zip!
+      logger.error("Cleanup of some files may have failed, error fixing zip file permissions\n STDOUT: \"#{stdout}\"\n STDERR: \"#{error}\"")
     end
   end
 

lib/cloud_controller/blobstore/base_client.rb

@@ -8,6 +8,7 @@ def cp_r_to_blobstore(source_dir)
         Find.find(source_dir).each do |path|
           next unless File.file?(path)
           next unless within_limits?(File.size(path))
+          next unless File.stat(path).mode.to_s(8)[3..5].to_i(8) >= 0600
 
           sha1 = Digester.new.digest_path(path)
           next if exists?(sha1)

lib/cloud_controller/packager/shared_bits_packer.rb

@@ -22,8 +22,7 @@ def copy_uploaded_package(uploaded_package_zip, app_packager)
         FileUtils.cp(uploaded_package_zip, app_packager.path)
       end
 
-      def populate_resource_cache(app_packager, root_path)
-        app_contents_path = File.join(root_path, 'application_contents')
+      def populate_resource_cache(app_packager, app_contents_path)
         FileUtils.mkdir(app_contents_path)
         app_packager.unzip(app_contents_path)
 
@@ -51,17 +50,18 @@ def append_matched_resources(app_packager, cached_files_fingerprints, root_path)
 
       def match_resources_and_validate_package(root_path, uploaded_package_zip, cached_files_fingerprints)
         app_packager = AppPackager.new(File.join(root_path, 'copied_app_package.zip'))
+        app_contents_path = File.join(root_path, 'application_contents')
 
         if package_zip_exists?(uploaded_package_zip)
           copy_uploaded_package(uploaded_package_zip, app_packager)
           validate_size!(app_packager)
-          populate_resource_cache(app_packager, root_path)
+          populate_resource_cache(app_packager, app_contents_path)
         end
 
         append_matched_resources(app_packager, cached_files_fingerprints, root_path)
 
         validate_size!(app_packager)
-        app_packager.fix_subdir_permissions
+        app_packager.fix_subdir_permissions(root_path, app_contents_path)
         app_packager.path
       end
 

spec/unit/lib/app_packager_spec.rb

@@ -32,39 +32,6 @@
       expect(Dir["#{@tmpdir}/subdir/*"].size).to eq 1
     end
 
-    context 'when the zip contains files with weird permissions' do
-      context 'when there are unreadable dirs' do
-        let(:input_zip) { File.join(Paths::FIXTURES, 'app_packager_zips', 'unreadable_dir.zip') }
-
-        it 'makes all files/dirs readable to cc' do
-          app_packager.unzip(@tmpdir)
-
-          expect(File.readable?("#{@tmpdir}/unreadable")).to be true
-        end
-      end
-
-      context 'when there are unwritable dirs' do
-        let(:input_zip) { File.join(Paths::FIXTURES, 'app_packager_zips', 'undeletable_dir.zip') }
-
-        it 'makes all files/dirs writable to cc' do
-          app_packager.unzip(@tmpdir)
-
-          expect(File.writable?("#{@tmpdir}/undeletable")).to be true
-        end
-      end
-
-      context 'when there are untraversable dirs' do
-        let(:input_zip) { File.join(Paths::FIXTURES, 'app_packager_zips', 'untraversable_dir.zip') }
-
-        it 'makes all dirs traversable to cc' do
-          app_packager.unzip(@tmpdir)
-
-          expect(File.executable?("#{@tmpdir}/untraversable")).to be true
-          expect(File.executable?("#{@tmpdir}/untraversable/file.txt")).to be false
-        end
-      end
-    end
-
     context 'when the zip contains broken symlinks' do
       let(:input_zip) { File.join(Paths::FIXTURES, 'app_packager_zips', 'broken-file-symlink.zip') }
 
@@ -126,21 +93,6 @@
         }.to raise_error(CloudController::Errors::ApiError, /Invalid zip archive/)
       end
     end
-
-    context 'when there is an error adjusting permissions' do
-      before do
-        allow(Open3).to receive(:capture3).with(/unzip/).and_return(['output', 'error', double(success?: true)])
-        allow(Open3).to receive(:capture3).with(/chmod/).and_return(['output', 'error', double(success?: false)])
-      end
-
-      it 'raises an exception' do
-        expect(logger).to receive(:error).with /Fixing zip file permissions error.*\n.*output.*\n.*error.*/
-
-        expect {
-          app_packager.unzip(@tmpdir)
-        }.to raise_error(CloudController::Errors::ApiError, /Invalid zip archive/)
-      end
-    end
   end
 
   describe '#append_dir_contents' do
@@ -215,7 +167,7 @@
       before { FileUtils.cp(File.join(Paths::FIXTURES, 'app_packager_zips', 'bad_directory_permissions.zip'), input_zip) }
 
       it 'deletes all directories from the archive' do
-        app_packager.fix_subdir_permissions
+        app_packager.fix_subdir_permissions(@tmpdir, "#{@tmpdir}/application_contents")
 
         has_dirs = Zip::File.open(input_zip) do |in_zip|
           in_zip.any?(&:directory?)
@@ -231,7 +183,7 @@
       before { FileUtils.cp(File.join(Paths::FIXTURES, 'app_packager_zips', 'special_character_names.zip'), input_zip) }
 
       it 'successfully removes and re-adds them' do
-        app_packager.fix_subdir_permissions
+        app_packager.fix_subdir_permissions(@tmpdir, "#{@tmpdir}/application_contents")
         expect(`zipinfo #{input_zip}`).to match %r(special_character_names/&&hello::\?\?/)
       end
     end
@@ -241,12 +193,12 @@
 
       before { FileUtils.cp(File.join(Paths::FIXTURES, 'app_packager_zips', 'many_dirs.zip'), input_zip) }
 
-      it 'batches the directory deletes so it does not exceed the max command length' do
+      it 'fixes the directory permissions and batches the directory deletes so it does not exceed the max command length' do
         allow(Open3).to receive(:capture3).and_call_original
         batch_size = 10
         stub_const('AppPackager::DIRECTORY_DELETE_BATCH_SIZE', batch_size)
 
-        app_packager.fix_subdir_permissions
+        app_packager.fix_subdir_permissions(@tmpdir, "#{@tmpdir}/application_contents")
 
         output = `zipinfo #{input_zip}`
 
@@ -257,7 +209,7 @@
 
         number_of_batches = (21.0 / batch_size).ceil
         expect(number_of_batches).to eq(3)
-        expect(Open3).to have_received(:capture3).exactly(number_of_batches).times
+        expect(Open3).to have_received(:capture3).exactly(number_of_batches + 1).times
       end
     end
 
@@ -268,7 +220,7 @@
       it 'raises an exception' do
         allow(Open3).to receive(:capture3).and_return(['output', 'error', double(success?: false)])
         expect {
-          app_packager.fix_subdir_permissions
+          app_packager.fix_subdir_permissions(@tmpdir, "#{@tmpdir}/application_contents")
         }.to raise_error(CloudController::Errors::ApiError, /The app package is invalid: Error removing zip directories./)
       end
     end
@@ -279,7 +231,7 @@
       it 'raises an exception' do
         allow(Open3).to receive(:capture3).and_return(['output', 'error', double(success?: false)])
         expect {
-          app_packager.fix_subdir_permissions
+          app_packager.fix_subdir_permissions(@tmpdir, "#{@tmpdir}/application_contents")
         }.to raise_error(CloudController::Errors::ApiError, /The app upload is invalid: Invalid zip archive./)
       end
     end

spec/unit/lib/cloud_controller/blobstore/fog/fog_client_spec.rb

@@ -185,6 +185,33 @@ def upload_tmpfile(client, key='abcdef')
               client.cp_r_to_blobstore(path)
             end
           end
+
+          context 'limit the file mode to those with sufficient permissions' do
+            subject(:client) do
+              FogClient.new(connection_config: connection_config,
+                            directory_key: directory_key)
+            end
+
+            it 'copies files with mode >= 0600' do
+              path = File.join(local_dir, 'file_with_sufficient_permissions')
+              FileUtils.touch(path)
+              File.chmod(0600, path)
+
+              expect(client).to receive(:exists?)
+              expect(client).to receive(:cp_to_blobstore)
+              client.cp_r_to_blobstore(path)
+            end
+
+            it 'does not copy files below the minimum file mode' do
+              path = File.join(local_dir, 'file_with_insufficient_permissions')
+              FileUtils.touch(path)
+              File.chmod(0444, path)
+
+              expect(client).not_to receive(:exists?)
+              expect(client).not_to receive(:cp_to_blobstore)
+              client.cp_r_to_blobstore(path)
+            end
+          end
         end
 
         describe '#download_from_blobstore' do

spec/unit/lib/cloud_controller/blobstore/webdav/dav_client_spec.rb

@@ -389,6 +389,48 @@ module Blobstore
           end
         end
 
+        describe 'file permissions' do
+          context 'sufficient file permissions' do
+            let!(:sufficiently_permissioned_file) do
+              Tempfile.open('', source_dir) do |tmpfile|
+                File.chmod(0600, tmpfile)
+                tmpfile
+              end
+            end
+            let(:sufficient_perm_file_sha) { Digester.new.digest_path(sufficiently_permissioned_file.path) }
+
+            it 'copies files above the maximum size limit' do
+              allow(response).to receive_messages(status: 201, content: '')
+              allow(httpclient).to receive(:put).and_return(response)
+
+              client.cp_r_to_blobstore(source_dir)
+
+              expect(httpclient).to have_received(:put).with(
+                "http://localhost/admin/droplets/#{sufficient_perm_file_sha[0..1]}/#{sufficient_perm_file_sha[2..3]}/#{sufficient_perm_file_sha}", a_kind_of(File), {})
+            end
+          end
+
+          context 'insufficient file permissions' do
+            let!(:insufficiently_permissioned_file) do
+              Tempfile.open('', source_dir) do |tmpfile|
+                File.chmod(0444, tmpfile)
+                tmpfile
+              end
+            end
+            let(:insufficient_perm_file_sha) { Digester.new.digest_path(insufficiently_permissioned_file.path) }
+
+            it 'does not copy files above the maximum size limit' do
+              allow(response).to receive_messages(status: 201, content: '')
+              allow(httpclient).to receive(:put).and_return(response)
+
+              client.cp_r_to_blobstore(source_dir)
+
+              expect(httpclient).not_to have_received(:put).with(
+                "http://localhost/admin/droplets/#{insufficient_perm_file_sha[0..1]}/#{insufficient_perm_file_sha[2..3]}/#{insufficient_perm_file_sha}", a_kind_of(File), {})
+            end
+          end
+        end
+
         context 'when an OpenSSL::SSL::SSLError is raised' do
           it 'reraises a BlobstoreError' do
             allow(httpclient).to receive(:put).and_raise(OpenSSL::SSL::SSLError.new)

spec/unit/lib/cloud_controller/packager/local_bits_packer_spec.rb

@@ -294,34 +294,6 @@ module CloudController::Packager
               `unzip #{local_tmp_dir}/package.zip path/to/content.txt -d #{local_tmp_dir}`
               expect(sprintf('%<mode>o', mode: File.stat(File.join(local_tmp_dir, 'path/to/content.txt')).mode)).to eq('100653')
             end
-
-            describe 'bad file permissions' do
-              context 'when the write permissions are too-restrictive' do
-                let(:mode) { '344' }
-
-                it 'errors' do
-                  expect {
-                    packer.send_package_to_blobstore(blobstore_key, uploaded_files_path, cached_files_fingerprints)
-                  }.to raise_error do |error|
-                    expect(error.name).to eq 'AppResourcesFileModeInvalid'
-                    expect(error.response_code).to eq 400
-                  end
-                end
-              end
-
-              context 'when the permissions are nonsense' do
-                let(:mode) { 'banana' }
-
-                it 'errors' do
-                  expect {
-                    packer.send_package_to_blobstore(blobstore_key, uploaded_files_path, cached_files_fingerprints)
-                  }.to raise_error do |error|
-                    expect(error.name).to eq 'AppResourcesFileModeInvalid'
-                    expect(error.response_code).to eq 400
-                  end
-                end
-              end
-            end
           end
         end
       end