TreyWW / MyFinances

MyFinances is a web application that can help you as an individual, or team, manage your finances!
https://docs.myfinances.cloud
GNU Affero General Public License v3.0
105 stars 157 forks source link

[BUG] [VULNERABILITY] Any file extention is allowed, should only be PNG/JPG #98

Closed TreyWW closed 11 months ago

TreyWW commented 1 year ago

I've realised that this field in models.py doesn't restrict the extention type at all. I thought django did this automatically, but it doesnt.

profile_picture = models.ImageField(
        upload_to="profile_pictures", blank=True, null=True
    )

I'm researching into this and will merge when i figure it out. Feel free to comment if you know anything about this, and how we can restrict it to PNG/JPG etc, I did try a custom validator but couldn't get that to work. Still trying though

PhilipZara commented 1 year ago

Hello @TreyWW, I have tried to recreate the bug, but I couldn't. I tested it and I can only upload image files as my profile pic. I am using Mac OS 13.6 and the latest Chrome version. I also tested it on the latest version of Safari and the only difference is that I can also upload HEIC images. Is there a way I can recreate the bug?

TreyWW commented 1 year ago

Hey @PhilipZara

So i could bypass this by bypassing the frontend verification. For example, in chrome when you upload anything you can manually set the filter to all files, so our frontend sets it to images only but the user can still bypass this really easily.

So it's always neccessary to check on the backend before accepting any form inputs, I thought django did this by default, which it does but only for django forms, we use just pure HTML and manual request.get() methods.

Let me know if you would like a video or more explanation on how to reproduce :)

image

PhilipZara commented 1 year ago

I managed to recreate the bug, you are right. I'll try to help you resolve this issue. What do you think is the right approach here?

TreyWW commented 1 year ago

Thanks. We'd need some sort of is image validation on the view. There is built in Django validator functions you can us a try except block around that. I'm happy to help tomorrow if you need it

TreyWW commented 1 year ago

Hey @PhilipZara,

I combat this problem in my other project with this code example

from django.core.validators import validate_image_file_extension
from django.core.exceptions import ValidationError
picture = request.FILES.get("profile_picture")
profile.profile_picture = picture
try:
    if not picture: # this isn't required
        raise ValidationError("Profile picture is required") # this isnt required
    profile.full_clean()
    validate_image_file_extension(picture) # this raises ValidationError if the extension isnt an Image format
except ValidationError:
    messages.error(request, "Invalid image")
    return render(request, self.template_path + "post/response.html", {})
profile.save() # only save if picture is valid
PhilipZara commented 1 year ago

Hello @TreyWW thank you for your help. I'll take a look at it right now and come back to you if I manage to do something.

TreyWW commented 1 year ago

Cool, thanks!

PhilipZara commented 1 year ago

Hello @TreyWW I opened a PR with a potential fix. I tested it on MacOS 13 and Chrome's latest version. Please let me know what you think.

DanieII commented 1 year ago

Hello! I see that this issue still exists so I wrote a custom validator to handle this case. def validate_profile_picture_extension(profile_picture): allowed_extensions = ["png", "jpg"] extension = Path(profile_picture.name).suffix[1:].lower() if extension not in allowed_extensions: raise ValidationError( f"The specified file could not be uploaded. Only files with the following extensions are allowed: {', '.join(allowed_extensions)}" ) I added it in the view and it works as expected. @TreyWW can you tell me if I should create a PR and if so in which file do I put the validator function.

PhilipZara commented 1 year ago

Hello! Thanks for noticing. I just corrected my code.