On Tue, Jun 12, 2012 at 7:16 PM, Luke Plant <l.plant...@cantab.net> wrote:
> 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".
>
I'm +1 on option 1: just keep it the way it is. The reason is that we
aren't in the same situation as Rails. We have explicit ModelForms
that conform loudly and obviously to the model. These are better than
the Rails status quo for two main reasons:
1) They are explicitly handling validation, even for implicit fields.
When you create a ModelForm you are intentionally defining a class
whose sole purpose is to manage validation of user input and pass it
to a model. It's very clear that the class sits on an untrusted
boundary, so people are less likely to say "Screw it, I'm just gonna
use the implicit scaffold my framework provides." They have to think
at least a little bit about what fields belong on their form, and if
the choice is "Everything by default" then at least it is a choice.
2) ModelForms *do* have a whitelist, in the sense that the fields they
accept in POST are exactly those fields that they generate in GET. So
far as I know no model fields other than primary keys use hidden input
widgets by default, so it's rather obvious when something changes. We
don't have the Rails problem where one dev makes a scaffold form for
an insecure model and another one adds a secure field to the model
opening up a security hole, because it's easy to notice when a
password field or top secret checkbox appears on a random form. No one
can sneak extra unexpected fields past a developer by editing HTML
client side, because if the field wasn't rendered to HTML it's not
going to validate.
My two cents.
Best,
Alex Ogier
--
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.
