Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
st3iny opened this issue Aug 14, 2024 · 6 comments
Open

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

st3iny opened this issue Aug 14, 2024 · 6 comments
Assignees

Comments

@st3iny
Copy link
Member

st3iny commented Aug 14, 2024

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 #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
Copy link
Member

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
Copy link
Member Author

st3iny commented Aug 14, 2024

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

@st3iny
Copy link
Member Author

st3iny commented Aug 14, 2024

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 #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
Copy link
Member

Also #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
Copy link
Member Author

st3iny commented Aug 19, 2024

And are they the correct/final values?

Yes, they are.

@nickvergessen
Copy link
Member

Started an attempt in #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants