Security - Githubissues

lorddaedra commented 3 years ago

Please, check django.contrib.auth views and decorators.

IMHO we should include

@method_decorator(sensitive_post_parameters())
    @method_decorator(csrf_protect)
    @method_decorator(never_cache)

decorators and do some additional checks for next url using url_has_allowed_host_and_scheme.

pyepye commented 3 years ago

Hi @lorddaedra

@method_decorator(sensitive_post_parameters()) - I don't know if anywhere sensitive_post_parameters is required / can help. The only 2 POST requests we have are signup and login, both don't include any sensitive info. It would be helpful to hide the token and perhaps ip_address in the LoginVerify view but this is a GET request.
@method_decorator(csrf_protect) - The csrf_protect decorator should only be used when overriding the lack of CSRF middleware. If someone does not want to include CSRF checks that is a choice they can make in their settings and not one I feel an individual 3rd party app should override.
@method_decorator(never_cache) - Yep this seems sensible for the verify view. While when heavy caching is turned on Django treats any request with different query parameters as a different page to cache. A magic link can be configured to be used multiple times etc.

Let me know if you think any of that sounds insane.

Thanks Matt

pyepye commented 3 years ago

FYI I've just released 1.0.2 to pypi with the never_cache decorator 👍

lorddaedra commented 3 years ago

@pyepye

This is how I did it: https://github.com/lorddaedra/django-magiclink/blob/master/magiclinks/views.py This is my customised fork (not yet stable). (some features were removed, verification mechanism was changed too, it uses signing now)

sensitive_post_parameters() in original version of Login view (username&password) it was used to secure password (IMHO) but also useful for any Login & Register views to hide some form details (to respect privacy of users, GDPR etc.), possible use case here: you add some user related fields to registration form and Sentry integration and you do not want to send form data to Sentry.

csrf_protect I think it's good idea to provide best default settings optimised for privacy&security and if user do not need them, he/she can just create his/her own views (based on app views or from scratch)

So I disagree with you a little bit about decorators but anyway thanks again for this project, I used a lot of things from your project to create my fork and will follow any updates.

Also note about next url. I see XSS there. https://owasp.org/www-community/attacks/xss/ url_has_allowed_host_and_scheme may help to fix it. (it available in Django 3.2)

Also you use hidden form field value to select form. But do not check values. It's possible to create request with incorrect field value and generate 500 error on server (not tested it myself, I just removed this code in my fork).

pyepye commented 3 years ago

Hey @lorddaedra

Thanks for the reply.

My issue with sensitive_post_parameters _ALL_ is what if someone wants to log that a user requested a login? By adding that decorator we remove that ability completely. On the other hand if you add sensitive information to the user model you don't want to log, you will need to hide that data in other places as well so there is probably a more appropriate way to do it. To take your example using sentry you would probably scrub the data from the sentry event

With csrf_protect - fair point with the sane defaults. For csrf_protect to work you only need the SessionMiddleware setup and this app won't work without it so there is no real point in not including it - https://github.com/pyepye/django-magiclink/commit/72e6640305c8295d3e3b9de3918a4aca556e3c45

Also thanks for bringing up url_has_allowed_host_and_scheme again. It slipped my mind after replying last time but it's a good check to put in. I'll fall back to using is_safe_url which is the pre 3.2 version (which I think is the same except for the less clear name). - https://github.com/pyepye/django-magiclink/commit/b399aacfcffa9d24a15a76f8bbac71c8ed83e75a

And good catch on the hidden form field on the signup page. Not sure it really matters considering to get to that point someone must have manually edited the HTML but it's better to return a redirect instead of a 500 👍 - https://github.com/pyepye/django-magiclink/commit/6cba359a5ee048847bf77f12b62c999bb90a5526

pyepye commented 3 years ago

1.0.4 has been pushed with those changes

dosdos commented 8 months ago

Can we close this issue now? It's pretty scary to see the "Security" thread still open and found at the end that most of the points have been solved 😛

pyepye / django-magiclink

Security #4