ModelForms and the Rails input handling vulnerability

[Luke Plant]

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/

[Alex Ogier]

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

[Karen Tracey]

On Tue, Jun 12, 2012 at 10:10 PM, Alex Ogier <alex....@gmail.com> wrote:

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.

But it may. If you have a template which renders specific fields, and yet the form is set to allow a wider set of fields than are actually rendered, client-side editing CAN result in the form allowing change to a field that had not been rendered in the template. The Django ModelForm doesn't know what fields were actually rendered in the HTML, it only knows what fields have been included/excluded from the ModelForm. You can post data for a field that was not rendered and it may pass validation and get saved.

Karen

[Gary Reynolds]

On 13 June 2012 09:16, Luke Plant <L.Pla...@cantab.net> wrote:

= 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.

+1 for option 2 as I've always felt like Meta.exclude was the wrong way to do go building a ModelForm which was supposed to have a subset of fields.

Given that the underlying Model definition may change, it would be easy to all of a sudden expose a newly added field. This would be particularly bad when in your template you use {{ form.as_p }} or similar. I never use Meta.exclude for this reason, always giving a whitelist anyway.

+0 for option 3 as I think the convenience of a missing Meta.fields is worthwhile, but I could live without it.

Another option could be to split ModelForm such that you could mix and match the functionality you require. Moving forward the ModelForm class could provide option 3 behaviour, and a newly introduced class could provide the option 2 behaviour. The only difference would be the alias to Meta.fields not existing results in all model fields, but this would be an explicit step.

Gary

[Alex Ogier]

Oof, you are right. I hadn't considered the wrench that templating
throws into the works. I've always done {% for field in form.fields %}
myself, but that's a bad assumption to make.

Best,
Alex Ogier

[Anssi Kääriäinen]

On 13 kesä, 02:16, Luke Plant <L.Plant...@cantab.net> wrote:
> 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!

<SNIP>

> = 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.

I support option 2. I will try to keep the reasoning part short. The
end of post is devoted to another idea.

Here are the reasons why I think Option 3 isn't worth the trouble:

1. Users will learn to use fields = [f.attname for f in
Permission._meta.fields]. At that point we haven't gained anything in
security, but made modelforms less convenient to use.

2. The fix doesn't actually fix the issue for all cases. You are still
allowed to use the same form in a way that renders additional fields
based on user permissions or if the user is authenticated. If you
reuse the same form in multiple views for example, or your template
contains some logic to add/remove fields dynamically.

Yes, option 3 is a bit more secure than option 2, but I don't find the
cost-benefit ratio correct. It will still not protect users from this
admittedly easy to do security mistake. So, I am -0 to option 3.



The short part is now over. I will throw in another idea I think is at
least worth presenting. The idea is that ModelForms contain attribute
contains_secure_data. Defaults to True. If True, then one must use
allowed_fields when initialising the form from request. So, you could
not do this for secure form:
MyForm(request.POST)
instead you would need to do:
MyForm(request.POST, allowed_fields=('field1', 'field2'))

Assume your model Document has three fields: title, body and creator.
If the creator edits his document, he would likely be able to change
the creator to somebody else when editing his document if the form is
defined like this using 1.4 behavior:

class DocumentForm:
class Meta:
model = Document

But, the developer could create this form:

class DocumentForm:
class Meta:
model = Document
fields = ('title', 'body')
contains_secure_data = False

This would would not need allowed_fields for form.__init__().

Alternatively, assume title shall not be changed on document edit.
Then the developer could use this form:

class DocumentForm:
class Meta:
model = Document
contains_secure_data = True

The developer would then use this form in create as:
DocumentForm(..., allowed_fields=('title', 'body'))
and in update as:
DocumentForm(..., allowed_fields=('body',))

My opinion is that the form.Meta isn't actually the right place to set
the security restriction. The same form can be used in different
places with different security restriction needs. The security
restriction should be done when the request data is added to the form.

The contains_secure_data could be thrown out for simplicity. However,
I do think it has its place. If allowed_fields (or form.meta.fields)
is always required, it leads to bad practices like using the "all
model.meta._fields". Ask the user if the form is security sensitive.
Only if it is, then ask for the field restrictions.

I'm not saying the above is the correct option. Just throwing in
ideas...

- Anssi

[Torsten Bronger]

Hall�chen!

Luke Plant writes:

> [...]

>
> == Options ==
>
> There were three main courses of action that we talked about on
> django-core.
>

> [...]

>
> = Option 2: Deprecate Meta.exclude, but still allow a missing
> Meta.fields attribute to indicate that all fields should be
> editable.

For our use cases, this would be very unfortunate. We have many
models in our app, and many fields in most of them. We "exclude"
most foreign key relationships, and sometimes fields which the user
must not be allowed to change. Option 2 would mean that we would
have huge name list in 80% of the ModelForms. Moreover, they needed
to be kept up-to-date, which is very error-prone.

Granted that it's worse if security is affected, but legibility and
DRY would be reduced too drastically in my opinion by option 2.

People in a similar situation like us WILL use

