Skip to content

Commit

Permalink
[ADD] doc: sescurity pitfalls
Browse files Browse the repository at this point in the history 
Write guidelines on how to write secure code and avoid commons
mistakes.

Update outdated information and move existing security guidelines to
the same section.

closes #58060

X-original-commit: 1018427
Signed-off-by: Martin Trigaux (mat) <mat@odoo.com>
  • Loading branch information
@mart-e
mart-e committed Sep 18, 2020
1 parent 17c39f2 commit a7eec1d
Show file tree
Hide file tree
Showing 3 changed files with 235 additions and 73 deletions.
72 changes: 5 additions & 67 deletions doc/reference/guidelines.rst
View file
Expand Up @@ -396,6 +396,11 @@ based upon the first one.
Python
======

.. warning::

Do not forget to read the :ref:`Security Pitfalls <reference/security/pitfalls>`
section as well to write secure code.

PEP8 options
------------

Expand Down Expand Up @@ -638,73 +643,6 @@ isolate its impact. A good example are the keys of ``mail`` module :
*mail_create_nosubscribe*, *mail_notrack*, *mail_notify_user_signature*, ...


Do not bypass the ORM
~~~~~~~~~~~~~~~~~~~~~
You should never use the database cursor directly when the ORM can do the same
thing! By doing so you are bypassing all the ORM features, possibly the
transactions, access rights and so on.

And chances are that you are also making the code harder to read and probably
less secure.

.. code-block:: python
# very very wrong
self.env.cr.execute('SELECT id FROM auction_lots WHERE auction_id in (' + ','.join(map(str, ids))+') AND state=%s AND obj_price > 0', ('draft',))
auction_lots_ids = [x[0] for x in self.env.cr.fetchall()]
# no injection, but still wrong
self.env.cr.execute('SELECT id FROM auction_lots WHERE auction_id in %s '\
'AND state=%s AND obj_price > 0', (tuple(ids), 'draft',))
auction_lots_ids = [x[0] for x in self.env.cr.fetchall()]
# better
auction_lots_ids = self.search([('auction_id','in',ids), ('state','=','draft'), ('obj_price','>',0)])
No SQL injections, please !
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Care must be taken not to introduce SQL injections vulnerabilities when using
manual SQL queries. The vulnerability is present when user input is either
incorrectly filtered or badly quoted, allowing an attacker to introduce
undesirable clauses to a SQL query (such as circumventing filters or
executing UPDATE or DELETE commands).

The best way to be safe is to never, NEVER use Python string concatenation (+)
or string parameters interpolation (%) to pass variables to a SQL query string.

The second reason, which is almost as important, is that it is the job of the
database abstraction layer (psycopg2) to decide how to format query parameters,
not your job! For example psycopg2 knows that when you pass a list of values
it needs to format them as a comma-separated list, enclosed in parentheses !

.. code-block:: python
# the following is very bad:
# - it's a SQL injection vulnerability
# - it's unreadable
# - it's not your job to format the list of ids
self.env.cr.execute('SELECT distinct child_id FROM account_account_consol_rel ' +
'WHERE parent_id IN ('+','.join(map(str, ids))+')')
# better
self.env.cr.execute('SELECT DISTINCT child_id '\
'FROM account_account_consol_rel '\
'WHERE parent_id IN %s',
(tuple(ids),))
This is very important, so please be careful also when refactoring, and most
importantly do not copy these patterns!

Here is a memorable example to help you remember what the issue is about (but
do not copy the code there). Before continuing, please be sure to read the
online documentation of pyscopg2 to learn of to use it properly:

