Dear Florian, Thank you for taking the time to reply to this email. From: django-developers@googlegroups.com <django-developers@googlegroups.com> on behalf of Florian Apolloner <f.apollo...@gmail.com> Sent: 04 January 2022 16:45 To: Django developers (Contributions to Django itself) <django-developers@googlegroups.com> Subject: Re: Django security releases issued: 4.0.1, 3.2.11, and 2.2.26
Hi, On Tuesday, January 4, 2022 at 2:09:18 PM UTC+1 Sjoerd Job Postmus wrote: Unfortunately (at least for us), this breaks the case where dictsort was used with a static argument that looked up a callable. A quick code search showed that the pattern dictsort.*get (https://github.com/search?q=%22dictsort.*get%22&type=Code) is less used than I would expect it to be used, but used nonetheless. I expected as much; literally every security fix nowadays will break some code. Sorry about that. You can change it to a property or write your own filter I guess. Yes, of course. I guess we'll introduce some kind of objsort ourselves for our usecases by copy/pasting the original dictsort implementation. On the other hand, searching publicly viewable code for cases where dictsort gets a dynamic value yields very little results: https://grep.app/search?q=dictsort%3A%5B%5E%22%270-9%5D®exp=true&case=true, only one which I can recognize as a Django template: https://github.com/crodas/Haanga/blob/develop/tests/assert_templates/regroup.tpl#L3 . Yes, that said there is no easy way for a template filter to determine whether it is a variable or a literal (I'd say it is simply impossible). Our first approach would have been to limit only those values provided via variables… I do not think that is possible in a nice way though and probably to much for a security patch. Likely possible, but at least very difficult without writing the tokenizer yourself instead of using register.tag, and definitely not something one would like to do in a security patch, I agree. Since the previous behaviour of allowing callables was in place already in 2005, (I could find https://github.com/django/django/commit/ed114e15106192b22ebb78ef5bf5bce72b419d13#diff-e05e2e8efbf1fa5eea1ffee16cc8b740cba7b1bff746b2e55cebf968a0983f2cR192, where the filter is introduced), I would argue that even though it may not have been explicitly documented that this syntax allows callables, I don't think it's far fetched to consider it to always have supported. I would argue that the Django team always said that the documentation is the public API. Everything else works by luck. I even went as far as supporting lookups on objects (ie a list of objects as opposed to a list of dicts) because I assumed that people would use that often (and the docs clearly say this filter is for dicts). Yes, indeed, Django's position has always been such, and I think that's in general a very good position. However, before creating a ticket, I was wondering what the sentiment of django-developers is here. My own sentiments are summarized by what's also mentioned in the announcement blogpost. >From a security PoV I think this is not going to fly. The main issue here is >that dictsort should perform a rather limited subset of normal template >variable resolution -- if we were going to support callables again we'd have >to also support `alter_data` etc (which is forbidden in templates) and then we >are basically back to what the previous code did. We opted for the most >limited subset possible while allowing a relatively wide range of code to keep >working, I don't think adding more features to that filter is feasible. I think this is where I differ of opinion. The question to me is whether dictsort should perform a rather limited subset of normal template variable resolution or not, and would myself have chosen to update the documentation instead of changing the implementation. However, I expect you to have discussed this internally in the security team as well, and you've landed on a different conclusion than I would have. Given my experience with communicating with the security team before, I know I can trust that your decision is the right decision for Django. > As a reminder, all untrusted user input should be validated before use. As an example, even with the change, {% for user in users|dictsort:"password" %} would still be supported, which is still be counted as potential information disclosure. There is a massive difference though. Being able to sort by every index inside the password means that you need to control far less password hashes than in your example to get a useful result. To be honest I do not think it is very realistic to use that attack on the password; but it might be usable for shorter api tokens or so that you can also view in plaintext. Yes, agreed. Being able to sort by any arbitrary index of an API key would indeed be problematic from a security PoV. I hope that helps. Cheers, Florian Yes, thank you for your reply. Very much appreciated. Kind regards, Sjoerd Job Postmus -- 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+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/AM9PR09MB5089A8C8F4F1157F4AAAA0FDAB4B9%40AM9PR09MB5089.eurprd09.prod.outlook.com.