fields = [f.name for f in MyModel._meta.fields if f.name not in ["blah", "blubb"]]

but Django should not compel its users to such things.

If you add a new field, your automatic next thought should be how
(and whether) to expose it to the user. The other way round: If a
developer adds a sensitive field and doesn't look at the code where
the model is made editable to the user, this shows a careless
attitude a library cannot prevent anyway.

Moreover:

> The policy here is essentially this: if any fields the model are
> sensitive, assume all are potentially, and require whitelisting,
> not blacklisting.

I doubt that this is sufficiently frequently true so that it would
justify the reduce of legibility and DRY. The sensitive fields
usually are special cases, e.g. administrative or internal. If you
add further fields to a model, however, they are "ordinary" ones.
Well, at least this is my humble assessment and experience.
Therefore, "exclude" does a sensible job for its use case, and is a
reasonable compromise between security and comfort in my opinion.

Tsch�,
Torsten.

--
Torsten Bronger Jabber ID: torsten...@jabber.rwth-aachen.de
or http://bronger-jmp.appspot.com

[Daniel Sokolowski]

(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.

--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to
django-develop...@googlegroups.com.
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.

[Michael]

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.s...@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

To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to django-developers+unsubscribe@googlegroups.com.

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

[Carl Meyer]

Hi Anssi,

On 06/13/2012 12:08 AM, Anssi K��ri�inen wrote:

> On 13 kes�, 02:16, Luke Plant <L.Plant...@cantab.net> wrote:
>> 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!
> <SNIP>
>> = 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.
>
> I support option 2. I will try to keep the reasoning part short. The
> end of post is devoted to another idea.
>
> Here are the reasons why I think Option 3 isn't worth the trouble:
>
> 1. Users will learn to use fields = [f.attname for f in
> Permission._meta.fields]. At that point we haven't gained anything in
> security, but made modelforms less convenient to use.

No, even if some users do this we will still have gained quite a lot.
There will always be some way for users to do whatever they like - this
is Python, and that's a good thing. But it's critically different when
the user must actively choose to circumvent a secure default, as opposed
to Django making a risky default the obvious path for naive newbies.

In any case, I think in actual fact only a small percentage of users
will do this; for many ModelForms just specifying the desired fields
will be simpler and easier.

> 2. The fix doesn't actually fix the issue for all cases. You are still
> allowed to use the same form in a way that renders additional fields
> based on user permissions or if the user is authenticated. If you
> reuse the same form in multiple views for example, or your template
> contains some logic to add/remove fields dynamically.

I don't see how this is relevant. Dynamic addition or removal of fields
is an uncommon edge case; it's way beyond the "insecure defaults" issue
and well into "if you're doing advanced stuff, you just have to know
what you're doing" territory.

> Yes, option 3 is a bit more secure than option 2, but I don't find the
> cost-benefit ratio correct. It will still not protect users from this
> admittedly easy to do security mistake. So, I am -0 to option 3.

Option 3 is the only option that addresses the real problem, which is
that the simplest and easiest way for a naive newbie to create a
ModelForm sets them up for trouble.

Asking for a solution that "protects users" is the wrong goal to begin
with - there's always a way to be insecure if you want. Our
responsibility as a framework is not to protect users, it's just to stop
actively encouraging them to do the wrong thing.

Carl

[Carl Meyer]

Hi Luke,

Thanks for identifying this issue and continuing to push for a resolution.

On 06/12/2012 05:16 PM, Luke Plant wrote:
> == 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 favor option 3; I think the Rails issue should have taught us that
exposing database data to modification by remote users is something that
deserves to always be done explicitly.

I think the most pressing issue is the behavior of UpdateView, because
it allows a naive developer to bypass the forms layer entirely (since it
automatically constructs a ModelForm subclass if given a model). As
others have pointed out, our situation in general is somewhat better
than Rails because we have the explicit Form layer; even if a developer
constructs a ModelForm with implicit fields they at least have to think
about the fact that this form should be responsible for data validation.
But this is not true with UpdateView; I think UpdateView is actually a
very close parallel to the Rails issue.

So while I favor option 3 for ModelForms themselves, I also think that
even if this discussion should end up rejecting that, we should still at
the very least require an explicit "fields" parameter for UpdateView.

Carl

[Alex Ogier]

On Tue, Jun 12, 2012 at 7:16 PM, Luke Plant <L.Pla...@cantab.net> wrote:
>

> = 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).
>

What about a fourth option:

= Option 4: Make the 'Meta.fields' attribute required, but allow a
semaphore value ("__all__"?) that enables the insecure shortcut. We
can plaster it with warnings in the docs, which people will inevitably
check when the error pops up, "Meta.fields is required." There are
clearly some use cases where the maintenance burden of whitelisting
all fields is just not worth it and people will resort to hacks if we
hamstring the current behavior. It also gives an upgrade path for
people who would otherwise need to rearchitecture their applications
if the Meta.fields attribute suddenly became required. (I can imagine
models with hundreds of fields, or worse, dynamically generated
fields.)

