Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Markus Holtermann]

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/

[2] https://github.com/django/django/pull/7591

[Tom Christie]

Just to be absolutely clear, in case it's needed...

> is to hold off the deprecation of the function-based views.

Markus is specifically referring to the FBV implementations of the contrib.auth views here.

(Not to FBVs generally, which we've no intention of deprecating whatsoever)

  - Tom

[Tim Graham]

I haven't extended these views much, so I can't talk about the pain points of extending the function-based views compared to the ease of extending the classes. I'm certainly more confident about reasoning with function-based code. There was a draft patch [0] a few months ago that converted some admin views to class-based views. I found it much more difficult to follow to the point where I didn't feel it was an improvement.

When reviewing the patch for the auth class-based view additions, I made the mistake of assuming that the existing tests would catch this type of obviously incorrect issue. Perhaps with the function-based views, the code was "too obviously correct" that a test for token verification on POST wasn't considered. I'd like to think that future work on the class-based views would be more careful about testing, especially after we made this mistake.

If you feel the class-based views aren't reliable enough to deprecate the function-based views immediately, then I would say we shouldn't ship those views in Django in the first place. contrib.auth isn't a good place to ship "experimental API." The class-based views would certainly benefit from further audit now that we realize this type of mistake is possible.

[0] https://github.com/django/django/pull/6045

[Yo-Yo Ma]

> I found it much more difficult to follow to the point where I didn't feel it was an improvement.

I think that patch was just an example of bad abstraction. For instance, _log_and_response was strange and confusingly named, and seemed to be there mostly for vanity, to mask the imperative nature of the top level of control.

When "properly" abstracted, class-based views, IMHO, are much simpler to reason about. And, despite the one extra indentation level of a method (vs a module "function"), CBV tend to offer flatter logic and better separation of concerns.

Somewhat ironically, I find better examples of functional style code in CBV than I do in view functions.

[Yoong Kang Lim]

> I think that patch was just an example of bad abstraction. For instance, _log_and_response was strange and confusingly named, and seemed to be there mostly for vanity, to mask the imperative nature of the top level of control.

Proposed patch author here. Yes, I agree that wasn't a very successful attempt at the abstraction, and I think that's why I abandoned that effort. 

Perhaps the best abstraction isn't a Class Based View at all? I'm sure we could write classes other than Views, instead of forcing the abstraction to be a subclass of a View.

--
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 django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/6acd4821-d51f-4bf4-9550-f3843eb28805%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Claude Paroz]

Le lundi 21 novembre 2016 23:56:56 UTC+1, Tim Graham a écrit :

When reviewing the patch for the auth class-based view additions, I made the mistake of assuming that the existing tests would catch this type of obviously incorrect issue. Perhaps with the function-based views, the code was "too obviously correct" that a test for token verification on POST wasn't considered. I'd like to think that future work on the class-based views would be more careful about testing, especially after we made this mistake.

+1 to that, I think this issue essentially demonstrated a weakness in the test suite.
 
More generally and considering the patch entered the master branch soon in the schedule, it would be a shame not being able to make it stable and secure for 1.11. I could understand the delay of the function-based view deprecation, but I would be very disappointed to see that code removed for 1.11.

Also notice I don't buy the argument of a "danger of shooting ourselves in the foot". Currently, when people have to customize some part of the current views, they have no choice beside copy-pasting large part of the Django code in their code base. As versions evolve, some parts become stale and could even miss patches for security issues. So for me, by using class-based views and allowing users to only override the precise part of those views which needs to be overriden, the design is globally more secure than the current function-based views.

And of course, sorry for being the author of the faulty commit, and thanks to the eagle eyes!

Claude

[Baptiste Mispelon]

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

--
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 django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

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/19b675f5-c25a-43e8-ac73-2a31b9e351d6%40googlegroups.com.

[Florian Apolloner]

Hi,

On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:

… that the existing tests would catch this type of obviously incorrect issue.

I think that is the main issue here. I was also really surprised to discover that the tests were missing cases like this -- then again, writing good tests is hard and I know that I am personally one not to write good tests usually. Either way, I think the way forward is to improve the testsuite in this case.

What worries me really though is that if we make this kind of simple but horrible mistakes how are we going to prevent (especially) newcomers from making the same mistakes?

Cheers,
Florian

[Erik Romijn]

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

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/32ca3ed6-abbe-1268-bd12-9cce3c314d9f%40gmail.com.

[benjaoming]

As a big fan of GCBVs, this got me out of the chair :) I am -1 for removing GCBVs for authentication.

