possible security risk with generic views

[Anthony]

It seems the generic view files may lead to a potential security risk when a function returns more data than the developer really wants to expose publicly: https://groups.google.com/d/msg/web2py/hf9si1jlOP8/-4nrCRYhkJEJ. For example:

 

def index():
    hmac_key='sha512:101a1127-3f4d-4d23-9c70-0dfb054c7baf'
    form=SQLFORM.factory(Field('card_number', requires=CRYPT(hmac_key)))
    if form.accepts(request.vars,session):
        response.flash='Accepted'
    return locals()

 

If a user goes to /app/default/index.xml, assuming there is no 'index.xml' view, the 'generic.xml' view will expose the 'hmac_key' variable:

 

<document>

<hmac_key>sha512:101a1127-3f4d-4d23-9c70-0dfb054c7baf</hmac_key>

<form>

<form action="" enctype="multipart/form-data" method="post"><table><tr id="no_table_card_number__row"><td class="w2p_fl"><label for="no_table_card_number" id="no_table_card_number__label">Card Number: </label></td><td class="w2p_fw"><input class="string" id="no_table_card_number" name="card_number" type="text" value="" /></td><td class="w2p_fc"></td></tr><tr id="submit_record__row"><td class="w2p_fl"></td><td class="w2p_fw"><input type="submit" value="Submit" /></td><td class="w2p_fc"></td></tr></table><div class="hidden"><input name="_formkey" type="hidden" value="b895d33a-c8aa-4fa6-b7ec-d96832af57fe" /><input name="_formname" type="hidden" value="no_table/create" /></div></form>

</form>

</document>

 

Another common pitfall would be to return a Rows object that includes all fields in a table when you only want/need to expose a subset of the fields. pbreit reported this problem in the above linked thread -- he only needed the user's name from auth_user but had returned an entire row from auth_user, which exposed all the fields to the generic.html view.

 

Can/should we do anything to mitigate this risk? At the very least, maybe a warning in the book. Remedies would be to make sure your functions return only the data you are comfortable exposing publicly (using 'return locals()' can be particularly dangerous), and/or deleting some or all of the generic views on production (assuming they are not needed by the application).

 

Thoughts? Other options?

 

Anthony

[Massimo Di Pierro]

generic views are supposed to be a development tool. In production they should not be used (or should be customized). I am not convinced we need to change this but if you have any idea, let me know.

Massimo

--
mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-d...@googlegroups.com
unsubscribe: web2py-develop...@googlegroups.com
details : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official : http://www.web2py.com/

[pbreit]

I'm a little nervous that this risk exists by default. Maybe a good compromise is for the generic views to check if it's a localhost request?

Separately, I typically do "select()" without specifying any fields. Is it better practice to specify only the fields you need? Is there also a performance or memory benefit? I do not use "return locals()".

[Bruno Rocha]

should have a "good practices guideline" on top of every default file on welcome app, in this guidelines we can include the security tips etc..

Could be an online list (included in book or no), may be a separate app in web2py.com/guidelines or I can handle a web2pyslices.com/guidelines [I can build a simple app for doing that]

On that app people can submit proposals and moderators needs to approve.

In the case of generic views the problem is not in locals() but in response._vars which stores every object returned by the controller to the view.

--

Bruno Rocha