Like Carl Meyer, I don't think Django's goals should be to protect
developers from themselves, but I agree that it is important that the
path of least resistance be secure in all areas. There is considerable
value in "A form that is automagically scaffolded from a Model" above
and beyond "A form that does default
model-field-type-to-form-field-type mappings" which is what ModelForms
will become under options 2 and 3. So long as that is an explicit
choice, I don't think there is too much harm in letting people do that
if they want to. In fact, the reason I consider Option 1 viable is
precisely because I consider the creation of a ModelForm to be
precisely that kind of choice, but I can see how having the default be
include-all is a problem.

In summary:

+0 on option 1. Making a ModelForm is already a choice to expose a
model directly.
-1 on option 2. Default is still insecure, and Meta.exclude has use cases.
+0 on option 3. It's like my option 4, but requires [for field in
Model._meta.fields] hackery to work around.
+1 on option 4. Auto-generated fields are useful, and this allows an
explicit opt-in.

Best,
Alex Ogier

[Anssi Kääriäinen]

On 13 kesä, 20:58, Carl Meyer <c...@oddbird.net> wrote:
> > 2. The fix doesn't actually fix the issue for all cases. You are still
> > allowed to use the same form in a way that renders additional fields
> > based on user permissions or if the user is authenticated. If you
> > reuse the same form in multiple views for example, or your template
> > contains some logic to add/remove fields dynamically.
>
> I don't see how this is relevant. Dynamic addition or removal of fields
> is an uncommon edge case; it's way beyond the "insecure defaults" issue
> and well into "if you're doing advanced stuff, you just have to know
> what you're doing" territory.

The point is the same form with the same fields can be used in
multiple places. There is no need for dynamic addition and removal of
fields in Python code. The dynamism is in which fields to display -
not which fields the form contains. You render only field1, except for
superusers you render field1 and field2. Thus, the fields = ('field1',
'field2') in the form's meta doesn't do the security restriction you
want for this case - the place to do the restriction is in the form
init call:

if user is superuser:
form = MyModelForm(request.POST, allowed_fields=('field1',
'field2'))
else:
form = MyModelForm(request.POST, allowed_fields=('field1',))

Part of the problem is that it is easy to think that if you don't have
the fields in the HTML form, then the user can't edit the fields.

It would be useful to know how many projects contain ModelForms with
no field restrictions, and only display a subset and always the same
subset of the fields.

- Anssi

[David Danier]

Hi list,

as I use "exclude" most of the times (to hide sensible fields from the
user) I don't like the idea of this beeing removed. Perhaps another
solution might be to force developers to either define fields _or_
exclude, so you have to at least think about this..without going through
too much trouble. Minimal form containing all fields would be:

class SomeModelForm(forms.ModelForm):
class Meta:
exclude = []

Everything else would lead to develpers using thinks like mentioned
before ([f.name for f in MyModel._meta.fields if f.name not in ...]). I
really think this only makes things more ugly.

David

[Carl Meyer]

On 06/13/2012 12:57 PM, Anssi K��ri�inen wrote:
> The point is the same form with the same fields can be used in
> multiple places. There is no need for dynamic addition and removal of
> fields in Python code. The dynamism is in which fields to display -
> not which fields the form contains. You render only field1, except for
> superusers you render field1 and field2. Thus, the fields = ('field1',
> 'field2') in the form's meta doesn't do the security restriction you
> want for this case - the place to do the restriction is in the form
> init call:
>
> if user is superuser:
> form = MyModelForm(request.POST, allowed_fields=('field1',
> 'field2'))
> else:
> form = MyModelForm(request.POST, allowed_fields=('field1',))

Ah, I see better what you're getting at now. I still think that using
the same form for two purposes in this way is an unusual case, and
probably not the best approach. I've never done it myself that I can
recall, I'd use two different ModelForm subclasses, probably one
subclassed from the other.

If someone is using the same ModelForm for both a superuser and a
regular user, I think it's pretty reasonable to expect them to realize
that that leads to some potential security issues, and do the
appropriate checks themselves.

IOW, unlike implicit Meta.fields, this doesn't seem to me like a case
where Django's defaults are actively leading them astray.

> Part of the problem is that it is easy to think that if you don't have
> the fields in the HTML form, then the user can't edit the fields.

Yes, this is really at the root of the whole issue. I don't see a good
way to address it directly - but we can address it indirectly by
ensuring developers have to think explicitly about which fields their
ModelForm is going to allow through.

Carl

[Peter]

Hi all,

Can I throw in option 5:

We leave ModelForms as they are, but emit a warning if you only partially render the form?

I'm not sure how feasible this is, but presumably we could keep track of which fields have been rendered for a given form instance?

That way, if you render the whole form ( {{ form.as_p }} ) you'll see your new sensitive field appear in the page. If you manually render the form, you'll get a warning.

One problem would be excessive warnings if you went further and hand craft the HTML - does anyone do that?

Cheers,

Peter

[Carl Meyer]

Hi Peter,

