diff options
| -rw-r--r-- | doc/reference/guidelines.rst | 72 | ||||
| -rw-r--r-- | doc/reference/security.rst | 234 | ||||
| -rw-r--r-- | doc/webservices/odoo.rst | 2 |
3 files changed, 235 insertions, 73 deletions
diff --git a/doc/reference/guidelines.rst b/doc/reference/guidelines.rst index a0a6fb8fcc6..91bf367fbdd 100644 --- a/doc/reference/guidelines.rst +++ b/doc/reference/guidelines.rst @@ -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 ------------ @@ -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 ~~~~~~~~~~~~~~~~ diff --git a/doc/reference/security.rst b/doc/reference/security.rst index fd6402893c3..6e072372257 100644 --- a/doc/reference/security.rst +++ b/doc/reference/security.rst @@ -68,7 +68,7 @@ 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: @@ -76,8 +76,6 @@ This means the first *group rule* restricts access, but any further 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`). @@ -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. diff --git a/doc/webservices/odoo.rst b/doc/webservices/odoo.rst index dfd99177ebd..cdbbd036e46 100644 --- a/doc/webservices/odoo.rst +++ b/doc/webservices/odoo.rst @@ -333,6 +333,8 @@ the login. common_config, "authenticate", asList( db, username, password, emptyMap())); +.. _webservices/odoo/calling_methods: + Calling methods =============== |