- The problem with query parameters (http://initd.org/psycopg/docs/usage.html#the-problem-with-the-query-parameters)
- How to pass parameters with psycopg2 (http://initd.org/psycopg/docs/usage.html#passing-parameters-to-sql-queries)
- Advanced parameter types (http://initd.org/psycopg/docs/usage.html#adaptation-of-python-values-to-sql-types)


Think extendable
~~~~~~~~~~~~~~~~

Expand Down
234 changes: 228 additions & 6 deletions doc/reference/security.rst
View file
Expand Up @@ -68,16 +68,14 @@ This means the first *group rule* restricts access, but any further
*group rule* expands it, while *global rules* can only ever restrict access
(or have no effect).

.. warning:: record rules do not apply to the Administrator user
.. warning:: record rules do not apply to the Superuser account
:class: aphorism

.. _reference/security/fields:

Field Access
============

.. versionadded:: 7.0

An ORM :class:`~odoo.fields.Field` can have a ``groups`` attribute
providing a list of groups (as a comma-separated string of
:term:`external identifiers`).
Expand All @@ -93,8 +91,232 @@ access to the field:

.. todo::

field access groups apply to administrator in fields_get but not in
field access groups apply to the Superuser in fields_get but not in
read/write...

.. _foo: http://google.com
.. _time module: https://docs.python.org/2/library/time.html
.. _time module: https://docs.python.org/3/library/time.html


.. _reference/security/pitfalls:

Security Pitfalls
=================

As a developer, it is important to understand the security mechanisms and avoid
common mistakes leading to insecure code.

Unsafe Public Methods
---------------------

Any public method can be executed via a :ref:`RPC call
<webservices/odoo/calling_methods>` with the chosen parameters. The methods
starting with a ``_`` are not callable from an action button or external API.

On public methods, the record on which a method is executed and the parameters
can not be trusted, ACL being only verified during CRUD operations.

.. code-block:: python
# this method is public and its arguments can not be trusted
def action_done(self):
if self.state == "draft" and self.user_has_groups('base.manager'):
self._set_state("done")
# this method is private and can only be called from other python methods
def _set_state(self, new_state):
self.sudo().write({"state": new_state})
Making a method private is obviously not enough and care must be taken to use it
properly.

Bypassing the ORM
-----------------
You should never use the database cursor directly when the ORM can do the same
thing! By doing so you are bypassing all the ORM features, possibly the
automated behaviours like translations, invalidation of fields, ``active``,
access rights and so on.

And chances are that you are also making the code harder to read and probably
less secure.

.. code-block:: python
# very very wrong
self.env.cr.execute('SELECT id FROM auction_lots WHERE auction_id in (' + ','.join(map(str, ids))+') AND state=%s AND obj_price > 0', ('draft',))
auction_lots_ids = [x[0] for x in self.env.cr.fetchall()]
# no injection, but still wrong
self.env.cr.execute('SELECT id FROM auction_lots WHERE auction_id in %s '\
'AND state=%s AND obj_price > 0', (tuple(ids), 'draft',))
auction_lots_ids = [x[0] for x in self.env.cr.fetchall()]
# better
auction_lots_ids = self.search([('auction_id','in',ids), ('state','=','draft'), ('obj_price','>',0)])
SQL injections
~~~~~~~~~~~~~~
Care must be taken not to introduce SQL injections vulnerabilities when using
manual SQL queries. The vulnerability is present when user input is either
incorrectly filtered or badly quoted, allowing an attacker to introduce
undesirable clauses to a SQL query (such as circumventing filters or
executing ``UPDATE`` or ``DELETE`` commands).

The best way to be safe is to never, NEVER use Python string concatenation (+)
or string parameters interpolation (%) to pass variables to a SQL query string.

The second reason, which is almost as important, is that it is the job of the
database abstraction layer (psycopg2) to decide how to format query parameters,
not your job! For example psycopg2 knows that when you pass a list of values
it needs to format them as a comma-separated list, enclosed in parentheses !

.. code-block:: python
# the following is very bad:
# - it's a SQL injection vulnerability
# - it's unreadable
# - it's not your job to format the list of ids
self.env.cr.execute('SELECT distinct child_id FROM account_account_consol_rel ' +
'WHERE parent_id IN ('+','.join(map(str, ids))+')')
# better
self.env.cr.execute('SELECT DISTINCT child_id '\
'FROM account_account_consol_rel '\
'WHERE parent_id IN %s',
(tuple(ids),))
This is very important, so please be careful also when refactoring, and most
importantly do not copy these patterns!

Here is a memorable example to help you remember what the issue is about (but
do not copy the code there). Before continuing, please be sure to read the
online documentation of pyscopg2 to learn of to use it properly:

- `The problem with query parameters <http://initd.org/psycopg/docs/usage.html#the-problem-with-the-query-parameters>`_
- `How to pass parameters with psycopg2 <http://initd.org/psycopg/docs/usage.html#passing-parameters-to-sql-queries>`_
- `Advanced parameter types <http://initd.org/psycopg/docs/usage.html#adaptation-of-python-values-to-sql-types>`_
- `Psycopg documentation <https://www.psycopg.org/docs/sql.html>`_

Unescaped field content
-----------------------

When rendering content using JavaScript and XML, one may be tempted to use
a ``t-raw`` to display rich-text content. This should be avoided as a frequent
`XSS <https://en.wikipedia.org/wiki/Cross-site_scripting>`_ vector.

It is very hard to control the integrity of the data from the computation until
the final integration in the browser DOM. A ``t-raw`` that is correctly escaped
at the time of introduction may no longer be safe at the next bugfix or
refactoring.

.. code-block:: javascript
QWeb.render('insecure_template', {
info_message: "You have an <strong>important</strong> notification",
})
.. code-block:: xml
<div t-name="insecure_template">
<div id="information-bar"><t t-raw="info_message" /></div>
</div>
The above code may feel safe as the message content is controlled but is a bad
practice that may lead to unexpected security vulnerabilities once this code
evolves in the future.

.. code-block:: javascript
// XSS possible with unescaped user provided content !
QWeb.render('insecure_template', {
info_message: "You have an <strong>important</strong> notification on " \
+ "the product <strong>" + product.name + "</strong>",
})
While formatting the template differently would prevent such vulnerabilities.

.. code-block:: javascript
QWeb.render('secure_template', {
message: "You have an important notification on the product:",
subject: product.name
})
.. code-block:: xml
<div t-name="secure_template">
<div id="information-bar">
<div class="info"><t t-esc="message" /></div>
<div class="subject"><t t-esc="subject" /></div>
</div>
</div>
.. code-block:: css
.subject {
font-weight: bold;
}
Evaluating content
------------------
Some may want to ``eval`` to parse user provided content. Using ``eval`` should
be avoided at all cost. A safer, sandboxed, method :class:`~odoo.tools.safe_eval`
can be used instead but still gives tremendous capabilities to the user running
it and must be reserved for trusted privileged users only as it breaks the
barrier between code and data.

.. code-block:: python
# very bad
domain = eval(self.filter_domain)
return self.search(domain)
# better but still not recommended
from odoo.tools import safe_eval
domain = safe_eval(self.filter_domain)
return self.search(domain)
# good
from ast import literal_eval
domain = literal_eval(self.filter_domain)
return self.search(domain)
Parsing content does not need ``eval``

========== ================== ================================
Language Data type Suitable parser
========== ================== ================================
Python int, float, etc. int(), float()
Javascript int, float, etc. parseInt(), parseFloat()
Python dict json.loads(), ast.literal_eval()
Javascript object, list, etc. JSON.parse()
========== ================== ================================

Accessing object attributes
---------------------------

If the values of a record needs to be retrieved or modified dynamically, one may
want to use the ``getattr`` and ``setattr`` methods.

.. code-block:: python
# unsafe retrieval of a field value
def _get_state_value(self, res_id, state_field):
record = self.sudo().browse(res_id)
return getattr(record, state_field, False)
This code is however not safe as it allows to access any property of the record,
including private attributes or methods.

The ``__getitem__`` of a recordset has been defined and accessing a dynamic
field value can be easily achieved safely:

.. code-block:: python
# better retrieval of a field value
def _get_state_value(self, res_id, state_field):
record = self.sudo().browse(res_id)
return record[state_field]
The above method is obviously still too optimistic and additional verifications
on the record id and field value must be done.
2 changes: 2 additions & 0 deletions doc/webservices/odoo.rst
View file
Expand Up @@ -333,6 +333,8 @@ the login.
common_config, "authenticate", asList(
db, username, password, emptyMap()));
.. _webservices/odoo/calling_methods:

Calling methods
===============

Expand Down

0 comments on commit a7eec1d

Please sign in to comment.