On 06/13/2012 02:55 PM, Peter wrote:
> Can I throw in option 5:
>
> We leave ModelForms as they are, but emit a warning if you only
> partially render the form?
>
> I'm not sure how feasible this is, but presumably we could keep track of
> which fields have been rendered for a given form instance?
>
> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
> your new sensitive field appear in the page. If you manually render the
> form, you'll get a warning.

I've thought about this. The main problem is that the implementation is
quite difficult in practice: at what point do you perform the check?
There isn't any such thing as an "ok, I think I'm all done rendering
this form now, tell me if I did it right" hook.

There's at least one third-party app out there that does this
(https://github.com/ulope/django-careful-forms), but it registers all
forms in a thread-local and performs the check in a middleware; that's
not something I think belongs in core Django.

> One problem would be excessive warnings if you went further and hand
> craft the HTML - does anyone do that?

Yes.

Carl

[Doug Blank]

On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer <ca...@oddbird.net> wrote:
> Hi Peter,
>
> On 06/13/2012 02:55 PM, Peter wrote:
>> Can I throw in option 5:
>>
>> We leave ModelForms as they are, but emit a warning if you only
>> partially render the form?
>>
>> I'm not sure how feasible this is, but presumably we could keep track of
>> which fields have been rendered for a given form instance?
>>
>> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
>> your new sensitive field appear in the page. If you manually render the
>> form, you'll get a warning.
>
> I've thought about this. The main problem is that the implementation is
> quite difficult in practice: at what point do you perform the check?
> There isn't any such thing as an "ok, I think I'm all done rendering
> this form now, tell me if I did it right" hook.

I, too, was thinking about this kind of solution. In fact, it came up
for me the other day because I had forgotten to exclude a field that I
did not have on the form, and so the value ended up getting wiped out
when I saved. So, perhaps a solution that prevented others from adding
fields could also be a solution that checked to make sure that the
form was editing all fields it should be.

What about a {% validate %} tag in the form which would do a runtime
check to make sure that all non-excluded fields had been rendered?

-Doug

> There's at least one third-party app out there that does this
> (https://github.com/ulope/django-careful-forms), but it registers all
> forms in a thread-local and performs the check in a middleware; that's
> not something I think belongs in core Django.
>
>> One problem would be excessive warnings if you went further and hand
>> craft the HTML - does anyone do that?
>
> Yes.
>
> Carl
>

> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.

> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.

[Carl Meyer]

On 06/13/2012 06:12 PM, Doug Blank wrote:
> On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer <ca...@oddbird.net> wrote:
>> On 06/13/2012 02:55 PM, Peter wrote:
>>> Can I throw in option 5:
>>>
>>> We leave ModelForms as they are, but emit a warning if you only
>>> partially render the form?
>>>
>>> I'm not sure how feasible this is, but presumably we could keep track of
>>> which fields have been rendered for a given form instance?
>>>
>>> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
>>> your new sensitive field appear in the page. If you manually render the
>>> form, you'll get a warning.
>>
>> I've thought about this. The main problem is that the implementation is
>> quite difficult in practice: at what point do you perform the check?
>> There isn't any such thing as an "ok, I think I'm all done rendering
>> this form now, tell me if I did it right" hook.
>
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.
>
> What about a {% validate %} tag in the form which would do a runtime
> check to make sure that all non-excluded fields had been rendered?

Yeah, this would be a cleaner way to implement the check. I'd like to
see it proved out as a third-party add-on before discussing it for core.
One of the unresolved issues in my mind is what it should actually _do_
if you haven't rendered all the fields on the form (blow up in DEBUG
mode only? Just a call to warnings.warn?).

And since it's opt-in (and easy to forget or not bother with) I'm not
sure that by itself it's a satisfactory solution to the original problem
of implicit Meta.fields.

Carl

[Alex Ogier]

On Jun 13, 2012 8:12 PM, "Doug Blank" <doug....@gmail.com> wrote:
>
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.
>

That suggests an idea to me. Perhaps the best way to check this isn't on the way out in the template renderer, but rather on the way back in in the form validation. If the form doesn't get back exactly those fields it sent out then you know that for whatever reason, the field was unable to make a round trip. In a ModelForm with implicit fields this is the root cause of this whole security dilemma.

This solution is parsimonious because it's easy to explain: "If a form didn't get back all the fields it expected then there was probably an error of omission rendering it. This causes a security hole where an attacker can modify fields the developer doesn't expect, for further examples see <link to description of rails debacle>"

It's an easy thing to justify turning on in an opt-out fashion, Meta.allow_partial_submissions or something.

Best,
Alex Ogier

[Atul Bhouraskar]

Hi All,

I can see two separate (but related) problems that need to be solved:

1. Ensure that sensitive fields are not accidentally exposed to users (e.g when a database model is updated to include one), and
2. Provide a means to confirm whether the fields as rendered and submitted exactly match the intent of the form.

This is my proposal for problem 1:

Add an "is_sensitive" or similarly named boolean attribute to model fields. This field would be False by default and would be set to True for fields that contain sensitive data e.g:

class Article(models.Model):
  text = models.TextField()
  secret_notes = models.TextField(is_sensitive=True)

Now a ModelForm would by default exclude any field that is marked as sensitive, unless it is explicitly included via the "fields" attribute.

The advantage of the above is that it allows mechanisms other than Forms to enforce rules associated with sensitive data and could potentially be double checked while being rendered as well. For developers who don't care about sensitive data, nothing changes, unless the is_sensitive attribute is made True by default (which would be a pain).

Regards,


Atul

Carl

[Anssi Kääriäinen]

On 14 kesä, 03:12, Doug Blank <doug.bl...@gmail.com> wrote:
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.

I hadn't realized all fields present on the Python form but not in the
HTML form will get overwritten to NULL... The above makes me a tad
more "-" on the require fields always proposal. The problem is there
only for forms used for model creation. If you use the form both for
update and creation, you will soon enough see that something funny is
going on as part of your fields are getting set to NULL on
form.save().

So, to hit the problem the user would need to:
1. Have a ModelForm with no field restrictions in Python.
2. Render only part of the fields.
3. All non-rendered fields must be null/blank=True for the form to
work at all.
4. Not use the form in update views.

(there are of course corner cases - the model has "deleted" attribute
which is usually null - you don't include that in the HTML form - the
user can update the delete field and it doesn't show up in testing)

This seems to be different from what Rails do: they have
update_attributes which updates all model attributes present in the
request, but lefts all others untouched. So, in Rails if you render
only part of the fields in update view, then you will not get the non-
included fields overwritten to NULL, which conveniently hides the
problem that the fields are in fact editable through the request.

To me it seems there is no similar attack vector against Django's
implementation as there were against Rails.

I hope my quick investigations are correct - double checking what
Rails do and the four conditions above is suggested...

- Anssi

[Torsten Bronger]

Hall�chen!

Alex Ogier writes:

> [...]

>
> That suggests an idea to me. Perhaps the best way to check this
> isn't on the way out in the template renderer, but rather on the
> way back in in the form validation. If the form doesn't get back
> exactly those fields it sent out then you know that for whatever
> reason, the field was unable to make a round trip.

But can one guarantee that fields rendered in the browser are also
sent back in the POST request? Even worse, how about non-browser
requests?

[Alex Ogier]

On Jun 14, 2012 12:48 AM, "Anssi Kääriäinen" <anssi.ka...@thl.fi> wrote:
>
> This seems to be different from what Rails do: they have
> update_attributes which updates all model attributes present in the
> request, but lefts all others untouched. So, in Rails if you render
> only part of the fields in update view, then you will not get the non-
> included fields overwritten to NULL, which conveniently hides the
> problem that the fields are in fact editable through the request.
>
> To me it seems there is no similar attack vector against Django's
> implementation as there were against Rails.
>

Right, the Django situation is already considerably more secure than the Rails status quo. They have a whitelist or blacklist of attributes that they have declared "accessible", independent of forms, making it easy to misunderstand  that any form can update any accessible attribute regardless of the input fields the developer has included.

Our forms only validate the fields they explicitly or implicitly include. The only way to get a security hole is to have a mismatch between the fields in your python form and the input fields in your HTML. Since all our forms are explicit, it is feasible to catch that scenario and throw an error, which I think we should do.

Best,
Alex Ogier

[Alex Ogier]

On Jun 14, 2012 1:54 AM, "Torsten Bronger" <bro...@physik.rwth-aachen.de> wrote:
>
> But can one guarantee that fields rendered in the browser are also
> sent back in the POST request?  Even worse, how about non-browser
> requests?
>

Ugh. I forget that checkboxes don't return anything else when they are unchecked. Kind of throws a wrench in my plan.

Best,
Alex Ogier

[Anssi Kääriäinen]

On 14 kesä, 09:03, Alex Ogier <alex.og...@gmail.com> wrote:
> Right, the Django situation is already considerably more secure than the
> Rails status quo. They have a whitelist or blacklist of attributes that
> they have declared "accessible", independent of forms, making it easy to
> misunderstand  that any form can update any accessible attribute regardless
> of the input fields the developer has included.
>
> Our forms only validate the fields they explicitly or implicitly include.
> The only way to get a security hole is to have a mismatch between the
> fields in your python form and the input fields in your HTML. Since all our
> forms are explicit, it is feasible to catch that scenario and throw an
> error, which I think we should do.

There is an additional important distinction between Django and Rails:
In Django, when a field is not part of the POST, but it is part of the
ModelForm, it will be validated and saved on basis of the POST value
(that is, set to None as it is missing), In Rails, it will keep its
original value from the database.

Thus, in Django it is easier to spot that all of the fields in the
ModelForm are editable due to "this field is required" errors on
form.is_valid(), or the field getting set to NULL on form.save().
Also, users are likely to restrict the fields by hand even if they
render only a part of them, as otherwise the fields will be
overwritten to NULL on save.

It would be useful to warn about missing POST keys. In addition to the
security issue, missing HTML form elements will likely result in
"overwrite to NULL" issues. We can not warn about checkboxes, as those
are not always part of the POST even if they are in the HTML form, but
for other fields we should be able to do the warnings. There are use
cases where the warnings are just noise, so add some way to suppress
the.

I made a _very_ quick branch made on the above idea: on
ModelForm.is_valid() the data and self.fields are checked for possible
missing pieces of data. I don't know if this idea is practical due to
differences in browsers. The branch is available here:
https://github.com/akaariai/django/compare/warn_missing_keys

I don't see our ModelForms situation as comparable to the Rails
situation. The scope for security issues is much smaller for us than
for Rails.

- Anssi

[Erik Romijn]

Hello Luke and others,

On Jun 13, 2012, at 1:16 AM, Luke Plant wrote:
> 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).

Especially after seeing Jessica McKellar's keynote at Djangocon EU, on
the experience of novice developers when using Django, I strongly feel
we should not leave the situation as it is. Although this risk and it's
mitigations may be obvious to people on this list, a more novice
developer using Django is much more likely to overlook this issue.

Many have made valid points that option two, three, or any other, may be
seen as difficult for the developer. And that may motivate developers to
use any of the mentioned Python one-liners to revert functionality back
to the existing situation. However, with any system we devise to make
anything secure, there are lazy shortcuts to break that security. But if
we do nothing, everyone will be exposed to this risk. If we tighten
security, and some projects will use methods to revert this security
improvement, all the others are still more secure than before. Obviously,
it's also our role to make sure we provide proper example code in the
documentation.

Basically, even if every existing Django project would, being lazy, use
these one-liners in their existing forms and therefore receive no
security benefit from this change, we would still be making an
improvement for all the new projects to be started.


Having said that, there's the question of option two or three. If most
forms we use in our Django apps already exclude one or more fields
today, there is little to no extra effort in option 3 compared to
option 2 - as those forms would require the same work when Meta.exclude
is dropped. If the effort for both options is equal (for the developers),
then I figure option 3 would be best.

Looking at my own projects, all editing of data in ModelForms is tied
to authentication, with a FK to the user. Which means all my ModelForms
use Meta.exclude already. Therefore, there is no difference in effort
between option 2 or 3 for those apps. But my apps are just a small sample.


Summary: I feel we should tighten this. The fact that this will be worked
around by some, is not a reason for me to simply do nothing. When
considering option 2 vs 3, we should, probably, also consider the actual
impact: how often do we create ModelForms with neither Meta.fields nor
Meta.exclude today?

cheers,
Erik

[Anssi Kääriäinen]

On 17 kesä, 23:14, Erik Romijn <e...@erik.io> wrote:
> Especially after seeing Jessica McKellar's keynote at Djangocon EU, on
> the experience of novice developers when using Django, I strongly feel
> we should not leave the situation as it is. Although this risk and it's
> mitigations may be obvious to people on this list, a more novice
> developer using Django is much more likely to overlook this issue.

I find the option of raising warnings or errors on missing data
elements the best option forward. There is one unfortunate downside,
that is we can't do this for checkboxes. So, it would be possible to
have a hidden-editable checkbox field.

The scope of this vulnerability is different than in Rails. In Rails
the fields not included in the HTML form are not edited at all, in
Django they are edited always - if there is no data in the sent HTML
form, they are set to None. Thus, you will usually spot they are
editable.

This can bite the user if there is a security sensitive nullable
field, which usually store the null value (so that you won't easily
spot the overwrite to null), you use a modelform with the field
present, and render only part of the fields of that form.

Adding a warning or error on missing data elements would be a security
improvement, and a usability improvement, too. If the field is not
nullable, you will not see the validation error, as the field isn't
part of the form you are rendering and you are left wondering why
didn't the form validate. If the field is nullable, and you don't spot
the overwrite to null, then you got a potential data loss or security
issue.

If the error/warning would be added, the scope of this security issue
would be limited to checkboxes, and even then to checkboxes which
usually contain the non-checked value, are security sensitive and you
are using a ModelForm with the checkbox field present, but not
rendering it in your html form.

If it is decided that 'fields' is required I think we should still add
the warning for missing data elements. Even if 'fields' is present in
the form's meta, you can do the same mistake this whole thread is
about.

For the record, Rails handles checkboxes by adding a hidden input with
the same name just above the checkbox. If the checkbox isn't checked,
then the hidden field's value is sent, if the checkbox is checked, the
checkbox value will be sent.

- Anssi

[Honza Král]

Thanks Luke for writing this up and representing all views. I am the
proponent of "we are all adults here".

On Thu, Jun 14, 2012 at 6:48 AM, Anssi Kääriäinen
<anssi.ka...@thl.fi> wrote:
> ...

> I hadn't realized all fields present on the Python form but not in the
> HTML form will get overwritten to NULL... The above makes me a tad
> more "-" on the require fields always proposal. The problem is there
> only for forms used for model creation. If you use the form both for
> update and creation, you will soon enough see that something funny is
> going on as part of your fields are getting set to NULL on
> form.save().
>
> So, to hit the problem the user would need to:
>  1. Have a ModelForm with no field restrictions in Python.
>  2. Render only part of the fields.
>  3. All non-rendered fields must be null/blank=True for the form to
> work at all.
>  4. Not use the form in update views.

Exactly - the combination of things that need to happen for this is so
complicated that I am perfectly ok with leaving things as they are.
(add 5. No tests for the form/view)

I believe exclude is way more useful than fields (I do see the
security advantage but in my mind this is the case where convenience
beats security, also still afraid of the fields = [f.name for f in
...]).

I would be most happy with a modified Option 1 - require exclude OR
fields on ModelForm.Meta. This should force the user to think about
this issue, when these attributes are missing we can throw a
reasonable error and refer the user to the docs for more info on how
their choices affect the overall security of the app.

so: -1 on options 2 and 3, +1 on option 1 + require exclude or fields

[Ian Lewis]

Hi,

I'm with Carl in supporting option 3. I think have always thought that
ModelForms were unsafe and requiring the fields option would go a long
way to making them safer. I don't think I'm stupid and I've personally
run into this issue. I have almost *NEVER* run into a case where I
want all fields on a Model to be updatable by the user and I now
always define the fields Meta attribute.

However, I'm not necessarily against leaving the current functionality
in Django for a few releases but with warning messages.

> So, to hit the problem the user would need to:
> 1. Have a ModelForm with no field restrictions in Python.
> 2. Render only part of the fields.
> 3. All non-rendered fields must be null/blank=True for the form to
> work at all.
> 4. Not use the form in update views.

No, the user would have to not miss of those things when developing.
Having a field get updated to NULL when not in the HTML does not
guarantee the developer will notice it. Maybe the field's default is
NULL? Or NULL is a perfectly valid value and they just don't notice? I
have been doing Django and Python for years at a high level and I have
done this.

On Tue, Jun 19, 2012 at 8:42 PM, Honza Král <honza...@gmail.com> wrote:
> I believe exclude is way more useful than fields (I do see the
> security advantage but in my mind this is the case where convenience
> beats security, also still afraid of the fields = [f.name for f in
> ...]).

Personally, I don't think convenience EVER beats security in a
framework like Django (if at all). This is the common "Oh but that
will never happen to me!" syndrome. Sane defaults that can be
overridden are going to always be better.

--
Ian

http://www.ianlewis.org/

[Honza Král]

> On Tue, Jun 19, 2012 at 8:42 PM, Honza Král <honza...@gmail.com> wrote:
>> I believe exclude is way more useful than fields (I do see the
>> security advantage but in my mind this is the case where convenience
>> beats security, also still afraid of the fields = [f.name for f in
>> ...]).
>
> Personally, I don't think convenience EVER beats security in a
> framework like Django (if at all). This is the common "Oh but that
> will never happen to me!" syndrome. Sane defaults that can be
> overridden are going to always be better.

TL;DR: Convenience beats security when it would otherwise result in
people circumventing the security alltogether (== using fields =
[f.name for MyModel._meta.fields])


I do agree that security is important and a framework worth it's name
should do everything it can to help people develop secure apps.

However I believe (from talking to people around me and looking at my
own code) that if we force people to use fields and nothing else the
majority of people will do [f.name for f in MyModel._meta.fields]
which is both terrible and insecure thus completely negating the
desired effect (worst of both worlds - insecure and ugly/hard to use).

If we instead just force users to specify one of fields or exclude
(even if just saying exclude = ()) we accomplish our goal - the goal
is not to make the app secure no matter what the developer thinks,
it's forcing the user to think about the implications of their choices
and pointing them to the right place. (if you don't supply exclude of
fields you get an error with a link to docs)


For me requiring fields falls on the same level as requiring super
strong password (one lowercase, one uppercase, a digit and a special
character) and requiring users to change them weekly while not
repeating 10 last passwords. In theory that's very good security (I
know it's not perfect and that http://xkcd.com/936/ is valid, I have
seen this requirement quite often though), but if you just force it on
people without the necessary education etc it will just result in
majority of people writing their password down on the other side of
their keyboard (I have seen this personally way too many times).

