(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: [email protected]
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
[2] 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 [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
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 [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
