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

Add check in frontend implementation for acceptedFileTypes violation #133

Closed
mshedsilegx opened this issue Oct 17, 2023 · 5 comments · Fixed by #139
Closed

Add check in frontend implementation for acceptedFileTypes violation #133

mshedsilegx opened this issue Oct 17, 2023 · 5 comments · Fixed by #139

Comments

@mshedsilegx
Copy link

1.9.0-9feecf3
acceptedFileTypes: 'image/,text/,application/pdf'

image
image
image

@Luzifer
Copy link
Owner

Luzifer commented Oct 17, 2023

As the documentation states:

# Pay attention this is NOT suited as a security measure as this is
# purely a frontend implementation and can easily be circumvented. The
# client tool does NOT check those file types and the server is not
# able to check them as it never sees the real content of the file.

This is not a security feature. Nothing can stop an user to attach other files than the ones you put into the attribute. The attribute only configures the accept attribute of the HTML file selector. If you change the "Custom Files" option of the file selector (might look different depending on your system) to "All Files" you can select any file you want to:

image

See this as a "suggestion" to the browser which files you want the user to choose. The browser configures the system file selector with those types but it can be ignored.

To securely limit the files you would need a server-side-check which is not possible as the server never sees the file in an unencrypted state. Even if I would try to parse that list in the frontend JS myself and match the file type (or the name) the browser gives me to the list this still does not guarantee anything.

Even if I would do this in the frontend JS and the OTS-CLI util you simply can write a fairly simple script to attach an executable to a secret.

@mshedsilegx
Copy link
Author

OK, noted.

@mshedsilegx
Copy link
Author

I was thinking of the following. Is is possible to have an implementation of a forbidden extension that would mimic the experience of a large size issue like below ?
image

Since you can do a validation test on the file size, why not on the file extension ?

@Luzifer
Copy link
Owner

Luzifer commented Oct 18, 2023

Well also the file size is not a security check and you really should set a max-body-size in the webserver. You cannot trust any check done in the frontend. Like I said above: I could parse that list in the JS code and then try to match the file to it (given I get correct information which itself is not guaranteed especially with more exotic file types) but what does it give? A false sense of security.

The CLI can upload a 1GiB file containing a giant virus collection… If we stop that in the browser and the CLI, the attacker will use a custom shell script. We lost.

I'm fully aware how security works and there is no security at all but doing checks in frontend is just like asking a thief not to steal your diamond laying in front of them while you drive to work…

The question is: Is it worth the effort implementing the parser and the matcher when it doesn't prevent anything?

@mshedsilegx
Copy link
Author

mshedsilegx commented Oct 18, 2023

I understand your arguments. I think it is OK to treat approved extensions as not a security check. It is more a business control, whose scope is only UI and CLI, not API for the reasons you stated. Just a simple list of allowed extensions as a property, and a file name check to validate the extension. If not in the list, return a blocking error message like the one above.

@Luzifer Luzifer added this to the 1.10.x milestone Oct 18, 2023
@Luzifer Luzifer changed the title acceptedFileTypes has no effect Add check in frontend implementation for acceptedFileTypes violation Oct 18, 2023
@Luzifer Luzifer linked a pull request Oct 18, 2023 that will close this issue
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