We cannot force this on users, we must make them aware and educate
them - I cannot stress this enough that I don't believe we can strong
arm people into doing security right, we need to make them aware of
the possible problem and provide them with good tools to help them
deal with it.

[Anssi Kääriäinen]

On 14 kesä, 08:53, Torsten Bronger <bron...@physik.rwth-aachen.de>
wrote:

> Hall chen!
>
> Alex Ogier writes:
> > [...]
>
> > That suggests an idea to me. Perhaps the best way to check this
> > isn't on the way out in the template renderer, but rather on the
> > way back in in the form validation. If the form doesn't get back
> > exactly those fields it sent out then you know that for whatever
> > reason, the field was unable to make a round trip.
>
> But can one guarantee that fields rendered in the browser are also
> sent back in the POST request?  Even worse, how about non-browser
> requests?

One way is to have a .is_valid(safe_fields=list_of_fields) bypass. You
could conveniently use form.is_valid(safe_field=form.fields) if you
want to take chances with security. In normal use you would need to
manually whitelist checkboxes, otherwise this should just work. This
is safer than requiring fields in form.meta, and protects against
hidden overwrites to NULL, too.

Even safer is to require always whitelisting all allowed data elements
in the is_valid() call. This is the most secure approach.

It is pointless to debate about the approaches on the amount of
security they give. This is all about the amount of convenience we are
willing to sacrifice for the amount of security gained. To me it seems
there isn't much security to be gained by always requiring fields, and
there is much convenience lost. Still, there isn't any one right
opinion here. BDFL decision seems likely here.

