immich-app / immich

High performance self-hosted photo and video management solution.
https://immich.app
GNU Affero General Public License v3.0
51.09k stars 2.7k forks source link

[BUG] Immich should detect content type server side and reject uploads early #2824

Closed uhthomas closed 2 months ago

uhthomas commented 1 year ago

The bug

Immich currently relies on the client to relay the content type of the file being uploaded, which causes some problems. The clients end up needing to implement a lot of logic for proper mime type detection and often lag behind when new types are supported, like is the case at the moment where Immich now supports lots of raw image files, but the CLI does not.

https://github.com/immich-app/CLI/issues/27

https://github.com/immich-app/CLI/issues/47

https://github.com/immich-app/CLI/issues/95

In addition, the server is too trusting at the moment. It's likely possible to upload a jpg file as video/mp4 and be accepted. This shouldn't be allowed, really.

Immich should instead try to derive the mime type from the file extension server side, or attempt to sniff the content type from the first few bytes of the stream, and then evaluate whether the upload is valid. This will take a lot of logic and bloat out of clients and make the experience across platforms (cli, web, android, iOS) more consistent.

The OS that Immich Server is running on

N/A

Version of Immich Server

v1.61.0

Version of Immich Mobile App

N/A

Platform with the issue

Your docker-compose.yml content

N/A

Your .env content

N/A

Reproduction steps

N/A

Additional information

No response

uhthomas commented 1 year ago

I've been thinking about this a bit more and we may be able to leverage exiftool here. It's more than happy to read arbitrary byte streams and figure out the correct mime type and extension. We should probably just use that. We need quite a lot of data, if not all data, before we can use exiftool to do that and so it may make sense to have an additional step to reject files without a valid extension.

To summarise:

  1. Before files are uploaded, validate filenames.
  2. When files are uploaded, detect the correct content type as described by exiftool.

We could also do some aggressive rejection for file extensions which don't match the data returned by exiftool?

uhthomas commented 1 year ago

Thinking further, it might make sense to just drop the content type from the database entirely. It might be easier to detect the content type from the file extension or the actual content when requested instead. This has some benefits:#

  1. Changes to the list of mime types wouldn't require a database migration.
  2. Mime types would be consistent. For instance, avi has four mime types. At current, it's possible that all four content type could be served depending what value was persisted during the upload.
  3. Fixes to the list of mime types (i.e image/xyz) wouldn't require a migration to fix.
  4. File uploads can use different heuristics to determine validity without needing to then figure out a mime type to store in the database.
  5. Less data to manage.
aechmtwash commented 1 year ago

This is also a security issue. Anything from a client needs to be sanitized and verified before use. Server side type determination is definitely preferred from a security perspective. Depending on what options exist to check content type on a partial upload, we may want to consider maintaining a client side method but add an API call to check if that type is supported by the server and then validate the type once the upload completes on the server side.

uhthomas commented 1 year ago

This is also a security issue. Anything from a client needs to be sanitized and verified before use. Server side type determination is definitely preferred from a security perspective. Depending on what options exist to check content type on a partial upload, we may want to consider maintaining a client side method but add an API call to check if that type is supported by the server and then validate the type once the upload completes on the server side.

I agree, but also am not sure it's that problematic. I can't see any obvious vulnerabilities at present, even if it makes me uncomfortable.

jrasm91 commented 2 months ago

I don't think there are any plans to migrate to an implementation like this. The current solution works well enough and has pretty low complexity.