Hello, Thank you for raising this, Markus.
I am +1 to everything Baptiste said. In particular, if our conclusion of this bug would be that CBV are entirely unsuitable for security sensitive features, I don’t think removing CBV for auth is enough. Because by that logic, our users will be making the same mistakes, and the logical step would be to remove CBV from Django entirely. Which is not something I would advocate. If we are concerned about it being easier to cause vulnerabilities in CBV, I think updates to the documentation might be the best place to address this. Erik > On 22 Nov 2016, at 09:02, Baptiste Mispelon <[email protected]> wrote: > > Hi Markus, > > Thanks for your clear description and for bringing this up for discussion. > > I don't agree with your conclusions though. > > > 1) Keeping around two implementations of auth views seems counter-productive > to me in terms of security because it effectively doubles the potential for > bugs or security issues (not to mention the added maintenance work). We > should have only one set of auth views. > > 2) Our users want more extension hooks for auth views. If we don't provide > them, some users will end up copy/pasting Django's views and tweak what they > need (I have done that myself more than once). This copy/pasted code then > won't receive updates (potentially security-related) when Django changes, > which also presents a danger to our users (unless they track changes in the > code, which is unlikely). > > 3) Your description of the security issues seems quite alarming but how does > it compare with others in the past? To me, a 4-month window actually seems > quite small, not to mention that the issue never even made it to a release. > > 4) I agree with Tim that there's an issue in our test suite. Function-based > views give you the assumption that all HTTP methods will use the same entry > point into your view. You lose this assumption with class-based views but I > don't view this as a defect: to me, this is one of the big advantages of > class-based views. This probably means that we should audit Django's code and > add tests to make sure we cover all supported HTTP methods. > > > Thanks, > Baptiste > > On 11/21/2016 08:11 PM, Markus Holtermann wrote: >> Hi all, >> >> As it turned out [1], due to their complexity, using class-based generic >> views for security-sensitive functionality can result in unintended >> behavior. Essentially, the reset token was only checked on GET requests, not >> on POST. This was due to the check being in `get_context_data()` (which is >> only called on GET but not POST except for invalid forms) and not higher up >> the stack. Validation could happen in the SetPasswordForm but doesn't really >> belong there either. The form is being used by the admin to allow superusers >> to change other users' password. Also, password resets could probably happen >> via other ways that want to leverage a form that doesn't require a token. In >> the end, from my perspective the check for the correct token does belong in >> the view. >> >> While the reported issue was fixed [2] it raises the question if the added >> functionality of class-based generic views is worth the danger of shooting >> ourselves in the foot. I see the benefits of GCBVs. But given that the >> reported issue stayed unnoticed for 4 months makes me think that those views >> are not the best for these use cases and easily underpin the security >> functionality. Hence I suggest to revert the patch (including all additional >> features they gained) unless they are integrated in the function-based views >> and add guidelines on how to use class-based generic views for security >> sensitive feature. >> >> This is the thread to get the discussion about this started. >> >> One thing I want to suggest regardless if the class-based generic views are >> removed again or not, is to hold off the deprecation of the function-based >> views. This allows users who feel the same to not use class-based generic >> views without having deprecation warnings. At least until the next LTS >> release. >> >> Furthermore, myself and Florian Apolloner, who discovered the issue, are >> leaning +0 to +1 on the revert of the class-based views. >> >> Cheers, >> >> Markus Holtermann >> >> [1] >> https://www.djangoproject.com/weblog/2016/nov/21/passwordresetconfirmview-security-advisory/ >> >> <https://www.djangoproject.com/weblog/2016/nov/21/passwordresetconfirmview-security-advisory/> >> [2] https://github.com/django/django/pull/7591 >> <https://github.com/django/django/pull/7591>-- >> You received this message because you are subscribed to the Google Groups >> "Django developers (Contributions to Django itself)" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected] >> <mailto:[email protected]>. >> To post to this group, send email to [email protected] >> <mailto:[email protected]>. >> Visit this group at https://groups.google.com/group/django-developers >> <https://groups.google.com/group/django-developers>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/django-developers/19b675f5-c25a-43e8-ac73-2a31b9e351d6%40googlegroups.com >> >> <https://groups.google.com/d/msgid/django-developers/19b675f5-c25a-43e8-ac73-2a31b9e351d6%40googlegroups.com?utm_medium=email&utm_source=footer>. >> For more options, visit https://groups.google.com/d/optout >> <https://groups.google.com/d/optout>. > > > -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected] > <mailto:[email protected]>. > To post to this group, send email to [email protected] > <mailto:[email protected]>. > Visit this group at https://groups.google.com/group/django-developers > <https://groups.google.com/group/django-developers>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/32ca3ed6-abbe-1268-bd12-9cce3c314d9f%40gmail.com > > <https://groups.google.com/d/msgid/django-developers/32ca3ed6-abbe-1268-bd12-9cce3c314d9f%40gmail.com?utm_medium=email&utm_source=footer>. > For more options, visit https://groups.google.com/d/optout > <https://groups.google.com/d/optout>. -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/8054CDD4-FD93-4FFA-8649-959ABC7DF586%40solidlinks.nl. For more options, visit https://groups.google.com/d/optout.
