nextcloud / files_accesscontrol

🚫 App to manage access control for files
https://apps.nextcloud.com/apps/files_accesscontrol
50 stars 21 forks source link

File upload limited by size is blocked after assembling chunks #580

Open st3iny opened 2 months ago

st3iny commented 2 months ago

Steps to reproduce

  1. Configure a rule to block uploads greater than 4 MB.
  2. Set the chunk size to 5 MB: occ config:app:set files max_chunk_size --value 5242880
  3. Upload a file bigger than 5 MB in the web interface, e.g. 10 MB.
  4. Observe the network log.

Configured flow

grafik

Only the last request is blocked, not the chunks

grafik

Expected behaviour

The upload should be blocked on the first chunk instead of wasting storage and bandwidth resources.

Actual behaviour

The upload is blocked after a large file has been uploaded and assembled on the server.

This is a regression of https://github.com/nextcloud/files_accesscontrol/issues/211.

Server configuration

Operating system: Linux

Web server: Apache

Database: MariaDB

PHP version: 8.1

Nextcloud version: master

Where did you install Nextcloud from: Git

nickvergessen commented 2 months ago

Apache starting with version 2.4.59 stopped accepting the headers Content-Length and Transfer-Encoding set by PHP-FPM due to a security issue found (CVE-2024-24795).

https://github.com/nextcloud/server/blob/cc1686dba93278fb0e64d83f71d3c0a1e7e5a50a/apps/workflowengine/lib/Check/FileSize.php#L83-L85

I assume this is related? The web interface could also send the OC-Total-Length up front (if it knows).

Can you confirm the Apache version?

st3iny commented 2 months ago

@nickvergessen Both OC-Total-Length and Content-Length are present and available in PHP. I'm running 2.4.62.

st3iny commented 2 months ago

Something weird is happening when uploading chunks: The first check on isUpdateable() fails but then the second check on writeStream() succeeds, hence the parts get uploaded. So it is not the check itself there is something broken on a deeper level.

EDIT: The problem is that the path is transformed between both calls. On the second call it is uploads/web-file-upload-36fe675e5ccdf718/2.ocTransferId603416412.part which is filtered by Operation::isBlockablePath().

Also https://github.com/nextcloud/files_accesscontrol/pull/330 breaks scanning part files. So they will be ignored anyway.

    protected function isBlockablePath(IStorage $storage, string $path): bool {
        // [...]

        // Problem 1: part files are ignored
        if (preg_match('/\.ocTransferId\d+\.part$/i', $path)) {
            return false;
        }

        // [...]

        return isset($segment[2]) && in_array($segment[2], [
            // Problem 2: 'uploads' is missing here
            'files',
            'thumbnails',
            'files_versions',
        ]);
    }
nickvergessen commented 2 months ago

Also https://github.com/nextcloud/files_accesscontrol/pull/330 breaks scanning part files.

yeah quite many people have rules for mimetypes and when we check the mimetype on part files it goes wrong and would block uploading bigger files when e.g. the configured rules only allow uploading txt, doc, docx and folders.

Both OC-Total-Length and Content-Length are present

And are they the correct/final values?

st3iny commented 2 months ago

And are they the correct/final values?

Yes, they are.

nickvergessen commented 2 months ago

Started an attempt in https://github.com/nextcloud/files_accesscontrol/pull/585 But we don't seem to have access to the rules, we get back the check ids and would then need a way to get the check to see which class they are.