The validation stage checking seems worth more investigation to me. If
we can pull a nice usable and secure API, this would be the best
choice in my opinion. Checkboxes are the real problem here.
Sacrificing some security for convenience and one could just skip
checkbox fields from the checking. If more security is wanted, then
require manual whitelisting of checkbox fields.

- Anssi

[Luke Plant]

This is an old thread, but we never came to a conclusion.

I'll respond to Anssi below, and then add my own ideas.

> I find the option of raising warnings or errors on missing data
> elements the best option forward. There is one unfortunate downside,
> that is we can't do this for checkboxes. So, it would be possible to
> have a hidden-editable checkbox field.

I don't think this is a great solution at this point. The
hidden-editable checkbox field is going to be backwards incompatible in
a serious way, because many forms are rendered manually in HTML, and
they will break.

I'm not convinced that warnings are ever a useful security mechanism,
because they are so easily ignored. In production, they are invisible by
default, and in development they are only visible if you look at the
devserver output.

Further, when a warning is correct, it shouldn't be a warning, it should
be an error. When the warning is in fact incorrect, then it is a
nuisance, and can easily lead to people ignoring or silencing all
warnings, unless we also provide an easy and very fine grained way to
deal with them. Warnings that are only sometimes correct always do the
wrong thing.

This contrasts with deprecation warnings, for example, which are always
correct - because the functionality is indeed going to go away, and you
do need to do something about it, but it isn't an error because the code
is working OK now. The presence of the warning encourages you to do the
right thing to get rid of it - migrate your code.


