Re: ModelForms and the Rails input handling vulnerability
On 12 helmi, 14:10, Emil Stenströmwrote: > 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. 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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers?hl=en. For more options, visit https://groups.google.com/groups/opt_out.
Re: ModelForms and the Rails input handling vulnerability
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. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers?hl=en. For more options, visit https://groups.google.com/groups/opt_out.
Re: ModelForms and the Rails input handling vulnerability
Le 4 févr. 2013 à 14:35, Luke Planta é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. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers?hl=en. For more options, visit https://groups.google.com/groups/opt_out.
Re: ModelForms and the Rails input handling vulnerability
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/ -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers?hl=en. For more options, visit https://groups.google.com/groups/opt_out.
Re: ModelForms and the Rails input handling vulnerability
On 14 kesä, 08:53, Torsten Brongerwrote: > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
> On Tue, Jun 19, 2012 at 8:42 PM, Honza Králwrote: >> 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. -- 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.
Re: ModelForms and the Rails input handling vulnerability
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álwrote: > 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/ -- 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.
Re: ModelForms and the Rails input handling vulnerability
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äinenwrote: > ... > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 17 kesä, 23:14, Erik Romijnwrote: > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 14 kesä, 09:03, Alex Ogierwrote: > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Jun 14, 2012 1:54 AM, "Torsten Bronger"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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Jun 14, 2012 12:48 AM, "Anssi Kääriäinen"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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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? Tschö, Torsten. -- Torsten BrongerJabber ID: torsten.bron...@jabber.rwth-aachen.de or http://bronger-jmp.appspot.com -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 14 kesä, 03:12, Doug Blankwrote: > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 On 14 June 2012 10:18, Carl Meyerwrote: > On 06/13/2012 06:12 PM, Doug Blank wrote: > > On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer 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 > > -- > 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. > > -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Jun 13, 2012 8:12 PM, "Doug Blank"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 " It's an easy thing to justify turning on in an opt-out fashion, Meta.allow_partial_submissions or something. 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.
Re: ModelForms and the Rails input handling vulnerability
On 06/13/2012 06:12 PM, Doug Blank wrote: > On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyerwrote: >> 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyerwrote: > 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-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. > -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To view this discussion on the web visit https://groups.google.com/d/msg/django-developers/-/J74yecS0r5sJ. 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 13 kesä, 20:58, Carl Meyerwrote: > > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Tue, Jun 12, 2012 at 7:16 PM, Luke Plantwrote: > > = 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
Hi Anssi, On 06/13/2012 12:08 AM, Anssi Kääriäinen wrote: > On 13 kesä, 02:16, Luke Plantwrote: >> 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! > >> = 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
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 in
Re: ModelForms and the Rails input handling vulnerability
(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 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
Re: ModelForms and the Rails input handling vulnerability
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 BrongerJabber ID: torsten.bron...@jabber.rwth-aachen.de or http://bronger-jmp.appspot.com -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 13 kesä, 02:16, Luke Plantwrote: > 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! > = 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Tue, Jun 12, 2012 at 11:43 PM, Karen Traceywrote: > On Tue, Jun 12, 2012 at 10:10 PM, Alex Ogier 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 > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On 13 June 2012 09:16, Luke Plantwrote: > = 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Tue, Jun 12, 2012 at 10:10 PM, Alex Ogierwrote: > 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 -- 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.
Re: ModelForms and the Rails input handling vulnerability
On Tue, Jun 12, 2012 at 7:16 PM, Luke Plantwrote: > 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
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