My reasons:

1) Security improvements also happen over time, finding a security hole in a component is not a reason to point the finger at the architecture. If you entirely remove something, you will have to rebuild it somewhere else, and new security holes will emerge. Thinking that users of builtin GCBVs will just opt for the FBVs is wrong, we won't, we'll write our own GCBVs or find some third-party that has less attentive eyes than the django.contrib.auth

2) In this case, having security checks in `get_context_data()` is far from the intentions of that method. If I override it, I would not expect security properties of the view to be removed, right? We shouldn't put security checks there, simple as that. If we assume the design pattern is (G)CBV, then this is a violation of the design principles and should have been rejected even before the security issue happened. In a GCBV, we should always ask "Is this logic to be expected by someone inheriting from this GCBV".

3) People have implemented and deployed GCBV from auth. The consequence of removing them because we don't want to deal with their security issues is alarming! I inherit from these classes, believing that we will be stronger together, because inheriting from a common codebase will have more eyes on the security aspects. This will be a bad example or bad signal to set for other usages of GCBVs.

4) SomeViewClass.as_view() generates an FBV, I'm sure we can bridge CBVs and FBVs if the concern is about having separate codebases as Baptiste expressed it.

5) I fully agree with Baptiste: If Django does not supply easily pluggable authentication patterns, someone else will (and not just 1 project, many will popup, they already have), and the effort will be fragmented, the security will be weakened.

6) I write CBVs in my own apps, I know there is a lot of afterthought and you often have to move logic between CBVs, mixins, and method decorators. I fully understand and sympathize with release notes that have entries about design changes and small refactors in GCBVs, that's awesome and you can learn from the considerations and decisions in Django's GCBVs.


Thanks :)
Ben

[Josh Smeaton]

+1 to everything Baptiste and Ben have said. A bug in a CBV isn't a good argument for throwing away CBVs entirely. We should probably review patches that touch security systems quite a bit more thoroughly in the future - meaning more eyes rather than a single set of eyes spending more time.

[João Sampaio]

+1 to Baptiste, Ben and Josh. Especially as Ben said, I think this does not justify removing the auth CBVs. The bug happened because of a misuse of the abstractions involved. ``.get_context_data()`` is not the correct place for a security check (as nobody overriding it would expect that, nor from the name or documentation of the method, nor from the pattern established by the other CBVs), and neglecting this is one of the causes of the bug. Another cause of the bug is the missing test case, which is also unrelated to the CBV.