[ About me: http://zerp.ly/rochacbruno ]

On Sat, May 14, 2011 at 1:48 PM, pbreit <pbreit...@gmail.com> wrote:

I'm a little nervous that this risk exists by default. Maybe a good compromise is for the generic views to check if it's a localhost request?

Separately, I typically do "select()" without specifying any fields. Is it better practice to specify only the fields you need? Is there also a performance or memory benefit? I do not use "return locals()".

--

[Anthony]

On Saturday, May 14, 2011 11:48:40 AM UTC-4, Massimo Di Pierro wrote:

generic views are supposed to be a development tool. In production they should not be used (or should be customized).

 

Well, the Services chapter section on Generic Views doesn't make it sound as if the generic views are for development only: http://web2py.com/book/default/chapter/09#Generic-Views. If you need to return XML or JSON from several different functions, the generic views can be convenient.

 

I'm not sure if we need to change the functionality, but at the very least, there are probably a few places in the book that should warn about this.

 

Anthony

 

[Anthony]

On Saturday, May 14, 2011 1:18:38 PM UTC-4, rochacbruno wrote:

 

In the case of generic views the problem is not in locals() but in response._vars which stores every object returned by the controller to the view.

 

Yes -- the point is if you do 'return locals()', all the local variables in the function end up in response._vars, which may be more than you really want to expose to the view. Instead, it might be safer to return a dict that includes only the specific variables you really want to expose. Obviously, it depends on the specific function.

 

Anthony

[Anthony]

On Saturday, May 14, 2011 12:48:40 PM UTC-4, pbreit wrote:

 Maybe a good compromise is for the generic views to check if it's a localhost request?

 

That's not a bad idea -- then the developer has to explicitly remove the localhost check from the generic view to get it to work on production.

[Michele Comitini]

I'd leave it as it is.  Doing heuristics on developer's intentions can make things worst.  Of course warnings in the handbook are always a good thing!

mic

[Massimo Di Pierro]

+1

[Martín Mulone]

I think is an issue, adding a simple check in all generics, only to work on localhost will fix this, why we want to work this production anyways?.

2011/5/14 Massimo Di Pierro <massimo....@gmail.com>

--

 http://martin.tecnodoc.com.ar

[mr.freeze]

IMHO, the developer should not pass data to the view if they don't
want it viewed :)

On May 15, 9:11 am, Martín Mulone <mulone.mar...@gmail.com> wrote:
> I think is an issue, adding a simple check in all generics, only to work on
> localhost will fix this, why we want to work this production anyways?.
>

> 2011/5/14 Massimo Di Pierro <massimo.dipie...@gmail.com>

>
>
>
>
>
>
>
>
>
> > +1
>
> > On May 14, 2011, at 12:51 PM, Michele Comitini wrote:
>
> > I'd leave it as it is.  Doing heuristics on developer's intentions can make
> > things worst.  Of course warnings in the handbook are always a good thing!
>
> > mic

[Massimo Di Pierro]

+1

[pbreit]

Sorry, but I don't agree. This is a pretty significant security hole that is open by default. It never would have occurred to me that I have to delete the generic files to avoid a data breach. If you have to name your fields in the select() to avoid security problems, that needs to be highlighted front and center. Don't forget that it is supposedly the view that is calling the controller so one might reasonably expect that the view only has access to the data it's using.

Hope none of our critics read this thread.

[Massimo Di Pierro]

I have a proposal in trunk...

request.is_local = True or False # set automatically

request.is_https = True or False # also set automatically

(mind none is reliable if over a proxy)

Then prevent generic.html from displaying request, session, response if not request.is_local

I do not believe there is any security issue in exposing response._vars so that is still there.

massimo

On May 15, 2011, at 1:02 PM, pbreit wrote:

Sorry, but I don't agree. This is a pretty significant security hole that is open by default. It never would have occurred to me that I have to delete the generic files to avoid a data breach. If you have to name your fields in the select() to avoid security problems, that needs to be highlighted front and center. Don't forget that it is supposedly the view that is calling the controller so one might reasonably expect that the view only has access to the data it's using.

Hope none of our critics read this thread.

[Anthony]

On Sunday, May 15, 2011 3:38:36 PM UTC-4, Massimo Di Pierro wrote:

 

I do not believe there is any security issue in exposing response._vars so that is still there.

 

One potential problem with response._vars is that sometimes functions return more than just the essential data to the view. For example, you might return an entire Rows object to the view even if you only plan to display a subset of the columns (or rows). The problem is that even if you define an explicit HTML view to display just what you want in the returned HTML, if a user requests a URL with .json or .xml, the generic JSON or XML views will return the entire Rows object, even if you didn't want to expose all the columns/rows. Also, sometimes controller functions do 'return locals()', which might expose more data than desired via a generic view.

 

