I personally think that a job of Django is to stop naive developers from doing stupid things. This is one of those things that a developer should understand, but it is substantially hidden and a real problem that bots exploit on the web. For this reason, option 3 would be my choice, but I think we should start looking into a default "what's expected" scheme for forms, because it does not really solve all of these problems.
I agree that option 3 will be white noise for developers who copy paste code conventions, but it would at least give us a clear chance to document why that decision was made. Could be a learning experience and for those who don't read the documentation, it will be an annoyance. Option 2 is concerning to me as well, because I think that a generic ModelForm would be most likely exploited in the current set up. One apparently easy (and DRY) way to provide granular permissions to users is to do it inside of templates (especially for a developer coming from the front end or PHP) by putting if statements around form fields [1]. I think this is where this is most likely going to happen and while option 3 doesn't solve this problem completely, it does at least give us a chance to explain why to developers. While this problem is different from Rails, it is still very possible that it could happen to Django in a similar way. 1.) https://docs.djangoproject.com/en/dev/topics/auth/#id9 On Wed, Jun 13, 2012 at 10:58 AM, Daniel Sokolowski < daniel.sokolow...@klinsight.com> wrote: > (I apologize if this is a duplicate - my other account appears to have an > issue sending to this group) > > I'm in the camp don't change it. ModelForms are doing what they are > designed to do: providing a very handy short cut. It is a design decision > to either use a ModelForm, or a a plain Form not tied to a model. If I want > front end user functionality I use a Form, if I want admin front end > functionality I use a ModelForm - I use a different approach based on the > expected level of trust here. The un-desired effect would also only come > into play if these new fields were not required so they would need to pass > validation - yes? Actually I would suggest to remove 'fields' and leave > the 'exclude' option, if I remove one two or three fields I might still > have functioning ModelForm where I don't have to override save() to pass DB > validation , any more excludes and a customized Form might be a better > approach; having the 'fields' option implies to me I can get away using > ModelForm instead of Form and have falsely in past tried to do so. > > -----Original Message----- From: Luke Plant > Sent: Tuesday, June 12, 2012 7:16 PM > To: django-developers@**googlegroups.com<django-developers@googlegroups.com> > Subject: ModelForms and the Rails input handling vulnerability > > > Hi all, > > On django-core we've been discussing an issue that was security related. > However, we decided it wasn't urgent enough to warrant a security > release, and so we're moving the discussion here (especially because we > couldn't agree on what to do). > > For the record, I brought up the issue initially, and favour the most > secure of the options outlined below, but I'll try to summarise with as > little bias as I can! > > The issue was brought up by the Rails input handling vulnerability a > while back [1]. To sum that up, it seems that a common Rails idiom is > something like, using Django syntax: > > MyModel.objects.create(****request.POST) > > (only more complicated). To avoid people having access to privileged > fields, you are supposed to set an attribute on MyModel to restrict the > fields that can be set in this way. This is a really poor design choice > (which they've finally admitted [2]), since it is insecure-by-default, > and led to the embarrassing github incident, and probably many more > unknown vulnerabilities in Rails apps. > > For us, we don't have this issue because we have the forms layer that > sits between the HTTP request and model creation, and that's the > recommended way to do things. > > However, in the case of ModelForms, you can easily get to a situation > where you've effectively got the same thing as Rails. In theory you've > got the forms layer, but in reality your form was autogenerated using > all the fields on the model. This happens if you use a ModelForm without > explicitly defining Meta.fields - which is the easiest thing to do since > it is less work. > > While you can use 'Meta.exclude' to remove specific fields, you still > have to remember to do that. > > This is particularly dangerous in two fairly common situations: > > 1) You add new fields to the model that are not supposed to be publicly > edited. > > 2) In the template, you are listing form fields individually, not just > doing {{ form.as_p }}, so you can't even see the fact that other fields > are editable, but the view/form code does allow an attacker to edit > those fields. > > Also, the UpdateView CBV makes it very easy to be vulnerable - it will > generate a ModelForm class with all fields editable by default. > > == Options == > > There were three main courses of action that we talked about on > django-core. > > = Option 1: Leave things as they are, just document the problem. This > was the least popular view on django-core. > > = Option 2: Deprecate Meta.exclude, but still allow a missing > Meta.fields attribute to indicate that all fields should be editable. > > The policy here is essentially this: if any fields the model are > sensitive, assume all are potentially, and require whitelisting, not > blacklisting. > > = Option 3: Make the 'Meta.fields' attribute a required attribute, which > must list all the model fields that should be editable. Without it, > ModelForms would not work at all. > > This also means deprecating Meta.exclude (it's redundant once you are > saying that all fields should be explicitly whitelisted). > > > Note that the admin would not be affected under any of these options - > the admin has its own mechanism for setting the Meta.fields, and "all > fields editable by default" is a good policy for something like the admin. > > For either 2) or 3), we would be making the change in Django 1.6, with a > deprecation path - faster than our normal deprecation path, but not > immediate. > > Also the UpdateView and CreateView CBVs would need modifying under at > least option 3, to match the requirements of the ModelForms they will > need to generate. > > == Comparison == > > == Option 1: This is the least secure, but most convenient - you have > several ways to specify which fields should be editable, you can use > whichever you like. "We're all consenting adults here". > > An argument in favour of keeping this is if we don't, people will just > use 'fields = [f.name for f in MyModel._meta.fields]' anyway. > > == Option 2: the retains some of the convenience of option 1, while > encouraging more careful handling of "sensitive" models. > > == Option 3: the most secure, the least convenient. You have to list all > fields for every ModelForm (except for cases of sub-classing forms, > where the base class's Meta.fields would still work, of course). > "Explicit is better than implicit". > > The option doesn't make an assumption that models are either 'sensitive' > or not. It is also more consistent than option 2: if you add a field to > a model, you know that if it is meant to be publicly editable, you have > to edit the ModelForms to add it, and if it is not meant to be editable, > you don't, because the list is always "opt in". > > ~ ~ ~ > > So - discuss! If you have other options to bring to the table, please > do. Apologies to the core devs if I missed or misrepresented something. > > > Thanks, > > Luke > > > [1] > http://chrisacky.posterous.**com/github-you-have-let-us-**all-down<http://chrisacky.posterous.com/github-you-have-let-us-all-down> > [2] > http://weblog.rubyonrails.org/**2012/3/21/strong-parameters/<http://weblog.rubyonrails.org/2012/3/21/strong-parameters/> > > -- > OSBORN'S LAW > Variables won't, constants aren't. > > Luke Plant || http://lukeplant.me.uk/ > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to > django-developers@**googlegroups.com<django-developers@googlegroups.com> > . > To unsubscribe from this group, send email to > django-developers+unsubscribe@**googlegroups.com<django-developers%2bunsubscr...@googlegroups.com> > . > For more options, visit this group at http://groups.google.com/** > group/django-developers?hl=en<http://groups.google.com/group/django-developers?hl=en> > . > > > Daniel Sokolowski > Web Engineer > Danols Web Engineering > http://webdesign.danols.com/ > Office: 613-817-6833 > Fax: 613-817-4553 > Toll Free: 1-855-5DANOLS > Kingston, ON K7L 1H3, Canada > > > Notice of Confidentiality: > The information transmitted is intended only for the person or entity to > which it is addressed and may contain confidential and/or privileged > material. Any review re-transmission dissemination or other use of or > taking of any action in reliance upon this information by persons or > entities other than the intended recipient is prohibited. If you received > this in error please contact the sender immediately by return electronic > transmission and then immediately delete this transmission including all > attachments without copying distributing or disclosing same. > > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to > django-developers@**googlegroups.com<django-developers@googlegroups.com> > . > To unsubscribe from this group, send email to > django-developers+unsubscribe@**googlegroups.com<django-developers%2bunsubscr...@googlegroups.com> > . > For more options, visit this group at http://groups.google.com/** > group/django-developers?hl=en<http://groups.google.com/group/django-developers?hl=en> > . > > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