I also agree that removing them would decreased security. When CBVs are well used, they are much stronger than FBVs, in which they allow reuse of reviewed code. They also make writing more modular code easier. (I've seen FBVs with hundreds of lines, while CBVs more often have code split in meaningful methods.)

The answer is simply to face this as something that happened to ring a loud bell: maybe security features should be more thoroughly reviewed, requiring maybe 2 or 3 reviewers to approve before being merged. Maybe we can even define a rule on "probation period", that new security-related features must sit in the master branch for 6+ months before being released (to avoid a security feature landing on master and being in a release a month later). This could have happened in this case, luckily it was caught before it was released. We can increase the chances of this happening more times.

--

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 django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.

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/9a031420-c8e9-407f-a32c-8a14e47a0d98%40googlegroups.com.

[Luke Plant]

Hi all,

First, I want to say that complex things fail in complex ways, so there it's probably a fallacy to look for a single root cause. I agree with various other points about mistakes that were made, but not others. Particularly:

On 22/11/16 12:41, Florian Apolloner wrote:

Hi,

On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:

… that the existing tests would catch this type of obviously incorrect issue.

I think that is the main issue here. I was also really surprised to discover that the tests were missing cases like this -- then again, writing good tests is hard and I know that I am personally one not to write good tests usually. Either way, I think the way forward is to improve the testsuite in this case.

I disagree with this, there was no issue with the existing test suite for the FBV version. Tests are never close to exhaustive and we should never think they are or can be. Tests are spot checks of correctness in an infinite universe of possible input values. When trying to work out what you should test in this vast space, there are a few sensible ways:

1. Look at the requirements, and pick spots that match each requirement. In this case:

• If a request has the required token, it should be allowed through
• If a request does not have the required token, it should not be allowed through

2. Make some guesses about well known edge cases and test them (e.g the empty string case etc.). However this could be a complete waste of time

3. Look at the implementation at work out where it might go wrong. This one is really important in reality.

And this is what the existing tests did. Out of the infinite number of HTTP requests to test under point 1, they happened to pick a GET request, which was perfectly sensible. In the case of FBVs, the control flow is so clear that all of the following are equally silly tests to add:

• Check that requests with query string parameter 'xyz' doesn't introduce a back door
  
• Check that on leap years tokens are checked
• Check that the security is correct for requests that are specifically GET, POST, HEAD, OPTIONS, DELETE....

The conclusion is this: in CBVs control flow is much, much less clear. In the FBV the mistake would have been immediately obvious, the CBV base classes obfuscated things to the point where it wasn't at all clear. To write adequate tests for the CBV version, under point 3 above, you have much, much more work to do.

I disagree that forcing people to write their own auth views is necessarily worse than providing a CBV that people can customise. Given this demonstration of how CBVs obfuscate control flow, it's quite possible that writing an FBV from scratch will be less error prone than using a CBV and subclassing, especially when we have encapsulated things like the token checking. Personally I would be massively happier auditing a custom FBV than a subclassed CBV, especially when you consider that the CBV subclass is inheriting from a stack of classes that could change in later versions of Django in some subtle way that breaks the code. Subclassing is not necessarily safer, it just feels safer ("I'm only changing this one little method"), and that feeling of greater safety is itself a huge danger.

Going forward, I think we need to:

1. Recognise that CBVs are much harder to reason about, and therefore require much more care.
2. Avoid using CBVs unless you really need them.
3. Recognise that tests for FBVs are inadequate against the CBV translation.
4. Not deprecate the FBVs. At the very least, they provide a sensible and easy to understand starting point for people that want to copy the logic to create their own, and do so in a way that they actually understand and can audit.

I'd be +0 on reverting to an FBV only, but it's part of a much bigger discussion

Regards,

Luke

[Joachim Jablon]

I'm +1 with Baptiste, Ben, Josh and João.

Also, Luke, you said :

1. Recognise that CBVs are much harder to reason about, and therefore require much more care. 

2. Avoid using CBVs unless you really need them.

 

Just wanted to note that this means never. FBV vs CBV is a choice, there's really few cases, if any, where one or the other is really needed.

From my (highly personal) point of view, some people will always be more at ease coding with CBV than with FBV and vice versa. FBVs are simpler at start, CBVs are more customizable, but in the end, you'll find a way to customize your FBV (a.k.a copy/pasting code) and the added complexity to your GCBV subclass probably won't be that high (chances are : you'll be adding some variable in the get_context_data(), while still calling super and that's it)

Maybe what we should be providing is more a way to put the complicated security-important logic out of a view altogether. A PasswordChanger class/module/... that would do the necessary work but just this work, independantly from how a view works. and then CBVs/FBVs would just call functions/methods on that. But then if there's already too much abstractions in CBV for the taste of some...

My 2 cents.

Joachim

[Markus Bertheau]

I'd like to argue in favor of exercising caution when using CBVs.

In my experience, CBVs shine when the requirements closely match the use case for a certain CBV and all customizations are purely declarative, often just setting the model. With the level of expressiveness that Python gives us a class is a good way to put into code what the view should do. Although one could argue that this kind of class does not align perfectly with how OOP intends classes to be used. Central principles of OOP, or programming with classes, are encapsulation and data hiding. When the requirements for a CBV become more complex from a django framework point of view, i.e. when it should do more than "save a model form with generic error handling" or "pass a paginated queryset to the template", the programmer has to overwrite methods in the class. In simple cases that's superficially simple, but the program and data flow that the programmer needs to control is hidden from his view in the class hierarchy. The more complex the class hierarchy and the more methods are overridden, the harder it is to figure out what the view does and whether it does it correctly.

Now an obvious strong point of CBVs is that it provides a lot of hooks (in the form of method overrides) for customization and extension without code duplication. OOP in Python also gives us mixins, which take this strong point even further. However, in my experience, it is more important to make the code easy to read and understand, than to make it simple to write. It's also more important than to avoid duplication in the extent that CBVs and OOP allow you to.

Finally, composition, deduplication and reuse are possible with functions as well, albeit in different ways.

My view is that in the case of the auth views the readability and understandability disadvantage of CBVs has led to this bug. In an FBVs such an omission would have been more obvious.

In conclusion, while CBVs allow for greater code reuse, they are also much harder to get right when the requirements are complex. What I'd do is:

- recommend CBVs for requirements that closely match the use cases of DetailView, ListView, UpdateView and so on.

- advise against the use of CBVs in more complex cases; the number of lines of imperative code in the CBV being a first indicator for complexity.

Markus