The problem can be avoided by (a) being very careful about what your controller functions return to the view, and/or (b) removing or altering the generic views. But because the risks are not immediately obvious, developers may not be aware that they need to do (a) or (b). The book doesn't actually discuss generic views that much, so many users may not even be aware that they are called automatically and may therefore be oblivious to the risks. Generic views are discussed in the Services chapter, but many users may not read that chapter, and even if they do, that chapter sounds like the generic views are intended to be used to deliver JSON and XML, so there's no implication that generic views should be removed (or even altered) on production.

 

Usually web2py tries to protect against potential security vulnerabilities by default rather than require the developer to be proactive. Because this vulnerability is unique to web2py (and therefore not one that developers are likely to have general knowledge about), all the more reason to be conservative with the default behavior.

 

Anthony

[Anthony]

Another common case where this might be a problem is when the controller intentionally returns some sensitive/restricted information to the view, and the view contains some authorization logic to limit the display of the restricted information to properly authorized users. In such a case, the generic views (e.g., generic.xml) will not include the necessary authorization logic and therefore end up exposing the restricted information to all users.

 

Anthony

[Massimo Di Pierro]

I agree on the problem. I need to think about the solution.

By restricting generic.* to localhost we solve the problem partially because it would still be open when running behind a proxy, we remove an important functionality of web2py (automatic .xml and .json).

[pbreit]

I appreciate the consideration on this. I think it's important that the system be secure by default and it seems like in this case we can make a good balance between out-of-the-box development functionality with in-production security.

Separately, what are peoples' thoughts on using select() vs specifying select fields? I currently do not select specific fields and I probably would prefer to avoid it. How do other frameworks handle this? I looked at Django and it seems to imply the DB access is done with "select *". Although I've read several articles about the danger or performance/memory problems with "select *".

It certainly seems that while it's more work to specify fields, it is better in production. Correct?

This might be crazy, but is it even conceivable that the necessary fields could be interpolated from the view?

[pbreit]

Rails also seems to use mostly "select *".

http://guides.rubyonrails.org/active_record_querying.html

[Martín Mulone]

Yes but with select is another thing, different to this, is up to the developer to use * or selects fields, is more for a style guide.

Agree with anthony, also I remember one time someone show a code returning global in the group. So the potential problem is out there, it's hurts, the web2py need to be secure by default, also because there are many users-starter in the programming web framework here.

Massimo you are going to fix this issue in the core of web2py or in the generic files?, because if you fix in the generic files we have to change all applications prior to the version fix. 

2011/5/16 pbreit <pbreit...@gmail.com>

Rails also seems to use mostly "select *".

http://guides.rubyonrails.org/active_record_querying.html

--

mail from:GoogleGroups "web2py-developers" mailing list
make speech: web2py-d...@googlegroups.com
unsubscribe: web2py-develop...@googlegroups.com
details : http://groups.google.com/group/web2py-developers
the project: http://code.google.com/p/web2py/
official : http://www.web2py.com/

--

 http://martin.tecnodoc.com.ar

[Anthony]

On Monday, May 16, 2011 6:14:47 AM UTC-4, Martin.Mulone wrote:

Yes but with select is another thing, different to this, is up to the developer to use * or selects fields, is more for a style guide.

Agree with anthony, also I remember one time someone show a code returning global in the group. So the potential problem is out there, it's hurts, the web2py need to be secure by default, also because there are many users-starter in the programming web framework here.

Massimo you are going to fix this issue in the core of web2py or in the generic files?, because if you fix in the generic files we have to change all applications prior to the version fix.

 

I think it might be safer to fix it in the core rather than just changing the generic files because that will fix the problem for (a) any application that upgrades, without requiring the generic files of the app (which may have been customized) to be replaced, and (b) any custom generic files that the developer may create (e.g., 'generic.csv', etc.).

 