My idea:

I think, given the lack of consensus in this thread, that we have to opt
for a conservative option.

I like Alex Ogier's solution of a sentinel "__all__" flag. This would be
introduced for ModelForm and UpdateView using a deprecation process, so
that a form without one of 'fields' or 'exclude' will raise a
PendingDeprecation warning initially, and eventually be illegal.

Both the '__all__' shortcut and the use of 'exclude' will be plastered
with warnings in the docs, and the docs will be re-written to assume
that you are going to provide 'fields' (at the moment the opposite is
assumed).

How does that sound?


Luke


--
"If you're not part of the solution, you're part of the
precipitate." (Steven Wright)

Luke Plant || http://lukeplant.me.uk/

[Aymeric Augustin]

Le 4 févr. 2013 à 14:35, Luke Plant <L.Pla...@cantab.net> a écrit :

> I like Alex Ogier's solution of a sentinel "__all__" flag. This would be
> introduced for ModelForm and UpdateView using a deprecation process, so
> that a form without one of 'fields' or 'exclude' will raise a
> PendingDeprecation warning initially, and eventually be illegal.
>
> Both the '__all__' shortcut and the use of 'exclude' will be plastered
> with warnings in the docs, and the docs will be re-written to assume
> that you are going to provide 'fields' (at the moment the opposite is
> assumed).
>
> How does that sound?

