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

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

Open
3 tasks done
uhthomas opened this issue Jun 17, 2023 · 4 comments
Open
3 tasks done
Assignees
Labels
bug Something isn't working enhancement New feature or request 📱mobile 🗄️server 🖥️web

Comments

@uhthomas
Copy link
Member

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

  • Server
  • Web
  • Mobile

Your docker-compose.yml content

N/A

Your .env content

N/A

Reproduction steps

N/A

Additional information

No response

@uhthomas uhthomas added bug Something isn't working needs triage Bug that needs triage from maintainer enhancement New feature or request and removed enhancement New feature or request labels Jun 17, 2023
@uhthomas uhthomas added enhancement New feature or request 📱mobile 🖥️web 🗄️server and removed needs triage Bug that needs triage from maintainer labels Jun 21, 2023
@uhthomas
Copy link
Member Author

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

uhthomas commented Jul 3, 2023

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.

@uhthomas uhthomas self-assigned this Jul 3, 2023
@aechmtwash
Copy link

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

uhthomas commented Jul 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request 📱mobile 🗄️server 🖥️web
Projects
None yet
Development

No branches or pull requests

2 participants