One possibility might be to turn off automatic usage of generic views by default. Then we can create a new response attribute, such as response.generic_views. The application code can set response.generic_views to True (to allow all generic views) or to a list of allowed extensions (e.g., 'xml', 'json', etc.) -- this could be set in a model file to cover all requests (possibly for development purposes), in a controller to cover all actions in the controller, or in a specific function. The run_view_in function in compileapp.py can then check the value of response.generic_views in order to decide whether the associated generic view is allowed for the current request (assuming there is no specific view for the request). We might also consider creating a function decorator, like '@response.allow_generic()' that will set response.generic_views to the proper value when the decorated action is called (this would just be syntactic sugar).

 

Something like this would break backward compatibility, but I think that's OK to fix a security hole. And broken apps could be restored very easily by simply setting response.generic_views=True in a model file (though hopefully they would be more careful than that).

 

Anthony

 

 

[pbreit]

Could core check for "localhost" before serving generics so that people who are currently using them in development don't have to change anything?

[Massimo Di Pierro]

I have a possible solution in trunk.

It disables all generics but generic.html UNLESS you have response.allow_generic_views=True

(this will close the vulnerability on upgrade on all existing apps)

The new scaffolding app opens this with

response.allow_generic_views = request.is_local

(so the examples continue to work unless the uses change this line)

The generic.html has been notified so that it does not expose request, response, session.

Is this acceptable?

Massimo

On May 16, 2011, at 11:01 AM, pbreit wrote:

Could core check for "localhost" before serving generics so that people who are currently using them in development don't have to change anything?

[Anthony]

On Monday, May 16, 2011 1:34:27 PM UTC-4, Massimo Di Pierro wrote:

I have a possible solution in trunk.

It disables all generics but generic.html UNLESS you have response.allow_generic_views=True

(this will close the vulnerability on upgrade on all existing apps)

 

I think we should disable generic.html as well. Consider components -- they will often have a .load view defined but no .html view, so someone could see all the response._vars of a component by calling it with a .html extension (that's exactly what happened to pbreit).

 

Also, how about the idea of allowing response.allow_generic_views to be a list of allowed extensions (maybe call it response.allowed_generic_views)? I think you would just need to change compileapp.py line 459 to:

 

elif (response.allowed_generic_views and

     (response.allowed_generic_views is True or request.extension in response.allowed_generic_views)):

 

 

That will enable you to allow only specific generic views for specific functions (or simply set it to True to allow all generic views).

 

 

The new scaffolding app opens this with

response.allow_generic_views = request.is_local

 

Isn't this still dangerous if running behind a proxy? Instead of (or in addition to) checking for localhost, maybe it should check that the user is an authorized 'admin' user (i.e., the way 'appadmin' does).

 

Anthony

[Massimo Di Pierro]

OK. in trunk. Now response.allowed_generic_extension is a list of globs. If one matches the requested view and the requested view is missing, the generic view is used.

In welcome/models/db.py:

response.allowed_generic_extensions = ['*.*'] if request.is_local else []

What do you think?

[Massimo Di Pierro]

Is this getting too complex?

Massimo

[Anthony]

On Monday, May 16, 2011 2:57:03 PM UTC-4, mdipierro wrote:

Is this getting too complex?

 

Perhaps. I'm not quite sure I follow the new code. Why are you matching against response.view? Shouldn't we just let users provide a list of exact extensions (e.g., 'xml', 'json', 'rss'), and check to make sure request.extension is in that list? I'm not sure we need to allow for complex globs/regular expressions -- response.allowed_generic_extensions should just be a list of specific extensions to match, with some easy way to allow all extensions without having to explicitly list them all (maybe by setting it to True, '*', 'ALL').

 

If we're going to limit the check in db.py to just localhost, then we should at least add a comment warning about proxy setups.

 

Anthony

[Massimo Di Pierro]

You can do '*.html' or '*.json' but you can also do 'somecontroller/*.json' etc. Why not? Comes at no extra price.

[Anthony]

On Monday, May 16, 2011 4:06:19 PM UTC-4, Massimo Di Pierro wrote:

You can do '*.html' or '*.json' but you can also do 'somecontroller/*.json' etc. Why not? Comes at no extra price.

 

Sorry, now I get what you're doing. This method allows you to specify a set of 'controller/function.extension' patterns to determine which requests should allow generic views. I was originally thinking along those lines but thought it was getting unnecessarily complicated (e.g, using regular expressions), but I hadn't thought about using globs, which are a lot easier to handle.

 

In that case, though, response.allowed_generic_extensions doesn't seem like quite the right name, as you are not just specifying allowed extensions -- you are specifying which controllers and functions are allowed to use which generic views. Haven't thought of a better name yet, though (maybe something like response.generic_patterns).

 

Instead of regex.match, how about using regex.search? That way you can do 'xml' instead of '*.xml' and 'index.*' instead of '*/index.*'. Also, to allow all generics, I think you can just do '*' instead of '*.*' (whether you use match or search).

 

Also, in the code, I see 'allow_generic' and 'allowed_generic' -- I assume those are both supposed to be the same variable.

 

Anthony

[Massimo Di Pierro]

Ok with the name change. I will wait a few hours in case there are other proposals. ;-)