Hi Luke,

This sounds like a good compromise between security and usability.

I've created a ticket to avoid losing track of this topic again:
https://code.djangoproject.com/ticket/19733

--
Aymeric.

[Emil Stenström]

On Monday, 4 February 2013 15:06:18 UTC+1, Aymeric Augustin wrote:

Hi Luke,

This sounds like a good compromise between security and usability.

I just want to add another voice of support for Option 3 to this thread.

I'm one of the developers for a large site, with ~40 apps, that has grown organically over time. Fixing all of them to have "fields" properly defined will be a lot of work, but it's well worth it. Accidentally opening up your models for writing is FAR to easy to do accidentally as things stand right now.

I understand why this started on the security list, this is serious.

[Anssi Kääriäinen]

My main gripe against this implementation is that the check against
which fields are allowed is done at the wrong point. The right point
is when the POST dict is passed to the form, and the fix is to have a
mandatory allowed_fields argument at that point.

If the "these fields are editable in this form" check is done at form
class creation time, then it is still possible to use the same form in
multiple places, and accidentally expose fields to unauthorized users
that way. It is actually somewhat likely to happen in the cases where
there could be a security issue currently.

As for the seriousness of this issue: this isn't that serious.
Currently every POST will set all fields not present in the HTML form
to NULL. So, to have a problem you will need to have models with
nullable fields not present in the HTML form. If this is the case you
will usually spot the error as your data is silently overwritten to
NULL on every form submission. Now, if the usual value is NULL, and
setting to non-NULL value requires authorization, then and only then
you will have security problem that is hard to spot. Such a case could
be "published_date" for example.

This is different from Rails. In Rails fields not present in the HTML
POST kept their original value, and thus it was really easy to miss
that the missing fields were actually editable.

Still, I have to agree that there is a possible security issue, and
the compromise solution forces developers to recognise this potential
issue.

- Anssi