using regex.search causes problems. For example 'xml' would allow "somexmlaction.html". Currently * matches both / and . therefore we can already do '*' instead of '*/*.*'.

Massimo

[Anthony]

On Monday, May 16, 2011 6:09:53 PM UTC-4, Massimo Di Pierro wrote:

Ok with the name change. I will wait a few hours in case there are other proposals. ;-)

using regex.search causes problems. For example 'xml' would allow "somexmlaction.html". Currently * matches both / and . therefore we can already do '*' instead of '*/*.*'.

 

Normally regex.search would allow 'xml' to match anywhere in a string, but because you're using fnmatch.translate to translate globs to regexes, 'xml' is required to be at the *end* of the string, so it will only match an 'xml' extension (the string being matched always ends in an extension).

 

>>> fnmatch.translate('xml')
'xml\\Z(?ms)'

>>> re.search(fnmatch.translate('xml'), 'somexmlaction.html') is None
True

Using regex.match forces you to wildcard the controller even if you just want to specify a function name:

 

>>> re.match(fnmatch.translate('index.*'), 'default/index.html') is None
True

 

So, I think we're OK with regex.search, but I may be missing something.

 

Anthony

[DenesL]

The best policy would still be to discourage the return of locals,
globals and broad selects from controllers.
Return only what you need.

[Massimo Di Pierro]

how about index.xhtml?

[Bruno Rocha]

I normally use locals() as return, but this is whar I do:

---controller --------

def myfunction():

    _hidden_object = request.args(0)

    _other_hidden_object = some_func(_hidden_object)

    visible_object = db(db.table.field==_other_hidden_object).select()

    return locals()

--------------------------

In the example above, only the 'visible_object' will be visible in the view, the '_' objects will not be returned 

 

[Massimo Di Pierro]

neat trick!

[Anthony]

On Monday, May 16, 2011 8:14:50 PM UTC-4, Massimo Di Pierro wrote:

how about index.xhtml?

 

Hmm, I'm not sure I have a problem with that. If you want to allow .html but disallow .xhtml, you can still do '*.html', but at least you have the option of just doing 'html' if you want to allow both or if you know you don't have a 'generic.xhtml' view (which is not included in the 'welcome' app anyway).

 

regex.match basically requires that your pattern must match the beginning (and end) of the 'controller/function.extension' string, and regex.search requires only that it match the end of the string (assuming globs translated by fnmatch.translate). I guess search is a little more flexible, at the expense of requiring a bit more care. I like search, but if you want to stick with match, it's not a big deal. :-)

 

Anthony

 

[Massimo Di Pierro]

I do not have a strong opinion. I am just playing devil's advocate to make sure we have considered all options.

I would like to hear a few more options about this. If there is no opposition I am happy to change it from match to search.

[Anthony]

Note, lines 461 and 466 in compileapp.py have 'allowed_generic', but I think it should be 'allow_generic' (which is the variable name defined earlier).

 

Anthony